search

rust-windowing / winit

Window handling library in pure Rust
https://docs.rs/winit/
Apache License 2.0
4.86k stars 909 forks source link

On Android, `winit` should panic instead of returning `(0, 0)` from `{inner,outer}_size()` when `native_window()` returns `None` #2482

Open ElhamAryanpur opened 2 years ago

ElhamAryanpur commented 2 years ago

As title says, trying out winit 0.26, it works fine, but on 0.27, window.inner_size and outer_size returns PhysicalSize { width: 0, height: 0 }.

To reproduce, just make a window and call inner_size or outer_size.

ElhamAryanpur commented 2 years ago

On desktop it's fine, works, only on android does it give that value. And yes I didn't try to set size, I tried default options for window and tried without windowbuilder too. All have same results

madsmtm commented 2 years ago

Possible cause: ndk-glue bump from v0.6 to v0.7, @MarijnS95?

msiglreith commented 2 years ago

As it interally relys on the native surface it suffers from the same restrictions as in access to the raw window handle: between Resume/Suspend only. We either should document this better or even panic similar to accessing the raw window handle when it's not available.

ElhamAryanpur commented 2 years ago

That'd be really hard to initialize things before the Resume tho... As in the whole engine in my case... It'll be hard to do pre loop things too as it all require the size.

Since resizes are not allowed on mobile, wouldn't it be possible to just send the window or screensize without needing to be in Resumed like before?

MarijnS95 commented 2 years ago

@madsmtm This has always been the case, as also mentioned by @msiglreith. We've previously discussed that a Window is just an empty shell that realistically shouldn't exist before the Resumed call, as Android doesn't know the size at that moment either (no Window is open).

For the offending code resulting in (0, 0):

https://github.com/rust-windowing/winit/blob/a4695c539783a8f8118cbfa783c93fee9b222325/src/platform_impl/android/mod.rs#L883-L891

MarijnS95 commented 2 years ago

Since resizes are not allowed on mobile, wouldn't it be possible to just send the window or screensize without needing to be in Resumed like before?

Resizes are definitely allowed on mobile; take a look at:

  1. Split-screen;
  2. Free-form windows;
  3. Landscape vs portrait.

So it isn't really possible to hardcode a size somewhere or cache it while a window isn't open. After all, you already need to cater surface creation and destruction to these events and it shouldn't be a far stretch for an engine to do the same with the Window/size. It is typically trivial to implement this if an engine already supports resizes, just need to make sure that no window size is needed/used before a Window appears (or an ugly workaround implemented in some engines/apps; hardcode an initial window size and resize everything when a Window is available and the engine finally knows the actual size).

ElhamAryanpur commented 2 years ago

Since resizes are not allowed on mobile, wouldn't it be possible to just send the window or screensize without needing to be in Resumed like before?

Resizes are definitely allowed on mobile; take a look at:

  1. Split-screen;
  2. Free-form windows;
  3. Landscape vs portrait.

So it isn't really possible to hardcode a size somewhere or cache it while a window isn't open. After all, you already need to cater surface creation and destruction to these events and it shouldn't be a far stretch for an engine to do the same with the Window/size. It is typically trivial to implement this if an engine already supports resizes, just need to make sure that no window size is needed/used before a Window appears (or an ugly workaround implemented in some engines/apps; hardcode an initial window size and resize everything when a Window is available and the engine finally knows the actual size).

Understandable.

My engine have resize capability and main issue was that event loop was ran last as game loop than checks for context creation. So my planned workaround is as you stated, a pre given value for size and then resize when a window is given. Thanks for the replies!

MarijnS95 commented 2 years ago

We should leave this issue open and fix the bad default value as described by @msiglreith (panicking has my preference, to match other uses of native_window() inside winit).

Unfortunately I seem to have lost (or never had?) permissions to reopen and retitle issues in the winit repo.

ElhamAryanpur commented 2 years ago

We should leave this issue open and fix the bad default value as described by @msiglreith (panicking has my preference, to match other uses of native_window() inside winit).

Unfortunately I seem to have lost (or never had?) permissions to reopen and retitle issues in the winit repo.

Reopened, lmk what to change title to

MarijnS95 commented 2 years ago

On Android, winit should panic instead of returning (0, 0) from {inner,outer}_size() when native_window() returns None

ElhamAryanpur commented 2 years ago

Done

kchibisov commented 2 years ago

@MarijnS95 I've restored your access to winit, was pretty strange that you've lost it.

Are you present on matrix/IRC in some form?

MarijnS95 commented 2 years ago

@kchibisov May have gotten lost as part of https://github.com/rust-windowing/winit/issues/2319. I'll join the Matrix channel and space shortly!

rib commented 2 years ago

Since it's tangentially related, I just wanted to link this to https://github.com/rust-windowing/winit/issues/2308 - which advocates for clearer semantics for APIs like inner_size across platforms.

In particular that would introduce the idea of a surface_size which would be more closely connected with the NativeWindow (aka SurfaceView) and maybe it would make sense for that API to be able to return None.

On Android the inner_size currently has unclear semantics because downstream crates (such as Bevy/wgpu) use the inner size to determine the surface size which doesn't really make sense on platforms that would want to represent surface insets (e.g. mobile OSs and potentially Wayland with client-side decorations). (In these cases the actual size of a render target / surface will be larger than the inset size)

It's a bit debatable whether the insets on Android really relate to the SurfaceView which is the thing that disappears between Suspended and Resumed events so I'm not entirely sure {inner,outer}_size should even need to be affected at all while an Android app is suspended.

I'd like to investigate whether we can remove the special case for returning (0,0) here while also supporting returning inset bounds on Android.