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 scale_factor_override with cursor position. #2639

Closed
wants to merge 1 commit into from
Closed

Fix scale_factor_override with cursor position. #2639

wants to merge 1 commit into from

Conversation

ahmedcharles
Copy link
Contributor

Sometimes the wrong scale factor was used (not accounting for
overrides).

Fixes #2501

Sometimes the wrong scale factor was used (not accounting for
overrides).

Fixes #2501
@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Aug 12, 2021
@cart
Copy link
Member

cart commented Aug 13, 2021

Nice catch! This implementation works, but to enable more flexibility and increased consistency, I think we should probably adopt our Window type's approach to width / physical_width here:

  1. Only store the physical_cursor_position on Window
  2. Add a physical_cursor_position getter method on Window.
  3. Make Window::cursor_position() return cursor_position / scale_factor (aka the "logical" position). Just look at Window::width() and mirror that.

Then when handling the winit events, all we need to do is:

  1. Set the "physical cursor position" when it changes
  2. Read the Window::cursor_position() from the window (to reuse the logic there).

@NiklasEi NiklasEi added A-Windowing Platform-agnostic interface layer to run your app in and removed S-Needs-Triage This issue needs to be labelled labels Aug 16, 2021
@ahmedcharles ahmedcharles deleted the scaling branch September 28, 2021 01:39
@swwind
Copy link

swwind commented Sep 30, 2021

Why this PR was closed? 🤔

I think it works fine for me.

@ahmedcharles
Copy link
Contributor Author

ahmedcharles commented Oct 2, 2021

Note: cart said that it works above.

I closed it because I don't plan on addressing the comments, as is my prerogative. This PR could've had two responses:

  1. Thanks, this works, I'll merge it. If you're looking for something else to do, please consider X.
  2. Thanks, this works, but I'll hold your contribution hostage until you resolve X.

Whether you think my characterization is fair or not, in specifics, the first one above clearly has no downside while showing new contributors that their time is valuable and the project avoids scope creep, at least for simple things. I don't plan on contributing to projects that don't value my time.

bors bot pushed a commit that referenced this pull request Oct 15, 2021
# Objective

- Fixes #2501 
- Builds up on #2639 taking #2639 (comment) into account

## Solution

- keep the physical cursor position in `Window`, and expose it.
- still convert to logical position in event, and when getting `cursor_position`


Co-authored-by: Ahmed Charles <acharles@outlook.com>
sharkdp pushed a commit to sharkdp/bevy that referenced this pull request Nov 8, 2021
# Objective

- Fixes bevyengine#2501 
- Builds up on bevyengine#2639 taking bevyengine#2639 (comment) into account

## Solution

- keep the physical cursor position in `Window`, and expose it.
- still convert to logical position in event, and when getting `cursor_position`


Co-authored-by: Ahmed Charles <acharles@outlook.com>
sharkdp pushed a commit to sharkdp/bevy that referenced this pull request Dec 12, 2021
# Objective

- Fixes bevyengine#2501 
- Builds up on bevyengine#2639 taking bevyengine#2639 (comment) into account

## Solution

- keep the physical cursor position in `Window`, and expose it.
- still convert to logical position in event, and when getting `cursor_position`


Co-authored-by: Ahmed Charles <acharles@outlook.com>
Subserial pushed a commit to Subserial/bevy_winit_hook that referenced this pull request Jan 24, 2024
# Objective

- Fixes #2501 
- Builds up on #2639 taking bevyengine/bevy#2639 (comment) into account

## Solution

- keep the physical cursor position in `Window`, and expose it.
- still convert to logical position in event, and when getting `cursor_position`


Co-authored-by: Ahmed Charles <acharles@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Windowing Platform-agnostic interface layer to run your app in
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Window scale_factor_override() doesn't affect cursor position
4 participants