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

mockersf · 2024-01-07T15:00:37Z

Objective

Since Fix perf degradation on web builds #11227, Bevy doesn't work on mobile anymore. Windows are not created.

Solution

Create initial window on mobile after the initial Resume event. macOS is included because it's excluded from the other initial window creation and I didn't want it to feel alone. Also, it makes sense. this is needed for Android

bevy/crates/bevy_winit/src/lib.rs

Line 152 in cfcb688

#[cfg(not(any(target_os = "android", target_os = "ios", target_os = "macos")))]
request redraw during plugin initialisation (needed for WebGPU)
request redraw when receiving AboutToWait instead of at the end of the event handler. request to redraw during a RedrawRequested event are ignored on iOS

alice-i-cecile

Ah, this is a nice and straightforward fix. Sorry for missing this in my review.

nicopap

I can't really test it, so I'm not confident thumbing up.

But I see window creation on the Event::NewEvents(StartCause::Init) for ios and macos. (lines 367 to 391) Are those now redundant? If they don't run, we should probably remove those lines.

mockersf · 2024-01-08T21:01:39Z

But I see window creation on the Event::NewEvents(StartCause::Init) for ios and macos. (lines 367 to 391) Are those now redundant? If they don't run, we should probably remove those lines.

Right, we should!

The first commit just fixed Android, and worked on iOS locally due to a cached build artefact with another change.

I added a correct fix for iOS (don't request redraw during a RedrawRequested event) and for WebGPU which was also broken (request redraw during plugin initialisation).

I tested this PR on macOS, WebGL2, WebGPU, iOS and Android

daxpedda · 2024-01-09T10:13:31Z

crates/bevy_winit/src/lib.rs

-                        );
-
-                        create_window_system_state.apply(&mut app.world);
+            Event::AboutToWait => {


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.

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

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

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

Yeah, this would be a bug in Winit.
Will report back when I know 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?

@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!

daxpedda · 2024-01-09T10:22:58Z

crates/bevy_winit/src/lib.rs

@@ -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"))]


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!

daxpedda · 2024-01-09T10:27:18Z

crates/bevy_winit/src/lib.rs

@@ -682,7 +663,6 @@ pub fn winit_runner(mut app: App) {
                event: DeviceEvent::MouseMotion { delta: (x, y) },
                ..
            } => {
-                runner_state.redraw_requested = true;


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.

nicopap

LGTM. Nice changes. I was wary of using AboutToWait, because of the disclaimer in the doc. But given the restriction on redraw_request on iOS and the fact there isn't an alternative seems to make it the only sensible choice.

mobile: create window on initial resume

71a45c6

mockersf added C-Bug An unexpected or incorrect behavior O-Android Specific to the Android mobile operating system O-iOS Specific to the iOS mobile operating system labels Jan 7, 2024

mockersf mentioned this pull request Jan 7, 2024

Fix perf degradation on web builds #11227

Merged

alice-i-cecile added this to the 0.13 milestone Jan 7, 2024

alice-i-cecile approved these changes Jan 7, 2024

View reviewed changes

nicopap added the P-High This is particularly urgent, and deserves immediate attention label Jan 7, 2024

nicopap reviewed Jan 7, 2024

View reviewed changes

mockersf marked this pull request as draft January 7, 2024 21:00

alice-i-cecile added the A-Windowing Platform-agnostic interface layer to run your app in label Jan 7, 2024

mockersf added 6 commits January 8, 2024 21:53

don't update on MouseMotion

b4a872c

request update on resume

104af29

cleanup

6ecdde6

request update during plugin initialization

ea29004

cleanup

a14c92c

request redraw on ABoutToWait instead of after an event

8005aaa

mockersf changed the title ~~mobile: create window on initial resume~~ mobile and webgpu: trigger redraw request when needed and improve window creation Jan 8, 2024

mockersf added the O-WebGPU Specific to the WebGPU render API label Jan 8, 2024

mockersf requested review from alice-i-cecile and nicopap January 8, 2024 20:59

mockersf marked this pull request as ready for review January 8, 2024 20:59

alice-i-cecile approved these changes Jan 8, 2024

View reviewed changes

alice-i-cecile mentioned this pull request Jan 8, 2024

Cleanup bevy_winit #11257

Closed

daxpedda reviewed Jan 9, 2024

View reviewed changes

nicopap approved these changes Jan 9, 2024

View reviewed changes

alice-i-cecile 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 Jan 9, 2024

alice-i-cecile added this pull request to the merge queue Jan 9, 2024

Merged via the queue into bevyengine:main with commit 0e61435 Jan 9, 2024
24 checks passed

IceSentry mentioned this pull request Jan 9, 2024

low_power example freezes #11274

Closed

nicopap mentioned this pull request Jan 29, 2024

0.13 announcement post bevyengine/bevy-website#891

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mockersf commented Jan 7, 2024 •

edited

Loading

alice-i-cecile left a comment

nicopap left a comment

mockersf commented Jan 8, 2024 •

edited

Loading

daxpedda Jan 9, 2024

mockersf Jan 9, 2024

daxpedda Jan 9, 2024

mockersf Jan 9, 2024 •

edited

Loading

daxpedda Jan 9, 2024

daxpedda Jan 9, 2024

mockersf Jan 10, 2024

daxpedda Jan 9, 2024 •

edited

Loading

daxpedda Jan 9, 2024 •

edited

Loading

nicopap left a comment

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

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

Conversation

mockersf commented Jan 7, 2024 • edited Loading

Objective

Solution

alice-i-cecile left a comment

Choose a reason for hiding this comment

nicopap left a comment

Choose a reason for hiding this comment

mockersf commented Jan 8, 2024 • edited Loading

daxpedda Jan 9, 2024

Choose a reason for hiding this comment

mockersf Jan 9, 2024

Choose a reason for hiding this comment

daxpedda Jan 9, 2024

Choose a reason for hiding this comment

mockersf Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

daxpedda Jan 9, 2024

Choose a reason for hiding this comment

daxpedda Jan 9, 2024

Choose a reason for hiding this comment

mockersf Jan 10, 2024

Choose a reason for hiding this comment

daxpedda Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

daxpedda Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

nicopap left a comment

Choose a reason for hiding this comment

mockersf commented Jan 7, 2024 •

edited

Loading

mockersf commented Jan 8, 2024 •

edited

Loading

mockersf Jan 9, 2024 •

edited

Loading

daxpedda Jan 9, 2024 •

edited

Loading

daxpedda Jan 9, 2024 •

edited

Loading