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.

Sign up for GitHub

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

Jump to bottom

mobile and webgpu: trigger redraw request when needed and improve window creation #11245

Merged
merged 7 commits into from
Jan 9, 2024
Merged

mobile and webgpu: trigger redraw request when needed and improve window creation #11245

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
71a45c6
mobile: create window on initial resume
mockersf Jan 7, 2024
b4a872c
don't update on MouseMotion
mockersf Jan 8, 2024
104af29
request update on resume
mockersf Jan 8, 2024
6ecdde6
cleanup
mockersf Jan 8, 2024
ea29004
request update during plugin initialization
mockersf Jan 8, 2024
a14c92c
cleanup
mockersf Jan 8, 2024
8005aaa
request redraw on ABoutToWait instead of after an event
mockersf Jan 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 56 additions & 44 deletions crates/bevy_winit/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ use bevy_window::{PrimaryWindow, RawHandleWrapper};
pub use winit::platform::android::activity as android_activity;

use winit::{
event::{self, DeviceEvent, Event, StartCause, WindowEvent},
event::{self, DeviceEvent, Event, WindowEvent},
event_loop::{ControlFlow, EventLoop, EventLoopBuilder, EventLoopWindowTarget},
};

Expand Down Expand Up @@ -352,6 +352,7 @@ pub fn winit_runner(mut app: App) {
app.finish();
app.cleanup();
}
runner_state.redraw_requested = true;

if let Some(app_exit_events) = app.world.get_resource::<Events<AppExit>>() {
if app_exit_event_reader.read(app_exit_events).last().is_some() {
Expand All @@ -360,45 +361,25 @@ pub fn winit_runner(mut app: App) {
}
}
}
runner_state.redraw_requested = false;

match event {
Event::NewEvents(start_cause) => match start_cause {
StartCause::Init => {
#[cfg(any(target_os = "ios", target_os = "macos"))]
{
let (
commands,
mut windows,
event_writer,
winit_windows,
adapters,
handlers,
accessibility_requested,
) = create_window_system_state.get_mut(&mut app.world);

create_windows(
event_loop,
commands,
windows.iter_mut(),
event_writer,
winit_windows,
adapters,
handlers,
accessibility_requested,
);

create_window_system_state.apply(&mut app.world);
Event::AboutToWait => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Whats the reason for moving Window::request_redraw() into Event::AboutToWait?

I would recommend to call Window::request_redraw() as soon as you know that you want to draw. Calling it in Event::AboutToWait has the disadvantage of queuing that request later then you could, which might let you miss frames.

The previous method of calling Window::request_redraw() at the end of the event loop was therefor better imo.

Copy link
Member Author

Choose a reason for hiding this comment

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

the previous method doesn't work on iOS, because most of Bevy request redraw come from the update which is called when reacting to a RedrawRequested event, and that doesn't work on iOS

Copy link
Contributor

Choose a reason for hiding this comment

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

What exactly doesn't work on iOS?
Are you saying that WindowEvent::RedrawRequested is not being sent on iOS?

Copy link
Member Author

@mockersf mockersf Jan 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yup, WindowEvent::RedrawRequested is not sent if request_redraw() is called while processing WindowEvent::RedrawRequested on iOS.

I also saw this assert in winit code that also makes me believe it should be avoided
https://github.com/rust-windowing/winit/blob/master/src/platform_impl/ios/app_state.rs#L648-L651

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this would be a bug in Winit.
Will report back when I know more!

Copy link
Contributor

Choose a reason for hiding this comment

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

@mockersf would you mind testing https://github.com/daxpedda/winit/tree/ios-fix-request-redraw-from-redraw-requested-v0.29 and checking if this solves the problem?

Copy link
Member Author

Choose a reason for hiding this comment

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

@daxpedda I haven't had time yet to test your branch and Bevy is currently broken for another reason on iOS. I will as soon as possible and update you!

if runner_state.redraw_requested {
let (_, winit_windows, _, _) =
event_writer_system_state.get_mut(&mut app.world);
for window in winit_windows.windows.values() {
window.request_redraw();
}
}
_ => {
if let Some(t) = runner_state.scheduled_update {
let now = Instant::now();
let remaining = t.checked_duration_since(now).unwrap_or(Duration::ZERO);
runner_state.wait_elapsed = remaining.is_zero();
}
runner_state.redraw_requested = false;
}
Event::NewEvents(_) => {
if let Some(t) = runner_state.scheduled_update {
let now = Instant::now();
let remaining = t.checked_duration_since(now).unwrap_or(Duration::ZERO);
runner_state.wait_elapsed = remaining.is_zero();
}
},
}
Event::WindowEvent {
event, window_id, ..
} => {
Expand Down Expand Up @@ -682,7 +663,6 @@ pub fn winit_runner(mut app: App) {
event: DeviceEvent::MouseMotion { delta: (x, y) },
..
} => {
runner_state.redraw_requested = true;
Copy link
Contributor

@daxpedda daxpedda Jan 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I believe this is a good change, I tried to describe the issue in #11235 (comment).

However, as far as I'm aware Bevy currently doesn't call Window::request_redraw() when it actually needs to redraw when using reactive mode. Keep in mind that removing this line will make the current status worse until this is properly addressed.

let (mut event_writers, ..) = event_writer_system_state.get_mut(&mut app.world);
event_writers.mouse_motion.send(MouseMotion {
delta: Vec2::new(x as f32, y as f32),
Expand All @@ -696,6 +676,44 @@ pub fn winit_runner(mut app: App) {
runner_state.active = ActiveState::WillSuspend;
}
Event::Resumed => {
#[cfg(any(target_os = "android", target_os = "ios", target_os = "macos"))]
Copy link
Contributor

@daxpedda daxpedda Jan 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Creating windows outside of Event::Resumed for these targets is indeed incorrect.
So this change is great!

I would recommend you to move window creation of all targets inside Event::Resumed, it would cut down on the cfg guards.
Let me know if there is a reason why this can't be done or isn't desirable!

{
if runner_state.active == ActiveState::NotYetStarted {
let mut create_window_system_state: SystemState<(
Commands,
Query<(Entity, &mut Window)>,
EventWriter<WindowCreated>,
NonSendMut<WinitWindows>,
NonSendMut<AccessKitAdapters>,
ResMut<WinitActionHandlers>,
ResMut<AccessibilityRequested>,
)> = SystemState::from_world(&mut app.world);

let (
commands,
mut windows,
event_writer,
winit_windows,
adapters,
handlers,
accessibility_requested,
) = create_window_system_state.get_mut(&mut app.world);

create_windows(
&event_loop,
commands,
windows.iter_mut(),
event_writer,
winit_windows,
adapters,
handlers,
accessibility_requested,
);

create_window_system_state.apply(&mut app.world);
}
}

let (mut event_writers, ..) = event_writer_system_state.get_mut(&mut app.world);
match runner_state.active {
ActiveState::NotYetStarted => {
Expand All @@ -706,6 +724,7 @@ pub fn winit_runner(mut app: App) {
}
}
runner_state.active = ActiveState::Active;
runner_state.redraw_requested = true;
#[cfg(target_os = "android")]
{
// Get windows that are cached but without raw handles. Those window were already created, but got their
Expand Down Expand Up @@ -748,12 +767,6 @@ pub fn winit_runner(mut app: App) {
}
_ => (),
}
if runner_state.redraw_requested {
let (_, winit_windows, _, _) = event_writer_system_state.get_mut(&mut app.world);
for window in winit_windows.windows.values() {
window.request_redraw();
}
}
};

trace!("starting winit event loop");
Expand Down Expand Up @@ -820,8 +833,7 @@ fn run_app_update_if_should(
app.update();

// decide when to run the next update
let (config, windows) = focused_windows_state.get(&app.world);
let focused = windows.iter().any(|window| window.focused);
let (config, _) = focused_windows_state.get(&app.world);
match config.update_mode(focused) {
UpdateMode::Continuous => {
runner_state.redraw_requested = true;
Expand Down