Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial Window Size #106

Open
daxpedda opened this issue May 13, 2023 · 21 comments
Open

Initial Window Size #106

daxpedda opened this issue May 13, 2023 · 21 comments

Comments

@daxpedda
Copy link
Member

Currently the initial window size is always 0 in width and height, this will lead to panics when calling buffer_mut() if the size wasn't set with resize() depending on the platform:

I would prefer if we make this consistent on all platforms. My current ideas:

  1. Require an initial size in Surface::new().
  2. Set the initial size to the window size (don't know if that's possible on all platforms).
  3. Introduce a new error variant instead of panicking.

Currently leaning towards idea 1.

@notgull
Copy link
Member

notgull commented May 13, 2023

Idea 1 sounds good to me, since an initial window size is also required by wgpu.

@ids1024
Copy link
Member

ids1024 commented May 13, 2023

This should definitely be consistent across backends, if nothing else.

Set the initial size to the window size (don't know if that's possible on all platforms).

I don't believe that's possible on Wayland, no. Only the code that created the surface has this information.

Introduce a new error variant instead of panicking.

This was discussed somewhat in #65, and I think it was generally agreed that panicking is a better way to handle programmer error than returning an error variant. So I think panicking consistently would be reasonable. Possibly if things were restructured a bit that could be handled in top level code instead of separately in each backend.

But requiring passing an initial size would statically prevent errors, so that's better if it doesn't make the library too much harder to use. Good point about Wgpu requiring that. That makes it seem more reasonable.

@LoganDark
Copy link

LoganDark commented Aug 25, 2023

Doesn't the contract of buffer_mut state that you always must call resize first? Or are you looking into making this unnecessary?

I call resize each frame even if the size hasn't changed. For consistency's sake it's just better to ensure the buffer size always matches the window size.

@daxpedda
Copy link
Member Author

Or are you looking into making this unnecessary?

Yeah, that was the original thought.

@LoganDark
Copy link

LoganDark commented Aug 25, 2023

Or are you looking into making this unnecessary?

Yeah, that was the original thought.

Hmm, what's the point of making it unnecessary? "every frame starts with a resize call" is a good rule of thumb as-is, I can't really see a reason to try to eliminate it if you're going to call resize every other time anyway.

Truth be told, I'd advocate for combining resize and buffer_mut into something like begin_frame.

@daxpedda
Copy link
Member Author

Hmm, what's the point of making it unnecessary? "every frame starts with a resize call" is a good rule of thumb as-is, I can't really see a reason to try to eliminate it if you're going to call resize every other time anyway.

It's an improvement, I don't see why we should have an invalid state if it's avoidable (it's not clear to me yet if it can be avoided). I'm also really really not a big fan of panics in libraries, but I'm not sure who shares that opinion.

Truth be told, I'd advocate for combining resize and buffer_mut into something like begin_frame.

I'm not sure why we should require passing in a size when it's not required, it's up to you if you wanna do it that way but I wouldn't want to force it on everybody else. But it's a very good alternative solution.

@LoganDark
Copy link

LoganDark commented Aug 25, 2023

I'm not sure why we should require passing in a size when it's not required, it's up to you if you wanna do it that way but I wouldn't want to force it on everybody else. But it's a very good alternative solution.

what do you mean "force"? is passing the size for each frame somehow a problem or inconvenience? don't you need those anyway in order to use the buffer properly? is there any use-case that would prevent someone from being able to pass the size? is it too expensive for softbuffer to check if the passed-in size is already the same as the buffer size and simply reuse the buffer if so?

(I'm just confused)

@daxpedda
Copy link
Member Author

is passing the size for each frame somehow a problem or inconvenience?

It isn't. It's why I said this is a very good alternative. But it doesn't have a benefit either unless it's an alternative to passing the size at the start.

I see the point of course that we could encourage users to always resize before drawing or fetching, but that's a completely different issue then whats being discussed here.

@LoganDark
Copy link

It isn't. It's why I said this is a very good alternative.

I'm not trying to be adversarial here. I just don't know why you would want to elide the resize call in the first place. Does it have some obvious benefit that I'm not seeing?

@daxpedda
Copy link
Member Author

I'm not trying to be adversarial here.

All good!

I just don't know why you would want to elide the resize call in the first place.

I'm not particularly against it, but I'm not particularly for it either.

Does it have some obvious benefit that I'm not seeing?

I don't see any particular benefit to it, which is why I'm not particularly in favor of it.
Did you maybe mean downside instead of benefit? No, but I don't see a reason to add it either, except again, as an alternative to what we were initially discussing.

@LoganDark
Copy link

Did you maybe mean downside instead of benefit?

I meant a benefit to eliding the call to resize. In case you're curious, I just completed a round of benchmarks on some feature branches and resize calls typically take under a microsecond, even going from a window size of 1600x1200 to 2880x1800. I don't see why anyone would need to eliminate this call by providing an initial size for the surface, nor why they would need to care enough to call resize only when surface size is changed rather than just every frame.

@daxpedda
Copy link
Member Author

I didn't really dispute anything you are saying here.

But as I pointed out in #106 (comment), eliding the resize() call is only an indirect goal, my point is to prevent leaving Softbuffer in an invalid state that forces us to panic.
Maybe this is what was confusing you?

@LoganDark
Copy link

my point is to prevent leaving Softbuffer in an invalid state that forces us to panic.

Oh, because the buffer size must always be non-zero, but when you first create the Surface, it is zero anyway? That does make more sense, yeah.

@kpreid
Copy link
Contributor

kpreid commented Aug 25, 2023

My 2¢: Passing the size every frame as part of the drawing (.buffer_mut([width, height])?) would likely lead to simpler (less repetitive) code for callers, compared to requiring it at creation — the size only needs to be considered and translated in one place instead of two. Additionally, it makes sense from the perspective of getting the buffer length right — the code that is about to write will specify the length and layout it needs to write into.

@LoganDark
Copy link

.buffer_mut([width, height])?

They should probably be separate arguments rather than just an array :)

@kpreid
Copy link
Contributor

kpreid commented Aug 25, 2023

I disagree, actually — it would be easier to write graphics API glue if everyone used some kind of 2d vector type instead of separate x and y args, and arrays are cheap whether or not you care — but that's not relevant to this issue.

@LoganDark
Copy link

LoganDark commented Aug 25, 2023

I disagree, actually — it would be easier to write graphics API glue if everyone used some kind of 2d vector type instead of separate x and y args, and arrays are cheap whether or not you care — but that's not relevant to this issue.

I use tuples for that case. You're right though, this API change belongs in a separate discussion

@daxpedda
Copy link
Member Author

Man, this issue is getting really off-topic.

I would also prefer using a shared 2D vector type, but I don't like using an array instead. Indeed I would be more happy with a tuple.

@kpreid
Copy link
Contributor

kpreid commented Aug 25, 2023

I apologize for bringing up an unrelated issue in my example. I intended only to suggest that the simplest API that is convenient for both parties (application and library), and free of invalid states, is one where the size is always specified at the moment the buffer is being obtained.

(Though, on further thought, perhaps in some cases there could be latency improvements by resizing sooner than just before drawing? Depends on how the backends treat a resize operation.)

@LoganDark
Copy link

sooner than just before drawing

This isn't really a thing because you want to draw immediately after a window resize. So there's never going to be any significant gap between resizing the buffer and drawing the next frame.

@jdahlstrom
Copy link

jdahlstrom commented Sep 15, 2023

IMHO, window resizing on every frame is superfluous if the window is not resizable, which is a common case with games. In these cases I'd prefer not having to call resize separately. How about a new constructor method eg. Surface::with_size?

  • MacOS: doesn't panic.

(Does panic as of e10c3609)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

6 participants