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

Allow specifying an initialization closure in EventLoop::run #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 and surfaces 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: 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 main() -> Result<()> {
    let event_loop = EventLoop::new()?;
    event_loop.run(|event_loop| {
        // We have access to `&dyn ActiveEventLoop` here, and can initialize windows.
        let window = event_loop.create_window(...).unwrap();
        let surface = ...;
        // Users are expected to return their now initialized `ApplicationHandler` here.
        App { window, surface, 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 issues.

Implementation details

We haven't really discussed this officially, but I think it's fair to say that Winit's mobile platforms are somewhat second-class citizens (and we could consider stating so in #3887). That is not to say that our support for them cannot be improved, #3833 is a great example of this, but I think that we should remember that our user-base is by far most concerned with desktop (and to some extent web).

With that in mind, I believe that the solution we've chosen for surface recreation is complicating our API, as it is a problem that purely exists on Android (also discussed in #3765). Instead, I believe that we should strive to make surface creation easy, and surface recreation possible. Furthermore, the current solution is probably also incorrect in that we're pushing users towards re-creating Window itself, while it really is only the surface that needs to be (from my understanding, unsure?).

My proposed solution to this is to instead 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 [NS|UI]ApplicationDidFinishLaunchingNotification. On Android, we do roughly the same thing, except that we delay initialization even further, all the way up until InitWindow/onNativeWindowCreated/surfaceCreated is called, and then emit the lifecycle events that have come before it. I am not yet sure about web, but I think it's mostly the same there.

To be clear, this changes what the user should do on Android from:
Initialize inside can_create_surfaces, destroy the surface in destroy_surfaces, recreate it in can_create_surfaces.
To:
Initialize inside the main closure, destroy the surface in destroy_surfaces, recreate it in recreate_surfaces.
I've implemented this in the window example, should be fairly self-explanatory.

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 surface_destroyed and recreate_surfaces easier to use correctly.

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?
  • Implement on all platforms:
  • 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 removed the request for review from jackpot51 September 1, 2024 12:40
@madsmtm madsmtm force-pushed the madsmtm/init-closure branch 2 times, most recently from 3c05c28 to f6a520e Compare September 1, 2024 12:44
Copy link
Member

@kchibisov kchibisov left a comment

Choose a reason for hiding this comment

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

I'm not sure how it could work given that the window creation is generally async and the intent was to move to such an async window creation.

It's also not yet decided where we store the window at the end of the day, and if it'll be in winit it'll be redundant again.

One option we could have is to allow swapping the application states on the fly, but that's all I can think of at the given time.

As for surface, it's generally won't always work when you use just Surface without Option since you may fail to create a surface, or surface could be temporally destroyed.

@MarijnS95
Copy link
Member

I especially want input from @MarijnS95 on whether he thinks this is a step forwards or backwards?

Specifically replying to:

Furthermore, the current solution is probably also incorrect in that we're pushing users towards re-creating Window itself, while it really is only the surface that needs to be (from my understanding, unsure?).

Anything that moves Window lifetime closer to Surface lifetime is a step backwards. On Android, we definitely have an analogy of Windows (Activitys) which persist much longer than the Surface (buffer producer) backing their visual output.

Note that the Android system creates these Activitys "asynchronously" for you somewhat like Wayland (except that it doesn't need to be an async reply to a user request, but is in most cases - such as initial startup - from another component in the system).

@madsmtm
Copy link
Member Author

madsmtm commented Sep 1, 2024

Thanks for the responses, in general, I accept that I may be wrong about when to do surface initialization, I'll change the PR to accommodate once I've thought a bit about it, but I do believe that this closure is the correct time to do window initialization.


I'm not sure how it could work given that the window creation is generally async and the intent was to move to such an async window creation.

Well, perhaps we will in the future tell the user to initialize their surfaces in WindowEvent::Created, or with async or something, I don't want to get derailed with that here, but the user still has to trigger the window creation itself, and that still has to be done inside the event loop.

It's also not yet decided where we store the window at the end of the day, and if it'll be in winit it'll be redundant again.

Regardless of where we store the window, the user is still going to want some extra state associated with that window, and that state still has to be initialized. The closure I propose provides a place to do that in a type-state safe manner.

One option we could have is to allow swapping the application states on the fly, but that's all I can think of at the given time.

Not sure what you mean here, the user would implement ApplicationHandler for several newtypes, and we'd somehow switch between them?

As for surface, it's generally won't always work when you use just Surface without Option since you may fail to create a surface, or surface could be temporally destroyed.

Sure, and that's also what the window example now showcases (which it didn't before).

Anything that moves Window lifetime closer to Surface lifetime is a step backwards. On Android, we definitely have an analogy of Windows (Activitys) which persist much longer than the Surface (buffer producer) backing their visual output.

Hmm, okay, I was really hoping we could do window and surface initialization in one step, it's just much easier to teach. Did you consider the approach I outlined (not initializing the application before the window is ready, i.e. skipping the lifecycle events that are emitted until the first surface is initialized)? It's kind of a hack, but it would allow the integration for the user to get Android to work to be more of an "add-on" rather than something that every user has to deal with from the onset, even if they don't support Android?

Note that the Android system creates these Activitys "asynchronously" for you somewhat like Wayland (except that it doesn't need to be an async reply to a user request, but is in most cases - such as initial startup - from another component in the system).

But... The programmer would still be in control of which ones are created, right? And would still be in control of when they close?

@MarijnS95
Copy link
Member

Anything that moves Window lifetime closer to Surface lifetime is a step backwards. On Android, we definitely have an analogy of Windows (Activitys) which persist much longer than the Surface (buffer producer) backing their visual output.

Hmm, okay, I was really hoping we could do window and surface initialization in one step, it's just much easier to teach. Did you consider the approach I outlined (not initializing the application before the window is ready, i.e. skipping the lifecycle events that are emitted until the first surface is initialized)? It's kind of a hack, but it would allow the integration for the user to get Android to work to be more of an "add-on" rather than something that every user has to deal with from the onset, even if they don't support Android?

I'm not sure how that could work, unless we push on with an idea where raw-window-handle deals with surface lifetime events internally and pretends there's always an Optional user-initialized Surface held by it. Then we have a 1:1 relation between Window and Surface again? (and this might help with arbitrary "non-`Window subsurfaces" which could also have lifetime callbacks on Android).

Specifically on Android this would hide the fact that the users' Surface disappears (and requires a surface/swapchain recreate on the graphics side) every time a user navigates away from the app, turns off the screen, etc.

Note that the Android system creates these Activitys "asynchronously" for you somewhat like Wayland (except that it doesn't need to be an async reply to a user request, but is in most cases - such as initial startup - from another component in the system).

But... The programmer would still be in control of which ones are created, right? And would still be in control of when they close?

Not sure and yes? Assume a case where I'm browsing my photos gallery, select a photo, and click on "share". If my Rust app declares an <intent-filter> that allows my Activity to start with a "data URL to files with picture MIME types" (or something like that), the Android system will start (another instance of, or in parallel to other Activitys already running within our Rust Application/process) our Rust Activity with the specified data (stored inside an Intent) when I wish to share via the Rust app. And there are numerous other (deep linking) examples where external factors (re)start (or launch "within" an already-running) an Activity.

We could always call .finish() to close it though.

@mickvangelderen
Copy link

mickvangelderen commented Oct 4, 2024

I am playing around with a different approach: to expose traits that together explicitly implement the state machine that winit already suggests you have today.

Here is what an application would look like.

enum Application {}

struct Uninitialized {
    window_attributes: winit::window::WindowAttributes,
}

struct Resumed {
    window_attributes: winit::window::WindowAttributes,
    #[allow(unused)]
    window: winit::window::Window,
}

struct Suspended {
    window_attributes: winit::window::WindowAttributes,
}

struct Exited;

type Error = Box<dyn std::error::Error>;

impl<TUserEvent: 'static> winit_ext::Application<TUserEvent> for Application {
    type Uninitialized = Uninitialized;
    type Resumed = Resumed;
    type Suspended = Suspended;
    type Exited = Exited;
    type Error = Error;
}

impl<TUserEvent: 'static> winit_ext::ApplicationUninitialized<TUserEvent> for Uninitialized {
    type Application = Application;

    fn initialize(self, event_loop: &winit::event_loop::ActiveEventLoop) -> Result<Resumed, Error> {
        let Self { window_attributes } = self;
        let window = event_loop.create_window(window_attributes.clone())?;
        Ok(Resumed {
            window_attributes,
            window,
        })
    }
}

impl<TUserEvent: 'static> winit_ext::ApplicationResumed<TUserEvent> for Resumed {
    type Application = Application;

    fn suspend(self, _event_loop: &winit::event_loop::ActiveEventLoop) -> Result<Suspended, Error> {
        let Self {
            window_attributes, ..
        } = self;
        Ok(Suspended { window_attributes })
    }

    fn exit(self, _event_loop: &winit::event_loop::ActiveEventLoop) -> Result<Exited, Error> {
        Ok(Exited)
    }
}

impl<TUserEvent: 'static> winit_ext::ApplicationSuspended<TUserEvent> for Suspended {
    type Application = Application;

    fn resume(self, event_loop: &winit::event_loop::ActiveEventLoop) -> Result<Resumed, Error> {
        let Self { window_attributes } = self;
        let window = event_loop.create_window(window_attributes.clone())?;
        Ok(Resumed {
            window_attributes,
            window,
        })
    }

    fn exit(self, _event_loop: &winit::event_loop::ActiveEventLoop) -> Result<Exited, Error> {
        Ok(Exited)
    }
}

fn main() {
    let event_loop = winit::event_loop::EventLoop::new().expect("event loop error");
    let Exited = winit_ext::run(
        event_loop,
        Uninitialized {
            window_attributes: winit::window::WindowAttributes::default(),
        },
    )
    .expect("event loop error")
    .expect("application error");
}

This achieves a couple of things:

  • it is now easy to forward errors out of the event loop
  • some transitions that should never occur can be removed
  • resume and suspend transitions are automatically deduplicated
  • the library consumer can take self by value to implement an application specific sub state machine

The downside is that the user is forced to implement multiple traits.

However, I would argue that any applications that goes into production will have to deal with these basic states, unless you do not want to support suspend/resume (android, ...).

The trait definitions are as follows (yes, the handle method needs to be flattened into separate methods but I haven't for brevity):

pub trait Application<TUserEvent: 'static = ()>: Sized {
    type Uninitialized: ApplicationUninitialized<TUserEvent, Application = Self>;
    type Resumed: ApplicationResumed<TUserEvent, Application = Self>;
    type Suspended: ApplicationSuspended<TUserEvent, Application = Self>;
    type Exited;
    type Error;
}

pub trait ApplicationUninitialized<TUserEvent: 'static = ()>: Sized {
    type Application: Application<TUserEvent, Uninitialized = Self>;

    fn initialize(
        self,
        event_loop: &winit::event_loop::ActiveEventLoop,
    ) -> Result<
        <Self::Application as Application<TUserEvent>>::Resumed,
        <Self::Application as Application<TUserEvent>>::Error,
    >;
}

pub trait ApplicationResumed<TUserEvent: 'static = ()>: Sized {
    type Application: Application<TUserEvent, Resumed = Self>;

    fn handle(
        self,
        event_loop: &winit::event_loop::ActiveEventLoop,
        event: EventResumed<TUserEvent>,
    ) -> Result<Self, <Self::Application as Application<TUserEvent>>::Error> {
        if let EventResumed::WindowEvent {
            window_id: _,
            event: winit::event::WindowEvent::CloseRequested,
        } = event
        {
            event_loop.exit()
        }
        Ok(self)
    }
    fn suspend(
        self,
        event_loop: &winit::event_loop::ActiveEventLoop,
    ) -> Result<
        <Self::Application as Application<TUserEvent>>::Suspended,
        <Self::Application as Application<TUserEvent>>::Error,
    >;
    fn exit(
        self,
        event_loop: &winit::event_loop::ActiveEventLoop,
    ) -> Result<
        <Self::Application as Application<TUserEvent>>::Exited,
        <Self::Application as Application<TUserEvent>>::Error,
    >;
}

pub trait ApplicationSuspended<TUserEvent: 'static = ()>: Sized {
    type Application: Application<TUserEvent, Suspended = Self>;

    fn handle(
        self,
        event_loop: &winit::event_loop::ActiveEventLoop,
        event: EventSuspended<TUserEvent>,
    ) -> Result<Self, <Self::Application as Application<TUserEvent>>::Error> {
        let _ = (event_loop, event);
        Ok(self)
    }
    fn resume(
        self,
        event_loop: &winit::event_loop::ActiveEventLoop,
    ) -> Result<
        <Self::Application as Application<TUserEvent>>::Resumed,
        <Self::Application as Application<TUserEvent>>::Error,
    >;
    fn exit(
        self,
        event_loop: &winit::event_loop::ActiveEventLoop,
    ) -> Result<
        <Self::Application as Application<TUserEvent>>::Exited,
        <Self::Application as Application<TUserEvent>>::Error,
    >;
}

I wrote an adapter that converts a type implementing Application<T> to winit::application::ApplicationHandler<T>. winit does not need to change for people to program against this interface. The complete code can be found at mickvangelderen/winit-ext. But if winit is considering modifying the interface anyway, I thought I should chip in.

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