Update window scale on window creation

[Dimchikkk]

Objective

Fixes #10407

I guess this is not a proper fix but more like a workaround, just sharing maybe someone will come up with better idea...

Solution

WindowScaleFactorChanged is fired on window creation, allowing to refresh window scale.

[github-actions]

Welcome, new contributor!

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

[UkoeHB]

It looks like this sends an event but doesn't actually change anything in the app. Are additional changes needed to fix the text issues (follow-up PR?)?

[Dimchikkk]

It looks like this sends an event but doesn't actually change anything in the app. Are additional changes needed to fix the text issues (follow-up PR?)?

Good question. Basically the situation is the following:

/// pre-PR#9826
2023-11-09T18:35:27.367530Z  INFO bevy_winit::system: Creating new window "App" (0v0)
2023-11-09T18:35:27.435296Z  INFO windowbug: F0 WindowResolution { physical_width: 500, physical_height: 300, scale_factor_override: Some(1.9), scale_factor: 2.0 }
2023-11-09T18:35:27.450425Z  INFO windowbug: F1 WindowResolution { physical_width: 500, physical_height: 300, scale_factor_override: Some(1.9), scale_factor: 2.0 }

// post-PR#9826
2023-11-09T18:34:53.438184Z  INFO windowbug: F0 WindowResolution { physical_width: 500, physical_height: 300, scale_factor_override: Some(1.9), scale_factor: 1.0 }
2023-11-09T18:34:53.463914Z  INFO bevy_winit::system: Creating new window "App" (0v0)
2023-11-09T18:34:53.523100Z  INFO windowbug: F1 WindowResolution { physical_width: 500, physical_height: 300, scale_factor_override: Some(1.9), scale_factor: 2.0 }

Window creation log may be printed after first frame (F0), so on Retina display it may be situation that

Frame 1: scale has default value 1
Frame 2: scale updated to 2

BUT WindowScaleFactorChanged is not fired in such scenario, the hotfix implements sending WindowScaleFactorChanged so userland can act on it. In my plugin I don't need to add any code since I'm already handling scale change event: https://github.com/StaffEngineer/bevy_cosmic_edit/blob/0758d43b98729a0368b0d1ea7a9412581672ac97/src/render.rs#L479

I am not sure that this is a proper fix, another option is to revert PR#9826... or maybe someone more familiar with the engine internals has better idea how to tackle this issue.

[UkoeHB]

Hmm this reminds me of another MacOS issue: #8391. It seems like winit events are not sent in some circumstances. Honestly the execution flow around winit_runner is quite unclear to me after staring at it for a bit, someone who knows more about the internals of winit needs to look at this.

I am concerned this PR may only be a bandaid on a much bigger problem - how many other window events are not being processed correctly?

Also, does this PR need to take into account the other stuff going on here?

WindowEvent::ScaleFactorChanged {
    scale_factor,
    new_inner_size,
} => {
    event_writers.window_backend_scale_factor_changed.send(
        WindowBackendScaleFactorChanged {
            window: window_entity,
            scale_factor,
        },
    );

    let prior_factor = window.resolution.scale_factor();
    window.resolution.set_scale_factor(scale_factor);
    let new_factor = window.resolution.scale_factor();

    if let Some(forced_factor) = window.resolution.scale_factor_override() {
        // This window is overriding the OS-suggested DPI, so its physical size
        // should be set based on the overriding value. Its logical size already
        // incorporates any resize constraints.
        *new_inner_size =
            winit::dpi::LogicalSize::new(window.width(), window.height())
                .to_physical::<u32>(forced_factor);
    } else if approx::relative_ne!(new_factor, prior_factor) {
        event_writers.window_scale_factor_changed.send(
            WindowScaleFactorChanged {
                window: window_entity,
                scale_factor,
            },
        );
    }

    let new_logical_width = (new_inner_size.width as f64 / new_factor) as f32;
    let new_logical_height = (new_inner_size.height as f64 / new_factor) as f32;
    if approx::relative_ne!(window.width(), new_logical_width)
        || approx::relative_ne!(window.height(), new_logical_height)
    {
        event_writers.window_resized.send(WindowResized {
            window: window_entity,
            width: new_logical_width,
            height: new_logical_height,
        });
    }
    window
        .resolution
        .set_physical_resolution(new_inner_size.width, new_inner_size.height);
}

[alice-i-cecile]

Can you test: is this change still required following the merging of #10389?

I strongly suspect that this was introduced during #9826.

[alice-i-cecile]

@devil-ira, I would appreciate your input here as well if you have opinions.

[UkoeHB]

@alice-i-cecile according to this comment it was not fixed by #10389 @rparrett.

[alice-i-cecile]

These changes seem basically fine to me as a robustness improvement / workaround, but I'd like to verify that we can reproduce the problem before merging.

[Dimchikkk]

It seems like winit events are not sent in some circumstances.

@UkoeHB in this case it's another situation, smt like:

-- bevy's frame 1 (default window scale is used (value 1)): scale = 1
-- winit window is created (window scale is 2) 
(winit shouldn't send any scale change event since window is just created)
-- bevy's frame 2: scale = 1

the current fix is basically

-- bevy's frame 1 (default window scale is used (value 1)): scale = 1
-- winit window is created (window scale is 2)
-- propogate window scale update using bevy's `WindowScaleFactorChanged` event
-- bevy's frame 2: scale = 2

[Dimchikkk]

These changes seem basically fine to me as a robustness improvement / workaround, but I'd like to verify that we can reproduce the problem before merging.

Tested just now on the latest main (33cd59f). Issue still occurs for me.

[UkoeHB]

@UkoeHB in this case it's another situation, smt like:

My question is, where is the scale changing so that a winit event used to be emitted but now is not emitted? Are there other winit events that are not being emitted because the window creation is delayed? What about all that other logic that winit_runner is doing when it receives a scale change event (are we breaking invariants by not replicating that logic in this PR?)?

[Dimchikkk]

My question is, where is the scale changing so that a winit event used to be emitted but now is not emitted?

As far as I understand the code (may be wrong) before user could read in setup system window scale and that value was "not default one" but from winit (2 for Retina display) (because window was created before setup system run)... so in the previous version of code we don't need to fire bevy's window scale event change... because it's always 2 for a Retina user :)

I am not sure about that invariant... code changes seems work fine for me but surely more testing is appreciated. It just sounds logical to me if we had scale 1 we have to send scale change event if scale is 2... better option would be to properly fix it and create window as it was before.. before the first frame.

[mockersf]

I believe this is not the right fix. with this, text will be rendered once without the proper scale, then once more with it.

With #9826 or #10389, there is still an update running before the window is actually created in winit, so before we have the proper scale.

I think instead we should investigate on each platform where the proper step to create windows should be, and ensure it's correctly place relative to the update

[Dimchikkk]

@mockersf yep, completely agree that the proper fix is to move window creation back as it was in 0.11 for MacOS.

[Dimchikkk]

@mockersf what do you think about running recently introduced update call only for iOS?

main...StaffEngineer:bevy:fix-macos-scaling#diff-b2fba3a0c86e496085ce7f0e3f1de5960cb754c7d215ed0f087aa556e529f97fR317