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

Picking out order #16231

Merged
merged 5 commits into from
Nov 15, 2024
Merged

Picking out order #16231

merged 5 commits into from
Nov 15, 2024

Conversation

NthTensor
Copy link
Contributor

Tweaks picking docs slightly for formatting and to add additional context about the ordering of Over and Out events. Also shifts Out to trigger before Over in the global event ordering.

Because of how focus is tracked, we must send all Over and Out events at the same time, in a block. Originally I had Over precede Out in the global event order, because this seemed natural. However, the effect of this, when a pointer moves between entities, is to have the new entity receive Over before the old entity received Out, which several users found confusing.

The new ordering (out before over globally, over before out locally per entity) should make it much easier to write hover state cleanup code.

@NthTensor NthTensor added C-Bug An unexpected or incorrect behavior A-Input Player input via keyboard, mouse, gamepad, and more A-UI Graphical user interfaces, styles, layouts, and widgets D-Complex Quite challenging from either a design or technical perspective. Ask for help! A-Picking Pointing at and selecting objects of all sorts labels Nov 4, 2024
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Nov 4, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really excellent docs. I'd love to have some tests to verify that order, but that's a ton of extra work and I won't block on it.

@alice-i-cecile alice-i-cecile added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Nov 4, 2024
@NthTensor
Copy link
Contributor Author

I'd love to have some tests to verify that order, but that's a ton of extra work and I won't block on it.

ooh good idea. I think this will make sense to do as part of the next winit upgrade, because the way this pointer stuff interacts with their pointer stuff will be complex and not entirely owned by us.

Copy link
Contributor

@doup doup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment on pointer presses might be wrong? I think it should say down/click/up. Apart from that the ordering explained at the beginning looks consistent with the code.

Btw, great docs 👍

crates/bevy_picking/src/events.rs Outdated Show resolved Hide resolved
@mockersf
Copy link
Member

@NthTensor could you look into the conflict and fixing the order in the comment?

@NthTensor
Copy link
Contributor Author

Yep, thanks for the poke.

@NthTensor NthTensor added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Nov 15, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Nov 15, 2024
Merged via the queue into bevyengine:main with commit 56ac381 Nov 15, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Input Player input via keyboard, mouse, gamepad, and more A-Picking Pointing at and selecting objects of all sorts A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior D-Complex Quite challenging from either a design or technical perspective. Ask for help! 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
Development

Successfully merging this pull request may close these issues.

4 participants