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 the ability to request a redraw from an external source #12197

Merged
merged 5 commits into from
Mar 4, 2024

Conversation

R081n
Copy link
Contributor

@R081n R081n commented Feb 29, 2024

Hi, this is a minimal implementation of #12159. I wasn't sure if the EventLoopProxy should be wrapped somewhat to make it more explicit.

Objective

Minimal implementation of #12159
When using UpdateMode::Reactive it is currently not possible to request a redraw when a long running task is finished or an external source has new data.

This makes the following possible which will then run an app update once

// EventLoopProxy is Send on most architectures
// The EventLoopProxy can also be saved in a thread local for WASM or a static in other architecturecs
pub fn example(proxy: NonSend<EventLoopProxy<()>>) {
    let clone: EventLoopProxy<()> = proxy.clone();
    thread::spawn(move || {
        // do long work
        clone.send_event(());
    });
}

Solution

By using the EventLoopProxy one can manually send events from external threads to the event loop as UserEvents.
This simply sets redraw_requested when a UserEvent is received.

Changelog

  • Added the ability to request a redraw from an external source

pub use the event loop proxy
Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@alice-i-cecile alice-i-cecile added A-Windowing Platform-agnostic interface layer to run your app in C-Feature A new feature, making something new possible labels Feb 29, 2024
@@ -690,6 +692,9 @@ fn handle_winit_event(
event_loop.set_control_flow(ControlFlow::Wait);
}
}
Event::UserEvent(_) => {
Copy link
Member

Choose a reason for hiding this comment

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

Could you expose a specific type that should be sent to trigger a redraw rather than match on anything?
Reusing https://docs.rs/bevy/latest/bevy/window/struct.RequestRedraw.html would be nice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I replaced the type with RequestRedraw. Also the reexport is now a type alias which prevents someone from using the wrong generic parameter when requesting the resource

@@ -49,6 +49,8 @@ use winit::{
event_loop::{ControlFlow, EventLoop, EventLoopBuilder, EventLoopWindowTarget},
};

pub use winit::event_loop::EventLoopProxy;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: it'd be nice to have some documentation pointing users to this pattern somewhere. Maybe on WinitSettings::desktop_app? Or maybe there's a better place for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi, i added some documentation. I also added it to the bullet points of Reactive and ReactiveLowPower

@aevyrie
Copy link
Member

aevyrie commented Mar 3, 2024

This is already possible today by importing EventLoopProxy from winit, if we add this, I think we need to include some docs and helpers. At work, we stuff it in a static so we can access EVENT_LOOP_PROXY anywhere we need it.

Worth noting bevy used to have something similar, so we should probably understand why it was removed before re-adding it: #674

@R081n
Copy link
Contributor Author

R081n commented Mar 3, 2024

This is already possible today by importing EventLoopProxy from winit, if we add this, I think we need to include some docs and helpers. At work, we stuff it in a static so we can access EVENT_LOOP_PROXY anywhere we need it.

@aevyrie Does this still work in 0.13? We used the same in version 0.12 but it was seemingly patched out. This pr is mostly to make the behavior explicit and prevent someone from using the wrong version of the event loop proxy.

@aevyrie
Copy link
Member

aevyrie commented Mar 3, 2024

Nope, haven't updated to bevy 0.13 yet. Not sure what you mean by patched out - it's being used in this PR.

This pr is mostly to make the behavior explicit and prevent someone from using the wrong version of the event loop proxy.

Yeah, that's a great point. Not needing to depend on winit manually, which can desync during updates, would be nice.

@R081n
Copy link
Contributor Author

R081n commented Mar 3, 2024

@aevyrie the winit runner received some fixes to reduce sporadic updates. This also lead to the user event having no longer any effect, because the AboutToWait now checks some flags before updating

@aevyrie
Copy link
Member

aevyrie commented Mar 3, 2024

Ah good catch. in that case, this PR is also a bugfix for a regression. 😆

@alice-i-cecile IMO this should be be on the 0.13.1 milestone, if the event loop proxy no longer works, that's a pretty serious regression.

@R081n
Copy link
Contributor Author

R081n commented Mar 3, 2024

@aevyrie yeah, i wasn't sure when making it. I did not know if the behavior was intentional ( also a case for better docs 😅)

@alice-i-cecile alice-i-cecile added this to the 0.13.1 milestone Mar 3, 2024
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior P-Regression Functionality that used to work but no longer does. Add a test for this! S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed C-Feature A new feature, making something new possible labels Mar 3, 2024
Copy link
Member

@mockersf mockersf left a comment

Choose a reason for hiding this comment

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

Please make the type of user event explicit instead of "anything". that way it will be a breaking change later if we add support for more events

@mockersf mockersf removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 4, 2024
@R081n R081n requested a review from mockersf March 4, 2024 09:04
@mockersf mockersf added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 4, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 4, 2024
Merged via the queue into bevyengine:main with commit d90cb57 Mar 4, 2024
26 checks passed
mockersf pushed a commit that referenced this pull request Mar 5, 2024
Hi, this is a minimal implementation of #12159. I wasn't sure if the
`EventLoopProxy` should be wrapped somewhat to make it more explicit.

Minimal implementation of #12159
When using `UpdateMode::Reactive` it is currently not possible to
request a redraw when a long running task is finished or an external
source has new data.

This makes the following possible which will then run an app update once

``` rust
// EventLoopProxy is Send on most architectures
// The EventLoopProxy can also be saved in a thread local for WASM or a static in other architecturecs
pub fn example(proxy: NonSend<EventLoopProxy<()>>) {
    let clone: EventLoopProxy<()> = proxy.clone();
    thread::spawn(move || {
        // do long work
        clone.send_event(());
    });
}
```

By using the EventLoopProxy one can manually send events from external
threads to the event loop as `UserEvent`s.
This simply sets redraw_requested when a `UserEvent` is received.

- Added the ability to request a redraw from an external source

---------

Co-authored-by: Kellner, Robin <Robin.Kellner@vector.com>
spectria-limina pushed a commit to spectria-limina/bevy that referenced this pull request Mar 9, 2024
…ne#12197)

Hi, this is a minimal implementation of bevyengine#12159. I wasn't sure if the
`EventLoopProxy` should be wrapped somewhat to make it more explicit.

# Objective

Minimal implementation of bevyengine#12159
When using `UpdateMode::Reactive` it is currently not possible to
request a redraw when a long running task is finished or an external
source has new data.

This makes the following possible which will then run an app update once

``` rust
// EventLoopProxy is Send on most architectures
// The EventLoopProxy can also be saved in a thread local for WASM or a static in other architecturecs
pub fn example(proxy: NonSend<EventLoopProxy<()>>) {
    let clone: EventLoopProxy<()> = proxy.clone();
    thread::spawn(move || {
        // do long work
        clone.send_event(());
    });
}
```

## Solution

By using the EventLoopProxy one can manually send events from external
threads to the event loop as `UserEvent`s.
This simply sets redraw_requested when a `UserEvent` is received.

## Changelog

- Added the ability to request a redraw from an external source

---------

Co-authored-by: Kellner, Robin <Robin.Kellner@vector.com>
mtsr pushed a commit to mtsr/bevy that referenced this pull request Mar 15, 2024
…ne#12197)

Hi, this is a minimal implementation of bevyengine#12159. I wasn't sure if the
`EventLoopProxy` should be wrapped somewhat to make it more explicit.

# Objective

Minimal implementation of bevyengine#12159
When using `UpdateMode::Reactive` it is currently not possible to
request a redraw when a long running task is finished or an external
source has new data.

This makes the following possible which will then run an app update once

``` rust
// EventLoopProxy is Send on most architectures
// The EventLoopProxy can also be saved in a thread local for WASM or a static in other architecturecs
pub fn example(proxy: NonSend<EventLoopProxy<()>>) {
    let clone: EventLoopProxy<()> = proxy.clone();
    thread::spawn(move || {
        // do long work
        clone.send_event(());
    });
}
```

## Solution

By using the EventLoopProxy one can manually send events from external
threads to the event loop as `UserEvent`s.
This simply sets redraw_requested when a `UserEvent` is received.

## Changelog

- Added the ability to request a redraw from an external source

---------

Co-authored-by: Kellner, Robin <Robin.Kellner@vector.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Windowing Platform-agnostic interface layer to run your app in C-Bug An unexpected or incorrect behavior P-Regression Functionality that used to work but no longer does. Add a test for this! S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants