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.

Sign up for GitHub

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

Jump to bottom

Check cursor position for out of bounds of the window #8855

Merged
merged 5 commits into from
Aug 28, 2023
Merged

Check cursor position for out of bounds of the window #8855

merged 5 commits into from
Aug 28, 2023

Conversation

Aceeri
Copy link
Member

@Aceeri Aceeri commented Jun 16, 2023 •
edited
Loading

Objective

Fixes #8840

Make the cursor position more consistent, right now the cursor position is sometimes outside of the window and returns the position and sometimes None.

Even in the cases where someone might be using that position that is outside of the window, it'll probably require some manual transformations for it to actually be useful.

Solution

Check the windows width and height for out of bounds positions.

Changelog

  • Cursor position is now always None when outside of the window.

@Aceeri Aceeri added A-Windowing Platform-agnostic interface layer to run your app in C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Jun 16, 2023
@mockersf
Copy link
Member

It is said in #8840 that the event is correctly fired from winit. If that's the case, could you try and fix the bug where it's not setting the position to None instead of adding bound checks?

@Plonq
Copy link

Plonq commented Jun 16, 2023 •
edited
Loading

Check out my latest comment: #8840 (comment)

EDIT: We could solve this without bounds checks, by storing a boolean for 'is cursor within this window', which is updated on cursor entered and left events. Then we could ignore new cursor positions if the cursor isn't within the window.
However, this doesn't address the root problem, and creates a sort of disconnect between cursor position and window events, which could cause other issues, or at least be confusing, so I'm not a fan of it.

@Aceeri
Copy link
Member Author

Aceeri commented Jun 16, 2023

Tbh I think moving the bounds checking to the setting of physical position would be the best bet here. That way even user setting of physical cursor position doesn't give weird results.

@Aceeri
Copy link
Member Author

Aceeri commented Jun 17, 2023

Actually hmm, maybe doing this on get methods is better, this handles problems if the window resizes for whatever reason as well.

Aceeri and others added 2 commits June 17, 2023 02:19
@Aceeri @ickshonpe
Use Rect instead of manual bounds checks
c77174a 
Co-authored-by: ickshonpe <david.curthoys@googlemail.com>
@Aceeri
Import Rect
a00ccb4
@Plonq
Copy link

Plonq commented Jun 18, 2023

Should this maybe be an optional feature? E.g. a boolean argument ignore_out_of_bounds, or maybe the inverse?

@Aceeri
Copy link
Member Author

Aceeri commented Jun 18, 2023

Should this maybe be an optional feature? E.g. a boolean argument ignore_out_of_bounds, or maybe the inverse?

I don't think so tbh. I'd be fine with adding a cursor_position_unchecked, but I don't see the use case for it. The oob cursors are highly inconsistent and prob shouldn't be relied on here.

@alice-i-cecile 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 Aug 7, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 14, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 14, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 14, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 14, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 28, 2023
Merged via the queue into bevyengine:main with commit 3cf94e7 Aug 28, 2023
@Aceeri Aceeri deleted the oob-cursor branch August 28, 2023 21:50
@Aceeri Aceeri mentioned this pull request Aug 30, 2023
Open
@rparrett rparrett mentioned this pull request Aug 31, 2023
Closed
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
@Aceeri @ickshonpe
Check cursor position for out of bounds of the window (bevyengine#8855)
7213809 
# Objective
Fixes bevyengine#8840 

Make the cursor position more consistent, right now the cursor position
is *sometimes* outside of the window and returns the position and
*sometimes* `None`.

Even in the cases where someone might be using that position that is
outside of the window, it'll probably require some manual
transformations for it to actually be useful.

## Solution
Check the windows width and height for out of bounds positions.

---

## Changelog
- Cursor position is now always `None` when outside of the window.

---------

Co-authored-by: ickshonpe <david.curthoys@googlemail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ickshonpe ickshonpe ickshonpe approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@aevyrie aevyrie Awaiting requested review from aevyrie

@IceSentry IceSentry Awaiting requested review from IceSentry

Assignees
No one assigned
Labels
A-Windowing Platform-agnostic interface layer to run your app in C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Window.cursor_position can return Some when it shouldn't
5 participants
@Aceeri @mockersf @Plonq @alice-i-cecile @ickshonpe