-
-
Notifications
You must be signed in to change notification settings - Fork 4k
Remove the id from PointerId::Touch
#17847
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
base: main
Are you sure you want to change the base?
Remove the id from PointerId::Touch
#17847
Conversation
* Removed the `deactivate_touch_pointers` system. * `touch_pick_event` local `touch_cache` hashmap also tracks the entity id of touch pointers as well as the intial touch state. * `touch_pick_event` despawns touch point entities when the touch ends or is cancelled.
My understanding from @aevyrie is that this doesn't work because of interrupted touches. They will be despawned in Bevy but keep their identifier. |
I'm not sure what problems that could cause, maybe I'm misunderstanding what you mean by an interrupted touch? The winit docs say that their finger ids are only unique for the duration of a touch and that if another touch event happens after a |
…kshonpe/bevy into touch-picking-remove-touch-id
To be clear, I much prefer this solution if it works. I'm not confident on my understanding of the behavior here. |
I worked through the code and believe I understand it. Seems sound. I was also able to run the default_picking example locally without error |
We already have some pointer id changes in flight, and I'm a bit worried about doing a partial switch to just one type of pointer input. I think we need to better understand the lifecycle of a pointer, and evaluate if the pointer id is needed at all. I think the split to Entity + PointerKind is probably right, but last time I used Entity to identify pointers, it was causing things to blow up when part of the system tried to access an entity that didn't exist. If I recall correctly it was something like dragging outside a window would remove the pointer when it got to the edge of the screen, and when it returned and respawned, it caused drag/up events to explode. I don't know if this is still the case, but that's why I'd like to have a bit more of a thorough understanding of the lifecycle of a pointer (and document it!) before changing the ID. The current state of most of this stuff is an accumulation of bug fixes discovered in use, sometimes things are a bit ugly because they were needed to solve some weird edge case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only one real comment, other than "changing PointerId
without a better understanding of pointer entity lifecycles strikes a deep fear in my heart".
@@ -270,6 +262,8 @@ pub enum PointerAction { | |||
pub struct PointerInput { | |||
/// The id of the pointer. | |||
pub pointer_id: PointerId, | |||
/// The pointer entity | |||
pub pointer_entity: Option<Entity>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Picking is already inconsistent enough across backends, I would strongly prefer if we didn't add more inconsistency, though I appreciate your reasoning.
Objective
Instead of storing winit's
u64
finger id for touches on the pointer entity in itsPointerId
component, just use the pointer entity itself to represent the touch within Bevy.Related #17808
Solution
touch_pick_event
so its localtouch_cache
hashmap also tracks the entity id of touch pointers as well as the intial touch states.deactivate_touch_pointers
system. Insteadtouch_pick_event
despawns the corresponding touch pointer entity when a touch ends or is cancelled.pointer_entity: Option<Entity>
field to thePointerInput
event. This is used to match events to the correct pointer for inputs that support multiple pointers. For touch it would be used to distinguish between touches for multi-touch.Made
pointer_entity
optional so that this PR doesn't have to touch any of the other pointer event systems but it would probably make sense to require it (if this PR is adopted).Testing
Works when running the bevy examples in chrome with touch enabled in dev tools.