-
Notifications
You must be signed in to change notification settings - Fork 909
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
Consistently deliver a Resumed
event on all platforms
#2331
Conversation
Nit: you don't need to have |
Resumed
event on all platforms
:) Nit nit: A few benefits to
|
I just think users weren't wrapping it like that, so perhaps this API change might already blow up if they created e.g. a swapchain, and then create it again in They certainly don't/didn't need to, which is the word that was used in the description that I'm nitting about 😅 - but they did need to have it around their code outside the event loop that (indirectly) calls into |
Something to consider here is that the existing I think there's probably still some value in having some portable lifecycle events like "suspend" and "resume" that have reasonably practical mappings on Android and iOS - even though there's some nuance to how we define "suspended" on Android. (and as an asside I recall thinking the android backend should probably be mapping "suspended" to the logical OR of 'lifecycle suspended' and 'window destroyed', instead of directly to the 'window destroyed' state). Since the mapping is a bit fuzzy if you need to handle some platform specific details I tend to think it would also be good if there was a practical way to also notified about Android or iOS specific lifecycle events. Maybe instead of having to expose any platform-specific details there could be some general What's extra funky here though is that the |
I concede the nit battle :) yeah, existing apps wouldn't necessarily need to Since I've been guarding the android-specific Resumed path in some apps I've written recently (just to give me one fewer thing to consider in case we do end up landing a change like this) then my wording was probably just echoing what I've felt I needed to do to write portable, forwards-compatible winit code. |
adc4d35
to
cdf12ca
Compare
Ok, with the updates I've pushed I bit the bullet and wrote much more comprehensive documentation for both the I had been thinking it could be good to try and deal with documentation improvements separately but while it makes sense to try and some general recommendations about handling these events it's a good opportunity to add more details too. I've updated the commit message in line with these changes, and some of the comments above and I've now marked the patch as Resolving #2185 I updated the web backend change as suggested by @MarijnS95 above and at least smoke tested the change via Although this has provoked some interesting discussions around some of the Android specific thorns I think for now most of that looks like it probably deserves follow up issues + discussions and hopefully doesn't need to block making this incremental change for improved consistency first. |
Right, that makes this situation a bit more complicated as I have zero experience with iOS. Are these events purely used for window/surface creation and destruction there too?
Correct, hence, my linking to #2118 - that PR author also got thrown off by the unfortunate naming, as you said:
They are not, which is why I think we should have separate events that represent window creation/destruction. Android doesn't ever raise
Exactly, I'll have to go over a bunch of applications and make sure that we're either guarding these or move the initialization for all platforms into
Fair enough, if your documentation looks good we should get this in as it is clearly an improvement over the status-quo - and separately before |
https://developer.apple.com/documentation/uikit/uiapplicationdelegate/1622956-applicationdidbecomeactive / https://developer.apple.com/documentation/uikit/uiapplicationdelegate/1622950-applicationwillresignactive (these callbacks map to More fuel for switching these to |
src/event.rs
Outdated
/// | ||
/// Also see [`Resumed`][Self::Resumed] notes. | ||
/// | ||
/// # Android |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this fits under the Portability
umbrella, should we make this h2 with two ##
?
src/event.rs
Outdated
/// destroyed, which indirectly invalidates any existing render surfaces that may have been | ||
/// created outside of Winit (such as an `EGLSurface`, `VkSurfaceKHR` or [`wgpu::Surface`][wgpu_surface]). | ||
/// | ||
/// [wgpu_surface]: https://docs.rs/wgpu/latest/wgpu/struct.Surface.html |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also make this [`wgpu::Surface`]
and remove the [wgpu_surface]
link target entirely. Same for android_ls
.
Shall we also add links for EGLSurface
and (Ash's) VkSurfaceKHR
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep the short hand wgpu::Surface linking makes sense.
There isn't really a canonical link for EGLSurface
- the official EGL spec is a Khronos PDF. The Vulkan spec is at least published as HTML so could maybe link to the spec but not sure it's that important for either really.
Not sure about the "Same for android_ls
" comment - that's a link about the Android activity life cycle so there wouldn't be a shorthand way of making that link?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can make that an [Android lifecycle]: https...
too.
Yeah, link Vulkan if easy, I tried linking (E)GL in the ndk (see recent doc improvement PRs) and it's a bit of a hit and miss. Only need this if it's easy and makes it more clear to the user what you're talking about / referencing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yeah I misinterpreted the comment about wgpu::Surface because I thought for a second there was dependency on wgpu::Surface and so we could rely on rustdoc figuring out the link, but that wasn't what you meant, you're meaning to avoid the separate link target which applies to the android_ls link too 👍
It's the other way around - they are purely iOS lifecycle based - and I'm fairly sure that there's no equivalent requirement to destroy and recreate render surfaces on iOS like there is on Android. With the way they are currently mapped to the iOS lifecycle events though I have a feeling that it could be slightly better to map |
Yeah, for sure, if we can land this change I'd be interested in seeing some (all ideally) of the examples be updated to demonstrate lazy initialization of graphics state via the Ideally the first-party examples in the Winit repo can try to demonstrate how to write portable code that will run on all supported platforms - even if there are things that could be improved about the API to make it harder to write non-portable code. Some of the examples here, demonstrate winit applications that can run on both Android and Windows: It would be good to verify the same lazy initialization via |
right, this is why I was suggesting the tweak on iOS that they should emit I currently still think it's reasonable to keep the Another way to think about this is could be to essentially forget about "lifecycle" events for a moment and consider that those are just implementation details for the backend. The purpose of the Winit events would conceptually be to delimit when applications are blocked from from rendering. On iOS the implementation would map to being "inactive" and on Android that would map to "being paused OR not having a surface view" - but the platform specific lifecycle state is really just an implementation detail. |
I think resume if fine, however could we name them where enum SuspendAction {
ClearGraphicsContenxt,
None,
ThrottleDrawing
} That was I think you can map it to anything and it'll be the most portable thing, since desktop platforms will have The issue when the window should be created is also related to macOS, since the window should e created when the event loop is running, otherwise it could miss on some features iirc. The problem with |
There's still quite a difference between "hey stop rendering now" and "hey you must recreate your graphics context/surface otherwise things don't work", and I think @kchibisov's suggestion solves that.
We can have an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm okay with this after fixing the is is
typo, and bringing up the same lifecycle-paused vs surface destroyed question: which may be simply addressed by s/Or/And
which has been done for Resumed
but not Suspended
?
6c775dc
to
9975660
Compare
I just pushed an update that resolves the changelog conflict and fixes an "is is" typo that @MarijnS95 pointed out. I haven't changed the wording about the implementation details that @MarijnS95 has recently commented on while I think the current wording is a correct reflection of the android-specific details. There might be some better wording we could come up with to clarify further but I'd like to keep the information itself which the last suggested changes wouldn't do imho. I also figure that this kind of re-wording can happen after the change lands. Even though this does try to improve the state of the docs for the Suspended/Resumed events, I think the technical consistency change is more important. |
9975660
to
b904d72
Compare
last push tries to add a couple of lines to further clarify the android-specific logic for suspended/resumed events and fixes a copy and paste error that was noticed from @MarijnS95's comments: --- a/src/event.rs
+++ b/src/event.rs
@@ -97,6 +97,9 @@ pub enum Event<'a, T: 'static> {
/// 2. Or the application has had its associated [`SurfaceView`] destroyed (which is expected
/// to closely correlate with being paused)
///
+ /// _(I.e. **either** of these may lead to a `Suspended` event, but applications should act
+ /// as if both have happened.)_
+ ///
/// Applications that need to run on Android should assume their [`SurfaceView`] has been
/// destroyed, which indirectly invalidates any existing render surfaces that may have been
/// created outside of Winit (such as an `EGLSurface`, [`VkSurfaceKHR`] or [`wgpu::Surface`]).
@@ -145,11 +148,13 @@ pub enum Event<'a, T: 'static> {
///
/// ## Android
///
- /// On Android specifically, the [`Suspended`] event relates to two things:
+ /// On Android specifically, the `Resumed` event relates to two things:
/// 1. The application has resumed (according to the [Activity lifecycle])
/// 2. And the application has an associated [`SurfaceView`] (which is expected to closely correlate
/// with being resumed)
///
+ /// _(I.e. **both** of these must happen before the application gets a `Resumed` event.)_
+ ///
/// Applications that need to run on Android must wait until they have been `Resumed`
/// before they will be able to create a render surface (such as an `EGLSurface`,
/// [`VkSurfaceKHR`] or [`wgpu::Surface`]) which depend on having a |
b904d72
to
a0e2601
Compare
After realising that I was actually documenting the Android behaviour of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just have some minor phrasing and punctuation nits. The text is certainly a lot easier to read now.
e4ca285
to
9ebeafc
Compare
To be more consistent with mobile platforms this updates the Windows, macOS, Wayland, X11 and Web backends to all emit a Resumed event immediately after the initial `NewEvents(StartCause::Init)` event. The documentation for Suspended and Resumed has also been updated to provide general recommendations for how to handle Suspended and Resumed events in portable applications as well as providing Android and iOS specific details. This consistency makes it possible to write applications that lazily initialize their graphics state when the application resumes without any platform-specific knowledge. Previously, applications that wanted to run on Android and other systems would have to maintain two, mutually-exclusive, initialization paths. Note: This patch does nothing to guarantee that Suspended events will be delivered. It's still reasonable to say that most OSs without a formal lifecycle for applications will simply never "suspend" your application. There are currently no known portability issues caused by not delivering `Suspended` events consistently and technically it's not possible to guarantee the delivery of `Suspended` events if the OS doesn't define an application lifecycle. (app can always be terminated without any kind of clean up notification on most non-mobile OSs) Resolves: rust-windowing#2185 Co-authored-by: Marijn Suijten <[email protected]> Co-authored-by: Markus Røyset <[email protected]>
9ebeafc
to
1b95746
Compare
Conceptually the Winit `Resumed` event signifies that the application is ready to run and render and since rust-windowing/winit#2331 all platforms now consistently emit a Resumed event that can be used to initialize graphics state for an application. On Android in particular it's important to wait until the application has Resumed before initializing graphics state since it won't have an associated SurfaceView while paused. Without this change then Android applications are likely to just show a black screen. This updates the Wgpu+Winit and Glow+Winit integration for eframe but it's worth noting that the Glow integration is still not able to fully support Android due to limitations in with Glutin's API that mean we can't destroy and create a Window without also destroying the GL context, and therefore the entire application. There is a plan (and progress on) to improve the Glutin API here: rust-windowing/glutin#1435 and with that change it should be an incremental change to enable Android support with Glow later. In the mean time the Glow changes keep the implementation consistent with the wgpu integration and it should now at least be possible to start an Android application - even though it won't be able to suspend correctly.
Conceptually the Winit `Resumed` event signifies that the application is ready to run and render and since rust-windowing/winit#2331 all platforms now consistently emit a Resumed event that can be used to initialize graphics state for an application. On Android in particular it's important to wait until the application has Resumed before initializing graphics state since it won't have an associated SurfaceView while paused. Without this change then Android applications are likely to just show a black screen. This updates the Wgpu+Winit and Glow+Winit integration for eframe but it's worth noting that the Glow integration is still not able to fully support Android due to limitations in with Glutin's API that mean we can't destroy and create a Window without also destroying the GL context, and therefore the entire application. There is a plan (and progress on) to improve the Glutin API here: rust-windowing/glutin#1435 and with that change it should be an incremental change to enable Android support with Glow later. In the mean time the Glow changes keep the implementation consistent with the wgpu integration and it should now at least be possible to start an Android application - even though it won't be able to suspend correctly.
Conceptually the Winit `Resumed` event signifies that the application is ready to run and render and since rust-windowing/winit#2331 all platforms now consistently emit a Resumed event that can be used to initialize graphics state for an application. On Android in particular it's important to wait until the application has Resumed before initializing graphics state since it won't have an associated SurfaceView while paused. Without this change then Android applications are likely to just show a black screen. This updates the Wgpu+Winit and Glow+Winit integration for eframe but it's worth noting that the Glow integration is still not able to fully support Android due to limitations in with Glutin's API that mean we can't destroy and create a Window without also destroying the GL context, and therefore the entire application. There is a plan (and progress on) to improve the Glutin API here: rust-windowing/glutin#1435 and with that change it should be an incremental change to enable Android support with Glow later. In the mean time the Glow changes keep the implementation consistent with the wgpu integration and it should now at least be possible to start an Android application - even though it won't be able to suspend correctly.
Conceptually the Winit `Resumed` event signifies that the application is ready to run and render and since rust-windowing/winit#2331 all platforms now consistently emit a Resumed event that can be used to initialize graphics state for an application. On Android in particular it's important to wait until the application has Resumed before initializing graphics state since it won't have an associated SurfaceView while paused. Without this change then Android applications are likely to just show a black screen. This updates the Wgpu+Winit and Glow+Winit integration for eframe but it's worth noting that the Glow integration is still not able to fully support Android due to limitations in with Glutin's API that mean we can't destroy and create a Window without also destroying the GL context, and therefore the entire application. There is a plan (and progress on) to improve the Glutin API here: rust-windowing/glutin#1435 and with that change it should be an incremental change to enable Android support with Glow later. In the mean time the Glow changes keep the implementation consistent with the wgpu integration and it should now at least be possible to start an Android application - even though it won't be able to suspend correctly.
Conceptually the Winit `Resumed` event signifies that the application is ready to run and render and since rust-windowing/winit#2331 all platforms now consistently emit a Resumed event that can be used to initialize graphics state for an application. On Android in particular it's important to wait until the application has Resumed before initializing graphics state since it won't have an associated SurfaceView while paused. Without this change then Android applications are likely to just show a black screen. This updates the Wgpu+Winit and Glow+Winit integration for eframe but it's worth noting that the Glow integration is still not able to fully support suspend and resume on Android due to limitations with Glutin's API that mean we can't destroy and create a Window without also destroying the GL context, and therefore (practically) the entire application. There is a plan (and progress on) to improve the Glutin API here: rust-windowing/glutin#1435 and with that change it should be an incremental change to enable Android suspend/resume support with Glow later. In the mean time the Glow changes keep the implementation consistent with the wgpu integration and it should now at least be possible to start an Android application - even though it won't be able to suspend correctly.
Conceptually the Winit `Resumed` event signifies that the application is ready to run and render and since rust-windowing/winit#2331 all platforms now consistently emit a Resumed event that can be used to initialize graphics state for an application. On Android in particular it's important to wait until the application has Resumed before initializing graphics state since it won't have an associated SurfaceView while paused. Without this change then Android applications are likely to just show a black screen. This updates the Wgpu+Winit and Glow+Winit integration for eframe but it's worth noting that the Glow integration is still not able to fully support suspend and resume on Android due to limitations with Glutin's API that mean we can't destroy and create a Window without also destroying the GL context, and therefore (practically) the entire application. There is a plan (and progress on) to improve the Glutin API here: rust-windowing/glutin#1435 and with that change it should be an incremental change to enable Android suspend/resume support with Glow later. In the mean time the Glow changes keep the implementation consistent with the wgpu integration and it should now at least be possible to start an Android application - even though it won't be able to suspend correctly.
Conceptually the Winit `Resumed` event signifies that the application is ready to run and render and since rust-windowing/winit#2331 all platforms now consistently emit a Resumed event that can be used to initialize graphics state for an application. On Android in particular it's important to wait until the application has Resumed before initializing graphics state since it won't have an associated SurfaceView while paused. Without this change then Android applications are likely to just show a black screen. This updates the Wgpu+Winit and Glow+Winit integration for eframe but it's worth noting that the Glow integration is still not able to fully support suspend and resume on Android due to limitations with Glutin's API that mean we can't destroy and create a Window without also destroying the GL context, and therefore (practically) the entire application. There is a plan (and progress on) to improve the Glutin API here: rust-windowing/glutin#1435 and with that change it should be an incremental change to enable Android suspend/resume support with Glow later. In the mean time the Glow changes keep the implementation consistent with the wgpu integration and it should now at least be possible to start an Android application - even though it won't be able to suspend correctly.
Conceptually the Winit `Resumed` event signifies that the application is ready to run and render and since rust-windowing/winit#2331 all platforms now consistently emit a Resumed event that can be used to initialize graphics state for an application. On Android in particular it's important to wait until the application has Resumed before initializing graphics state since it won't have an associated SurfaceView while paused. Without this change then Android applications are likely to just show a black screen. This updates the Wgpu+Winit and Glow+Winit integration for eframe but it's worth noting that the Glow integration is still not able to fully support suspend and resume on Android due to limitations with Glutin's API that mean we can't destroy and create a Window without also destroying the GL context, and therefore (practically) the entire application. There is a plan (and progress on) to improve the Glutin API here: rust-windowing/glutin#1435 and with that change it should be an incremental change to enable Android suspend/resume support with Glow later. In the mean time the Glow changes keep the implementation consistent with the wgpu integration and it should now at least be possible to start an Android application - even though it won't be able to suspend correctly. Fixes emilk#1951
Conceptually the Winit `Resumed` event signifies that the application is ready to run and render and since rust-windowing/winit#2331 all platforms now consistently emit a Resumed event that can be used to initialize graphics state for an application. On Android in particular it's important to wait until the application has Resumed before initializing graphics state since it won't have an associated SurfaceView while paused. Without this change then Android applications are likely to just show a black screen. This updates the Wgpu+Winit and Glow+Winit integration for eframe but it's worth noting that the Glow integration is still not able to fully support suspend and resume on Android due to limitations with Glutin's API that mean we can't destroy and create a Window without also destroying the GL context, and therefore (practically) the entire application. There is a plan (and progress on) to improve the Glutin API here: rust-windowing/glutin#1435 and with that change it should be an incremental change to enable Android suspend/resume support with Glow later. In the mean time the Glow changes keep the implementation consistent with the wgpu integration and it should now at least be possible to start an Android application - even though it won't be able to suspend correctly. Fixes emilk#1951
Conceptually the Winit `Resumed` event signifies that the application is ready to run and render and since rust-windowing/winit#2331 all platforms now consistently emit a Resumed event that can be used to initialize graphics state for an application. On Android in particular it's important to wait until the application has Resumed before initializing graphics state since it won't have an associated SurfaceView while paused. Without this change then Android applications are likely to just show a black screen. This updates the Wgpu+Winit and Glow+Winit integration for eframe but it's worth noting that the Glow integration is still not able to fully support suspend and resume on Android due to limitations with Glutin's API that mean we can't destroy and create a Window without also destroying the GL context, and therefore (practically) the entire application. There is a plan (and progress on) to improve the Glutin API here: rust-windowing/glutin#1435 and with that change it should be an incremental change to enable Android suspend/resume support with Glow later. In the mean time the Glow changes keep the implementation consistent with the wgpu integration and it should now at least be possible to start an Android application - even though it won't be able to suspend correctly. Fixes emilk#1951
* eframe: allow hooking into EventLoop building This enables native applications to add an `event_loop_builder` callback to the `NativeOptions` struct that lets them modify the Winit `EventLoopBuilder` before the final `EventLoop` is built and run. This makes it practical for applications to change platform specific config options that Egui doesn't need to be directly aware of. For example the `android-activity` glue crate that supports writing Android applications in Rust requires that the Winit event loop be passed a reference to the `AndroidApp` that is given to the `android_main` entrypoint for the application. Since the `AndroidApp` itself is abstracted by Winit then there's no real need for Egui/EFrame to have a dependency on the `android-activity` crate just for the sake of associating this state with the event loop. Addresses: #1951 * eframe: defer graphics state initialization until app Resumed Conceptually the Winit `Resumed` event signifies that the application is ready to run and render and since rust-windowing/winit#2331 all platforms now consistently emit a Resumed event that can be used to initialize graphics state for an application. On Android in particular it's important to wait until the application has Resumed before initializing graphics state since it won't have an associated SurfaceView while paused. Without this change then Android applications are likely to just show a black screen. This updates the Wgpu+Winit and Glow+Winit integration for eframe but it's worth noting that the Glow integration is still not able to fully support suspend and resume on Android due to limitations with Glutin's API that mean we can't destroy and create a Window without also destroying the GL context, and therefore (practically) the entire application. There is a plan (and progress on) to improve the Glutin API here: rust-windowing/glutin#1435 and with that change it should be an incremental change to enable Android suspend/resume support with Glow later. In the mean time the Glow changes keep the implementation consistent with the wgpu integration and it should now at least be possible to start an Android application - even though it won't be able to suspend correctly. Fixes #1951
To be more consistent with mobile platforms this updates the Windows,
macOS, Wayland, X11 and Web backends to all emit a Resumed event
immediately after the initial
NewEvents(StartCause::Init)
event.The documentation for the Resumed event has also been updated to
recommend that applications should wait until they receive the Resumed
event before creating any graphics context and their first window to
help ensure portability.
This consistency makes it possible to write applications that lazily
initialize their graphics state when the application resumes without
any platform-specific knowledge. Previously, applications that wanted
to run on Android needed to have
target_os="android"
specific codeto initialize within the
Resumed
event because that same logicwould never run on non-mobile platforms.
Relates to:
#2111 - Is it possible to automatically port over a GUI application from Linux/Windows to Android that was written in Rust?
#2185 - Documentation on
Event::{Suspended, Resumed}
is lacking#2144 - Add Foreground and Background events for iOS
#2293 - Evolution of the Android backend
CHANGELOG.md
if knowledge of this change could be valuable to userstarget_os="android"
handling which could be removed: https://github.com/rib/bluey/blob/7428d9512ecfff22f2e437820f02585bc6ccf1ba/bluey-ui/src/lib.rs#L115Fixes: #2185