Fix Reactive and ReactiveLowPower update modes #11325

mockersf · 2024-01-13T05:49:31Z

Objective

Partial fix for Button example is broken #11235
Fixes low_power example freezes #11274
Fixes WinitSettings::desktop_app() is broken #11320
Fixes window_settings example never becomes visible #11273

Solution

check update mode to trigger redraw request, instead of once a redraw request has been triggered
don't ignore device event in case of Reactive update mode
make sure that at least 5 updates are triggered on application start to ensure everything is correctly initialized
trigger manual updates instead of relying on redraw requests when there are no window or they are not visible

ldubos · 2024-01-13T06:02:01Z

I tested for the bug on Windows and the window never become visible :/
when I try to dbg!(frames.0) it never print anything in the console.

mockersf · 2024-01-13T06:08:37Z

thanks for testing! could you debug the event in the event loop and copy the log of events for the first few seconds when running the example on windows?
here:

bevy/crates/bevy_winit/src/lib.rs

Line 365 in e6a324a

match event {

ldubos · 2024-01-13T06:12:45Z

events:

[crates\bevy_winit\src\lib.rs:377] &event = NewEvents(
    Init,
)
[crates\bevy_winit\src\lib.rs:377] &event = Resumed
[crates\bevy_winit\src\lib.rs:377] &event = WindowEvent {
    window_id: WindowId(
        WindowId(
            3934682,
        ),
    ),
    event: Resized(
        PhysicalSize {
            width: 500,
            height: 300,
        },
    ),
}
[crates\bevy_winit\src\lib.rs:377] &event = AboutToWait
[crates\bevy_winit\src\lib.rs:377] &event = NewEvents(
    WaitCancelled {
        start: Instant {
            t: 97434.7408719s,
        },
        requested_resume: None,
    },
)
[crates\bevy_winit\src\lib.rs:377] &event = DeviceEvent {
    device_id: DeviceId(
        DeviceId(
            10813529,
        ),
    ),
    event: Added,
}
[crates\bevy_winit\src\lib.rs:377] &event = DeviceEvent {
    device_id: DeviceId(
        DeviceId(
            134547849,
        ),
    ),
    event: Added,
}
[crates\bevy_winit\src\lib.rs:377] &event = DeviceEvent {
    device_id: DeviceId(
        DeviceId(
            212731889,
        ),
    ),
    event: Added,
}
[crates\bevy_winit\src\lib.rs:377] &event = DeviceEvent {
    device_id: DeviceId(
        DeviceId(
            148442009,
        ),
    ),
    event: Added,
}
[crates\bevy_winit\src\lib.rs:377] &event = DeviceEvent {
    device_id: DeviceId(
        DeviceId(
            65603,
        ),
    ),
    event: Added,
}
[crates\bevy_winit\src\lib.rs:377] &event = AboutToWait

nicopap

I'm on Linux/X11.

Testing with the button example:

cargo run --example button

Still shows the bugs described in #11235.

Thank you for cleaning after us, but this isn't ready yet.

nicopap · 2024-01-13T08:31:47Z

crates/bevy_winit/src/lib.rs

@@ -254,6 +254,8 @@ struct WinitAppRunnerState {
    active: ActiveState,
    /// Is `true` if a new [`WindowEvent`] has been received since the last update.
    window_event_received: bool,
+    /// Is `true` if a new [`DeviceEvent`] has been received since the last update.
+    device_event_received: bool,


In a future PR we could replace this with a bitbybit or bilge bitflags To reduce the size of this struct

mockersf · 2024-01-13T11:24:25Z

I did some fixes for Windows:

Windows gets fewer RedrawRequested events at start, so I'm forcing to send at least 5 on application start. This feels a bit hackish but I don't think it should be an issue
Windows doesn't send events if there are no visible windows. now this case is handled on its own by triggering a manual update

mockersf · 2024-01-13T11:49:45Z

For the button example, it should be a partial fix, for the first steps with the transparent window.

the fact that it's not updated is "work as intended", or the bug is in rendering ordering, not in windows management: ui nodes are queued before they are extracted, so the state being drawn is the one from the previous update. extract_uinodes runs after queue_uinodes

I did not have that issue because I'm on a macOS laptop and using the touchpad, which sends events for touch pad pressure and I'm not maintaining the exact same constant pressure so new events are send continuously while clicking

alice-i-cecile · 2024-01-13T13:46:19Z

crates/bevy_winit/src/lib.rs

+                    }
+                };
+
+                if runner_state.start_forced_updates > 0 {


Commenting here about why this is required will help avoid breaking this again.

alice-i-cecile · 2024-01-13T13:48:11Z

crates/bevy_winit/src/lib.rs

            redraw_requested: false,
            wait_elapsed: false,
            last_update: Instant::now(),
            scheduled_update: None,
+            start_forced_updates: 5,


A comment here about how this number was chosen would help a lot.

very scientifically!
on the windows machine I tested I was getting 1 event at start, on my mac it's 3, so 5 has a safe margin of error

crates/bevy_winit/src/lib.rs

nicopap · 2024-01-13T15:54:31Z

ui nodes are queued before they are extracted

I'm not sure I follow. In

bevy/crates/bevy_ui/src/render/mod.rs

Lines 80 to 101 in 69760c7

    
           .add_systems( 
        
               ExtractSchedule, 
        
               ( 
        
                   extract_default_ui_camera_view::<Camera2d>, 
        
                   extract_default_ui_camera_view::<Camera3d>, 
        
                   extract_uinodes.in_set(RenderUiSystem::ExtractNode), 
        
                   extract_atlas_uinodes 
        
                       .in_set(RenderUiSystem::ExtractAtlasNode) 
        
                       .after(RenderUiSystem::ExtractNode), 
        
                   extract_uinode_borders.after(RenderUiSystem::ExtractAtlasNode), 
        
                   #[cfg(feature = "bevy_text")] 
        
                   extract_text_uinodes.after(RenderUiSystem::ExtractAtlasNode), 
        
                   extract_uinode_outlines.after(RenderUiSystem::ExtractAtlasNode), 
        
               ), 
        
           ) 
        
           .add_systems( 
        
               Render, 
        
               ( 
        
                   queue_uinodes.in_set(RenderSet::Queue), 
        
                   sort_phase_system::<TransparentUi>.in_set(RenderSet::PhaseSort), 
        
                   prepare_uinodes.in_set(RenderSet::PrepareBindGroups), 
        
               ),

queue_uinodes is put in the Queue render set, which is part of the Render schedule, which is the "main" schedule of the render app. As the SubApp::new documentation says, the "main" schedule will always run after the 2nd argument to SubApp::new is ran. In the case of the RenderApp, the 2nd argument to SubApp::new calls extract, which itself runs the ExtractSchedule. So indeed, queue_uinodes is ran after extract_uinodes, and before rendering.

My best guess would be that the input change has 1 frame of latency. Looking at Interaction, it's updated in ui_focus_system which itself is scheduled after InputSystem, in PreUpdate, which is before button_system, so not that. But what? hmm idk

nicopap

This new revision fixes the issue with the window not rendering on Linux/X11 (When I tested it during my last review, it didn't even show up). I think it's good to go once alice's comments are addressed.

mockersf · 2024-01-13T16:05:38Z

@nicopap could you try running the button example on this branch? https://github.com/mockersf/bevy/tree/button-ui-system-order

It's just adding some prints. I don't have access to a windows anymore so I'm going from memory of what I saw earlier, but the green background was extracted and not queued on the first click, and the extract system was running after the queue system

alice-i-cecile

Button example works on Windows for me!

check for update conditions outside of update

a4908d8

mockersf added the A-Windowing Platform-agnostic interface layer to run your app in label Jan 13, 2024

mockersf force-pushed the fix-reactive branch from 14b67bc to 08472af Compare January 13, 2024 05:54

don't ignore other device events

5235b00

mockersf force-pushed the fix-reactive branch from 08472af to 5235b00 Compare January 13, 2024 05:55

clippy

e66a628

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

nicopap suggested changes Jan 13, 2024

View reviewed changes

fixes for windows

20cce3d

alice-i-cecile added the C-Bug An unexpected or incorrect behavior label Jan 13, 2024

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

alice-i-cecile reviewed Jan 13, 2024

View reviewed changes

crates/bevy_winit/src/lib.rs Outdated Show resolved Hide resolved

nicopap approved these changes Jan 13, 2024

View reviewed changes

rename and comment

c243aaf

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

Texture Atlas rework #5103

Merged

alice-i-cecile approved these changes Jan 15, 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 15, 2024

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

Merged via the queue into bevyengine:main with commit 3d628a8 Jan 15, 2024
23 checks passed

Vrixyz mentioned this pull request Jan 18, 2024

Winit 0.29 follow up #11052

Closed

23 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Reactive and ReactiveLowPower update modes #11325

Fix Reactive and ReactiveLowPower update modes #11325

mockersf commented Jan 13, 2024 •

edited

Loading

ldubos commented Jan 13, 2024

mockersf commented Jan 13, 2024 •

edited

Loading

ldubos commented Jan 13, 2024

nicopap left a comment

nicopap Jan 13, 2024

mockersf commented Jan 13, 2024

mockersf commented Jan 13, 2024 •

edited

Loading

alice-i-cecile Jan 13, 2024

alice-i-cecile Jan 13, 2024

mockersf Jan 13, 2024

nicopap commented Jan 13, 2024

nicopap left a comment

mockersf commented Jan 13, 2024

alice-i-cecile left a comment

Fix Reactive and ReactiveLowPower update modes #11325

Fix Reactive and ReactiveLowPower update modes #11325

Conversation

mockersf commented Jan 13, 2024 • edited Loading

Objective

Solution

ldubos commented Jan 13, 2024

mockersf commented Jan 13, 2024 • edited Loading

ldubos commented Jan 13, 2024

nicopap left a comment

Choose a reason for hiding this comment

nicopap Jan 13, 2024

Choose a reason for hiding this comment

mockersf commented Jan 13, 2024

mockersf commented Jan 13, 2024 • edited Loading

alice-i-cecile Jan 13, 2024

Choose a reason for hiding this comment

alice-i-cecile Jan 13, 2024

Choose a reason for hiding this comment

mockersf Jan 13, 2024

Choose a reason for hiding this comment

nicopap commented Jan 13, 2024

nicopap left a comment

Choose a reason for hiding this comment

mockersf commented Jan 13, 2024

alice-i-cecile left a comment

Choose a reason for hiding this comment

mockersf commented Jan 13, 2024 •

edited

Loading

mockersf commented Jan 13, 2024 •

edited

Loading

mockersf commented Jan 13, 2024 •

edited

Loading