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

Add initialization closure and drop on exit #3895

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

madsmtm
Copy link
Member

@madsmtm madsmtm commented Sep 1, 2024

Summary

Allow windows to be created before the user's application state, avoiding Option around these in user code.

This is paired with using the Drop impl of ApplicationHandler instead of exited to communicate that the event loop is done (composes much better).

See this example for how things now look:

struct App {
    window: Box<dyn Window>,
    surface: Option<wgpu::Surface<...>>,
    in_flight_network_requests: Vec<...>,
}

impl Drop for App {
    fn drop(&mut self) {
        self.in_flight_network_requests.cancel();
        // And other such cleanup logic previously done in `exited`
    }
}

impl ApplicationHandler for App {
    fn can_create_surfaces(&mut self, _event_loop: &dyn ActiveEventLoop) {
        self.surface = Some(...);
    }

    // ...
}

fn main() -> Result<()> {
    let event_loop = EventLoop::new()?;
    event_loop.run(|event_loop| {
        // We have access to `&dyn ActiveEventLoop` here, and can initialize windows.
        // We _could_ potentially also initialize the surface here, if we didn't want to support Android.
        let window = event_loop.create_window(...).unwrap();
        // Users are expected to return their now initialized `ApplicationHandler` here.
        App { window, surface: None, network_connections: Vec::new() }
    })?;
    Ok(())
}

Motivation

One of the most common problems that users encounter in v0.30 is that their window (and surface) needs to be wrapped in an Option, because they can no longer initialize windows at the start of main, and the method they're expected to initialize it in (resumed in v0.30, can_create_surfaces on master) isn't called before the application state is already created.

See #3447 for the original change, it tackles many real bugs, such as:

  • The window being in a weird partially initialized state when created before EventLoop::run on macOS and iOS.
  • create_window being called in a non-sync fashion on Wayland.
  • Surfaces can be destroyed and must be recreated on Android.

We were aware that the solution was overly restrictive (as also noted in #3626 and the changelog), and provided the deprecated EventLoop::create_window to ease the transition. This API has since been removed in #3826 to prepare for v0.30, but users are still left with no real solution to the ergonomic issue!

Furthermore, the current design is also pushing the user towards re-creating Window, but that is incorrect, it really is only the surface that needs to be re-created on Android - @MarijnS95 stated it well:

Anything that moves Window lifetime closer to Surface lifetime is a step backwards.

Proposed solution

We add a closure to the initial EventLoop::run call (keeping EventLoop::run_app as deprecated fallback), which allows access to ActiveEventLoop before creating the application. On most platforms, this closure will simply be called before the event loop starts, but on macOS/iOS, it will be called at key initialization steps in the application's lifecycle, namely NSApplicationDidFinishLaunchingNotification/UIApplicationDidFinishLaunchingNotification.

This will require some boxing to be made object-safe, see this playground, but that's the cost of going the trait route (which we've discussed at length, so I won't belabour the point ;) ).

This PR was my original motivation for doing #3721, and resolves my primary concerns from #2903.

Prior art

The SDL uses basically the old poll_events API that we moved away from a long time ago. To tackle iOS, it spawns a new thread and handles the user's call back in that thread (which is questionably thread safe). On macOS, to allow e.g. handling events while resizing, it provides the extra SDL_SetEventFilter which handles events immediately in a callback.

SDL3 provides the same API for backwards compat, but also allows a new design with SDL_AppInit/SDL_AppQuit, see this documentation. This is literally just a poor mans static/global closure, i.e. effectively the same as what we're doing in this PR.

Possible future work

Make using can_create_surfaces and surface_destroyed easier to use correctly.

  • One solution would be to transition the ApplicationHandler between different type-states, similar to that described in Encode event lifecycle in the type-system #2903.
  • Crazy idea for this would be to use FnMut instead of FnOnce for the initialization closure. This depends on at what point in the lifecycle the surface creation callbacks are called, which I'm not really sure of.
  • Previous iteration of this PR tried to merge the initialization closure and can_create_surfaces, see the comment history for details. There might still be a space to explore there.

TODO

  • Discuss whether we want to go this direction.
    • I especially want input from @MarijnS95 on whether he thinks this is a step forwards or backwards?
  • Update example to help users understand how to use this correctly.
  • Write proper documentation and update changelog entry.
  • Figure out if this is also desired for EventLoopExtRunOnDemand::run_app_on_demand. I think it is?
  • Figure out if this is also desired for EventLoopExtWeb::spawn_app?
  • Figure out if this is also desired for EventLoopExtPumpEvents::pump_app_events. Should we just ignore the issue there?
  • Do we still want [StartCause::Init]?
  • Implement on all platforms:
    • Android
    • iOS/UIKit
    • macOS/AppKit
    • Orbital
    • Wayland
    • Web
    • Windows
    • X11
  • Test on all platforms (other maintainers, feel free to edit this if you have done so):
    • Android
    • iOS/UIKit
    • macOS/AppKit
    • Orbital
    • Wayland
    • Web
    • Windows
    • X11

@madsmtm madsmtm added DS - android S - api Design and usability C - needs discussion Direction must be ironed out C - nominated Nominated for discussion in the next meeting labels Sep 1, 2024
@madsmtm madsmtm added this to the Version 0.31.0 milestone Sep 1, 2024
@madsmtm madsmtm force-pushed the madsmtm/init-closure branch from 9827aff to d713d52 Compare September 1, 2024 12:39
@madsmtm madsmtm removed the request for review from jackpot51 September 1, 2024 12:40
@madsmtm madsmtm force-pushed the madsmtm/init-closure branch 3 times, most recently from f6a520e to 478b4df Compare September 1, 2024 13:23
kchibisov

This comment was marked as resolved.

@MarijnS95

This comment was marked as resolved.

@madsmtm

This comment was marked as outdated.

@MarijnS95

This comment was marked as resolved.

@mickvangelderen

This comment was marked as off-topic.

@madsmtm madsmtm force-pushed the madsmtm/init-closure branch from 478b4df to 1d1d9b2 Compare December 3, 2024 11:18
@madsmtm madsmtm changed the title Allow specifying an initialization closure in EventLoop::run Add initialization closure and drop on exit Dec 3, 2024
@madsmtm
Copy link
Member Author

madsmtm commented Dec 3, 2024

Thanks for the input @MarijnS95! You're right that I was mistaken in my original solution, and I've changed the approach in the PR (only add initialization closure, don't touch can_create_surfaces), see the updated PR description. I've hidden the now outdated comments for clarity.
The same goes for @kchibisov's comments, since I felt that I've resolved them with #3895 (comment), and because I've re-done the PR, but feel free to re-open / continue the discussion of it.

@mickvangelderen: I've linked to your comment in #2903 (comment), but I think it's out of scope for this PR, so I've hidden the comment.

@madsmtm madsmtm requested a review from kchibisov December 3, 2024 11:51
@kchibisov
Copy link
Member

In general I'd prefer exploring type state patterns during runtime then just having a closure, since it won't really solve e.g. android stuff or anything that has an option to remove windows and keep running.

@madsmtm madsmtm force-pushed the madsmtm/init-closure branch from 1d1d9b2 to fbc6fdc Compare December 3, 2024 12:28
@madsmtm
Copy link
Member Author

madsmtm commented Dec 3, 2024

In general I'd prefer exploring type state patterns during runtime then just having a closure

What do you mean by this? Something like #3710?

it won't really solve e.g. android stuff or anything that has an option to remove windows and keep running.

Indeed, this won't solve Android stuff. And for a lot of "real world" applications, you'll want multiple windows, and then the point is a bit moot anyways, true.

But a lot of uses of Winit are not full-fledged applications, and there it does make sense to allow more easily creating windows.

@kchibisov
Copy link
Member

impl ApplicationHandler for Inactive {}
impl ApplicationHandler for Active {}

and a way to switch between them during a runtime with something on the event loop.

@madsmtm
Copy link
Member Author

madsmtm commented Dec 3, 2024

impl ApplicationHandler for Inactive {}
impl ApplicationHandler for Active {}

and a way to switch between them during a runtime with something on the event loop.

Interesting, could you give more details on how this would work? How do you imagine you would switch between these? And how does it differ from the ideas in #2903?

In any case, I feel like that perhaps relates more to the Android surface stuff, not so much this specific PR?

@kchibisov
Copy link
Member

I'd assume that you store all of them in the big struct and then borrow a field, then you'd have a way to get back to your big wrapper. The problem is that it's self referencial, so requires a bit of unsafe.

struct Foo {
     state1: Option<State1>,
     state2: Option<State2>,
}

EventLoop {
     active: &mut dyn AppHandler, // either state1 or state2.
     original_state: Foo // You just store it for reference.
}

Comment on lines 87 to +90
///
/// With OpenGL it could be EGLDisplay.
#[cfg(not(android_platform))]
context: Option<Context<DisplayHandle<'static>>>,
context: Context<DisplayHandle<'static>>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this is a softbuffer context (which shouldn't be using OpenGL at all, so the doc-comment is weird unless it was using glutin).

I really wish we could make progress on rust-windowing/softbuffer#130, so that this weird cfg can be removed.

Comment on lines -289 to +294
/// Applications that need to run on Android should assume their [`NativeWindow`] 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`]).
/// Applications that need to run on Android must be able to handle their underlying
/// [`SurfaceView`] being destroyed, which in turn indirectly invalidates any existing
/// render surfaces that may have been created outside of Winit (such as an `EGLSurface`,
/// [`VkSurfaceKHR`] or [`wgpu::Surface`]).
///
/// When receiving [`destroy_surfaces()`] Android applications should drop all render surfaces
/// before the event callback completes, which may be re-created when the application next
/// receives [`can_create_surfaces()`].
/// This means that in this method, you must drop all render surfaces before the event callback
/// completes, and only re-create them in or after [`can_create_surfaces()`] is next recieved.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks to be accidentally reverting the documentation fixes from #3786, which were initially discovered in #3505. Perhaps this was due to a conflict-resolve?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, you're right!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C - needs discussion Direction must be ironed out C - nominated Nominated for discussion in the next meeting DS - android S - api Design and usability
Development

Successfully merging this pull request may close these issues.

4 participants