Allow completed tasks to wake the winit event loop.

[andriyDev]

Objective

• Fixes bevy_assets should send wake events to bevy_window when assets finish loading (async tasks and desktop_app interactions) #20873.
• Fixes Asset loading doesn't update correctly when using WinitSettings::desktop_app() #5844.

Solution

• Allow setting a "kicker" on a task pool.
• Implement a future wrapper KickOnWake, which wraps the future's waker to execute the kicker, followed by waking the waker.
• Make all the spawn methods on a task pool wrap the spawned future in a KickOnWake before passing it to async-tasks.

This strategy might have some performance implications - we now need to read a RwLock for each task spawned. We also allocate an Arc each time a future is polled (to wrap the wakers). Lastly, we kick any time a future is ready to make progress, or resolved.

Testing

• The desktop_request_redraw example now renders the cube as soon as it finishes loading - no input required!
  • Previously, if you didn't move your mouse or otherwise interact with the window, the window would stay on a black screen, and the cube would never load. Now it does!
  • In order to see this, the cube needs to be modified to disable the animation by default (this will prevent the game from updating to start).

[andriyDev]

Looks like using alloc::task::Wake isn't allowed... I guess I could fork the impl of Waker::from(impl Wake) using our bevy_platform::sync::Arc instead, which should be ok.

[NthTensor]

I've not done a full review but my gut feeling is that this is not the right approach to a fix. I have some pretty strong reservations about this.

I will try to respond with more specifics soon.

[NthTensor]

This is the crux of the issue for me. Waking the event loop whenever a task resolves is absolutly incorrect for many types of tasks.

Why not simply embrace a pattern where, for tasks that do need to wake the event loop when they complete, you bring a EventLoopProxy into the closure?

let proxy = proxy.clone()
spawn(move async {
    ... //
    proxy.send_event(WinitUserEvent::WakeUp);
})

[andriyDev]

This isn't enough. If a task needs to sleep, when it wakes, that won't kick the event loop, meaning for single threaded apps, they won't make progress until there's more window events.

I guess we could conditionally compile this to only happen on single threaded, but that seems cursed...

[andriyDev]

Also can I get a concrete example of a task you don't want to know completed? Maybe I'm too narrow minded but I don't really know a situation where someone would want a task to not report its status to Bevy - which is effectively what's happening when you don't kick the event loop.

[NthTensor]

It sounds like the root cause is the implementation of single-threaded apps being embedded within the window loop. I'm very skeptical about solutions like this, which to me seem to be addressing the symptoms rather than the cause.

[NthTensor]

@andriyDev how long has this been an issue? Is this a recent regression or has this always been a facet of the event loop?

[andriyDev]

@NthTensor check the linked issues in the description. TL;DR basically forever. Recently there's been talks mentioning this for real stuff https://discord.com/channels/691052431525675048/743663673393938453/1491989166475575407 which is why I think we should just solve it

[NthTensor]

To summarize the conversation from discord, my preferred solution would be to:

1. Separate the ECS and the task executor from the window update loop, so that we don't have to be redrawing frames to resolve tasks.
2. Provide an easy mechanism for triggering reactive updates in bevy_app. Something like the following:

static UPDATE_REQ_FN: AtomicPtr<()> = AtomicPtr::new(ptr::null());

/// Sets the function that is called when a task requests an update.
pub fn handle_update_request(func: fn()) {
    UPDATE_REQ_FN.store(func as *mut ());
}

/// Requests an ECS update. When running in reactive mode on most desktops, this 
/// will cause the ECS to run a simulation tick and draw a new frame.
pub fn request_update() {
    let ptr = UPDATE_REQ_FN.load(Ordering::Relaxed);
    if !ptr.is_null() {
        let func: fn() = unsafe { std::mem::transmute(ptr) };
        func();
    }
}

Windowing backends like bevy_winit would call handle_update_request during setup to set up a hook that triggers a window refresh and repaint. Tasks that need to trigger updates would call request_update() before they resolve.

The only downside to this approach is that some degree of care is required to properly annotate side-effect-causing tasks with calls to request_update().

[NthTensor]

I know this is somewhat counter to what's been discussed before, sorry about flip-flopping on this.

[aevyrie]

It seems undesirable to add bevy_task registration inside bevy_winit. We already use the bevy/winit event loop proxy at work with a global event loop waker, and all of that can happen as a 3rd party thing, nothing upstream linking (or further linking) bevy subcrates together. This is also making bevy tasks "special" in a sense, when it doesn't need to be.

The way we do this is we just take the event loop proxy, and lazily initialize it once bevy has inserted it. You can then just call fsl_waker::wake_up() from anywhere, like tasks, to wake the event loop.

It's also worth asking if we want to make this fully automatic. One of the common issues I see with event loop waking is someone naively adds it in a way that keeps the event loop awake forever, and if you don't build this with observability in mind (e.g. track_caller) it's very hard to diagnose the issue without disabling chunks of code to bisect. Additionally, if you make this fully automatic, you'll make it impossible for users to choose if they actually want to wake up in reactive mode. If you are already opting into reactive mode, I assume you want that kind of control.

I think we need to first make manual event loop waking easy, observable, task agnostic (e.g. from rayon), and windowing agnostic (e.g. not tied to winit) before trying to make it automatic. I could see if we could just upstream the crate we have for this, it's tiny.

We could automatically wake for specific things, e.g. asset load, but I'm a bit wary to do this for all futures on the task pool.