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.

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

Fix mouse behavior in scale_factor_override example #7382

Closed
wants to merge 1 commit into from

Conversation

rparrett
Copy link
Contributor

@rparrett rparrett commented Jan 27, 2023

Objective

Fixes #7377

Solution

Add an intermediate resource for the override state so that we can update the title only when it has been changed.

Other potential options:

  1. Just revert the non-essential changes made to this example in [Merged by Bors] - Windows as Entities #5589.
  2. Consolidate the toggle and change systems and display systems into one, and keep track of "changedness" there
  3. Make the display system a normal function and call it from the toggle and change systems
  4. Display the current setting in the Text instead, checking to see if the formatted text differs from the current value

@rparrett rparrett added C-Bug An unexpected or incorrect behavior C-Examples An addition or correction to our examples labels Jan 27, 2023
@tim-blackbird
Copy link
Contributor

tim-blackbird commented Jan 27, 2023

Bevy shouldn't be messing with the cursor position just because some property of the window was changed.
Looks like .bypass_change_detection() is being abused to avoid this problem internally. I don't like that at all :\

edit: I have an idea of how to fix this properly. I'll probably make a PR soon-ish

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Jan 27, 2023
@mockersf
Copy link
Member

The complete fix would be to have different api to update the cursor position from Winit, and to expose to the user. Then add a flag to it, so we know when it comes from the user. This is closer to how it was done before the window as entity PR. I hoped change detection would be enough to avoid the issue, but as it keeps popping up we need to go back to the difference between internal/user demanded updates.

@tim-blackbird
Copy link
Contributor

The fix is pretty simple. A clone of the Window struct to compare against to see what properties the user has changed, but this clone needs to be updated when we receive new info about the window from winit so we don't see them as user changes, which would cause a feedback loop.

@rparrett
Copy link
Contributor Author

rparrett commented Feb 1, 2023

Even if this is "properly fixed," it's probably not good practice to update the window title every frame. But a proper fix might enable a more elegant solution.

I don't personally feel like this example should update the window title anyhow, so I'm just gonna close those.

@rparrett rparrett closed this Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Bug An unexpected or incorrect behavior C-Examples An addition or correction to our examples X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mouse movement is sluggish in scale_factor_override and window_settings examples
4 participants