Skip to content

Added PressedButtons to PickingInteraction::Pressed. #16004

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

mintlu8
Copy link
Contributor

@mintlu8 mintlu8 commented Oct 19, 2024

Objective

Fixes #15983. PickingInteraction is preferable to events in some use cases for code organization. This PR adds the keys pressed to PickingInteraction::Pressed, allowing the user to ignore certain keys like the RMB in such use cases.

Solution

  • Added field PressedButtons to PickingInteraction::Pressed.

Testing

  • Added a test for merging PickingInteractions.
  • Added an example for PickingInteraction.

Migration Guide

PickingInteraction::Pressed now has a field, code matching against it should use matches!(interaction, PickingInteraction::Pressed(..)) instead.

@IQuick143 IQuick143 added C-Feature A new feature, making something new possible D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward A-Picking Pointing at and selecting objects of all sorts labels Oct 19, 2024
@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Oct 19, 2024
@alice-i-cecile alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Oct 20, 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.

I like this. It makes PickingInteraction much more useful. The use of bitflag packing to reduce memory bloat is clever too.

@alice-i-cecile
Copy link
Member

This will be useful as part of #15597, and I'd like to get this merged first.

@BenjaminBrienen
Copy link
Contributor

BenjaminBrienen commented Oct 28, 2024

Many computer mice have a "Back" and a "Forward" button, but I don't know if those affect focus.

/// Middle mouse button is pressed.
const MIDDLE = 4;
/// Touch input is pressed.
const TOUCH = 8;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is correct. Touch inputs are probably akin to a primary press, and some touch devices, like pens, can have a primary secondary, tertiary press, just like a mouse.

Copy link
Contributor

@BenjaminBrienen BenjaminBrienen Oct 29, 2024

Choose a reason for hiding this comment

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

I've had something like that before: my Note 9 has a pen and it supports hovering over something with the pen in the air and clicking a side-button to perform a secondary action.

Copy link
Contributor

Choose a reason for hiding this comment

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

A stylus is not a touch pointer. Winit does not currently support stylus input, so neither do we.

Copy link
Member

@aevyrie aevyrie Oct 29, 2024

Choose a reason for hiding this comment

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

It definitely works, I've tested it. It's treated like a touch input. This was on a MS surface with a surface pen.

Regardless, if you are matching on whether a button was pressed, why would you not want it to be considered pressed if it was touched by a different pointing device?

Copy link
Member

Choose a reason for hiding this comment

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

This is why I specifically used the wording "primary/secondary". A primary press can come from many types of devices.

Copy link
Contributor Author

@mintlu8 mintlu8 Oct 31, 2024

Choose a reason for hiding this comment

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

The PR just maps the input from picking in a form that makes the most sense to me. Definitely would like to see more detailed information and more buttons, but that has to come from picking first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to left and right.

Copy link
Member

@aevyrie aevyrie left a comment

Choose a reason for hiding this comment

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

Tying an interaction struct to hardware is asking for people to just default to the mouse, and suddenly a bunch of software doesn't work with other pointing devices for no good reason.

I would take a look at the JS pointer event APIs - they were designed to be hardware agnostic for this reason: https://developer.mozilla.org/en-US/docs/Web/API/Pointer_events#determining_button_states

I would strongly prefer we follow this model, and make the easy path one that is device agnostic. E.g. under the hood, this struct might be a huge bitflag that encodes which device buttons are down, but the easiest-to-access methods are things like is_primary_pressed, which return true if the primary button of any pointing device is down.

I'd also like to ask whether this structure should even exist anymore. It was originally added to match Interaction in bevy_ui. However, with observers, we can do the same thing with less memory and cpu, because we don't need to update the state of every interactive entity even if no one is interested in reading it. That might mean refactoring the pointer events a bit to coalesce button information.

@alice-i-cecile alice-i-cecile added X-Contentious There are nontrivial implications that should be thought through S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 29, 2024
@alice-i-cecile
Copy link
Member

I'd also like to ask whether this structure should even exist anymore. It was originally added to match Interaction in bevy_ui. However, with observers, we can do the same thing with less memory and cpu, because we don't need to update the state of every interactive entity even if no one is interested in reading it. That might mean refactoring the pointer events a bit to coalesce button information.

Yeah, this is increasingly my opinion. It's nice for the "change widget style based on interaction", but that's the only use case where it's nicer than observers.

@NthTensor
Copy link
Contributor

That might mean refactoring the pointer events a bit to coalesce button information.

Which I am more than happy to do if we get specs for the events we need. Enter/Leave is already on the radar.

@aevyrie
Copy link
Member

aevyrie commented Oct 29, 2024

Yeah, this is increasingly my opinion. It's nice for the "change widget style based on interaction", but that's the only use case where it's nicer than observers.

We definitely have a need to coalesce multiple events to determine state. You can't, for example, use Over and Out to determine if an entity is hovered, that entity might receive multiple of these events from multiple pointers and bubbled from children. I'm not sure what the right approach is though. I'm kinda leaning towards keeping this struct for that reason though.

I think this PR would be okay to merge with some modifications to make it match the pointer API I linked. It currently mixes the concept of abstract pointer buttons and device button.

  1. Change the name to PointerDevicePressed or something similar. This struct tracks which device buttons are down, but presents a public API that aggregates them, to help push users toward correct code. PressedButtons is probably fine too, but we should disambiguate from, say keyboard buttons. PointerPressed?
  2. Change PRIMARY/SECONDARY to something like LEFT_MOUSE/RIGHT_MOUSE/MIDDLE_MOUSE. E.g. every bit field should be device specific, this would also include mouse fwd/back and pen barrel when we have them. This structure would become an aggregate of all types of devices, if there are multiple devices (e.g. touch) all states should be OR'ed (like present).
  3. Make the fields private if possible, and expose methods primary(), secondary(), middle(). To expose the internal details, add any(button: PointerDeviceButtons) and all(button: PointerDeviceButtons) so they can check if a specific button(s) was pressed.
    • This would also support chording, because you could pass in buttons.any(PointerDeviceButtons::LMB | PointerDeviceButtons::RMB ) or buttons.all(PointerDeviceButtons::LMB | PointerDeviceButtons::RMB ).
    • LMB and touches should both be considered a primary press. RMB and pen barrel (once it is supported) would be secondary presses. Again, follow the spec used on the web.
  4. Virtual (custom) pointers should be forced to decide what kind of input they are mocking, e.g. touch/mouse/pen.

} else {
match id {
PointerId::Mouse => PickingInteraction::Pressed((*pointer_press).into()),
PointerId::Touch(_) => PickingInteraction::Pressed(PressedButtons::TOUCH),
Copy link
Member

Choose a reason for hiding this comment

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

See here for the mapping from Touch. I think that this is the right approach for the current PR.

@mintlu8
Copy link
Contributor Author

mintlu8 commented Oct 31, 2024

  1. Change the name to PointerDevicePressed or something similar. This struct tracks which device buttons are down, but presents a public API that aggregates them, to help push users toward correct code. PressedButtons is probably fine too, but we should disambiguate from, say keyboard buttons. PointerPressed?

Do you think it is fine to just reserve X1 and X2 on the bitflags? I don't see a reason to encapsulate it further.

https://developer.mozilla.org/en-US/docs/Web/API/Pointer_events#determining_button_states

According to this spec the current implementation is a upgrade on touch input since usually we treat touch as left click, but with this implementation you can potentially treat them differently.

@alice-i-cecile alice-i-cecile removed this from the 0.16 milestone Feb 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Picking Pointing at and selecting objects of all sorts C-Feature A new feature, making something new possible D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Picking: Filter keys in PickingInteraction
6 participants