Fix Limiter::Auto not working with bevy 0.17.0#71
Conversation
|
Hi! GitHub Actions did not trigger because this PR is from a fork. Could someone approve the workflow run? |
| #[cfg(not(target_arch = "wasm32"))] | ||
| use bevy_winit::WINIT_WINDOWS; | ||
| #[cfg(not(target_arch = "wasm32"))] | ||
| use std::cell::Cell; |
There was a problem hiding this comment.
Can this be moved into a full path where Cell is used, instead of a new gated import?
| settings: Res<FramepaceSettings>, | ||
| windows: Query<Entity, With<Window>>, | ||
| frame_limit: Res<FrametimeLimit>, | ||
| _token: NonSend<MainThreadToken>, |
There was a problem hiding this comment.
Do we actually need a new type here? Can you just use NonSend<()>?
There was a problem hiding this comment.
Thanks for the feedback. NonSend<()> would work but then we have to insert an empty resource like this: .insert_non_send_resource(()). This is a bit clunky because the empty resource can be removed by another library and the programm would crash. I suggest using Option<NonSend<()>>. This would make the solution trivial. There is no need to insert a resource and it still does the trick.
There was a problem hiding this comment.
FWIW, the Bevy migration docs indicate that accepting a bevy_ecs::system::NonSendMarker input param would be the conventional mechanism to achieve this.
https://bevy.org/learn/migration-guides/0-16-to-0-17/#nonsend-systems
There was a problem hiding this comment.
bevy_ecs::system::NonSendMarker appears to be the official Bevy mechanism. Thanks for pointing that out. I have updated the PR accordingly.
There was a problem hiding this comment.
Thanks! Feel free to poke me on the bevy discord in the future if I am slow to respond.
| settings: Res<FramepaceSettings>, | ||
| windows: Query<Entity, With<Window>>, | ||
| frame_limit: Res<FrametimeLimit>, | ||
| _non_send_marker: NonSendMarker, // This system may call WINIT_WINDOWS.with_borrow indirectly which requires the system runs on the Bevy main thread. |
There was a problem hiding this comment.
can you just use the full path here instead of adding a feature gated import up top?
3 participants
As described in #69, WINIT_WINDOWS.with_borrow must be called from the bevy main thread.