From d7ea5b6aa955abf18e4ce2f0dab785a0152667e0 Mon Sep 17 00:00:00 2001 From: Miles Silberling-Cook Date: Thu, 19 Sep 2024 19:55:41 -0500 Subject: [PATCH] Various picking bugfixes (#15293) # Objective - Intended to resolve https://github.com/bevyengine/bevy/issues/15290. - Fix four duplicate `DragEnd` firing when drag finished. - Fix redundant `DragStart` firing when dragging across pick-able entities. - Fix `Click` coming after `Drop` and obliterating finished drag interactions. Big thanks to B. Reinhart for testing picking in their codebase and identifying these issues early. ## Solution - Fix press & drag state being cleared after the first entity is read from the hover map on pointer release, rather than after all entities are read. This caused only the first hovered entity to receive `Up` and `Click` events. - Fixes `Down` being determined using the `previous_hover_map` rather than `hover_map`, a regression compared to `bevy_mod_picking`. I think this is what was messing up drag events. - Fixes and issue where `PointerEnd` would fire multiple times and `PointerStart` would fire when dragging onto a new entity. - Re-orders events to make them easier to handle. `Out` now fired before `DragLeave` and `Click/Up` now fire before `DragDrop`. - Generally refactors the picking event code to be more clean and sane. ## Testing These changes are currently sporadically tested. --- crates/bevy_picking/src/events.rs | 261 ++++++++++++++++-------------- 1 file changed, 141 insertions(+), 120 deletions(-) diff --git a/crates/bevy_picking/src/events.rs b/crates/bevy_picking/src/events.rs index cb6edebbf8d6e..07a7e80da2746 100644 --- a/crates/bevy_picking/src/events.rs +++ b/crates/bevy_picking/src/events.rs @@ -253,7 +253,7 @@ pub struct DragEntry { #[derive(Debug, Clone, Default)] pub struct PointerState { /// Stores the press location and start time for each button currently being pressed by the pointer. - pub pressing: HashMap, + pub pressing: HashMap, /// Stores the the starting and current locations for each entity currently being dragged by the pointer. pub dragging: HashMap, /// Stores the hit data for each entity currently being dragged over by the pointer. @@ -266,9 +266,9 @@ pub struct PointerState { /// + The sequence [`DragEnter`], [`Over`]. /// + Any number of any of the following: /// + For each movement: The sequence [`DragStart`], [`Drag`], [`DragOver`], [`Move`]. -/// + For each button press: Either [`Down`], or the sequence [`DragDrop`], [`DragEnd`], [`DragLeave`], [`Click`], [`Up`]. +/// + For each button press: Either [`Down`], or the sequence [`Click`], [`Up`], [`DragDrop`], [`DragEnd`], [`DragLeave`]. /// + For each pointer cancellation: Simply [`Cancel`]. -/// + Finally the sequence [`DragLeave`], [`Out`]. +/// + Finally the sequence [`Out`], [`DragLeave`]. /// /// Only the last event in a given sequence is garenteed to be present. /// @@ -326,6 +326,7 @@ pub fn pointer_events( ); continue; }; + // Possibly send DragEnter events for button in PointerButton::iter() { let state = pointer_state.entry((pointer_id, button)).or_default(); @@ -350,6 +351,7 @@ pub fn pointer_events( ); } } + // Always send Over events commands.trigger_targets( Pointer::new(pointer_id, location.clone(), Over { hit: hit.clone() }), @@ -370,76 +372,39 @@ pub fn pointer_events( PointerAction::Pressed { direction, button } => { let state = pointer_state.entry((pointer_id, button)).or_default(); - // Possibly emit DragEnd, DragDrop, DragLeave on button releases - if direction == PressDirection::Up { - // For each currently dragged entity - for (drag_target, drag) in state.dragging.drain() { - // Emit DragDrop - for (dragged_over, hit) in state.dragging_over.iter() { - commands.trigger_targets( - Pointer::new( - pointer_id, - location.clone(), - DragDrop { - button, - dropped: drag_target, - hit: hit.clone(), - }, - ), - *dragged_over, - ); - } - // Emit DragEnd - commands.trigger_targets( - Pointer::new( + // The sequence of events emitted depends on if this is a press or a release + match direction { + PressDirection::Down => { + // If it's a press, emit a Down event and mark the hovered entities as pressed + for (hovered_entity, hit) in hover_map + .get(&pointer_id) + .iter() + .flat_map(|h| h.iter().map(|(entity, data)| (*entity, data.clone()))) + { + let event = Pointer::new( pointer_id, location.clone(), - DragEnd { + Down { button, - distance: drag.latest_pos - drag.start_pos, + hit: hit.clone(), }, - ), - drag_target, - ); - // Emit DragLeave - for (dragged_over, hit) in state.dragging_over.iter() { - commands.trigger_targets( - Pointer::new( - pointer_id, - location.clone(), - DragLeave { - button, - dragged: drag_target, - hit: hit.clone(), - }, - ), - *dragged_over, ); - } - } - } - - // Send a Down or possibly a Click and an Up button events - for (hovered_entity, hit) in previous_hover_map - .get(&pointer_id) - .iter() - .flat_map(|h| h.iter().map(|(entity, data)| (*entity, data.clone()))) - { - match direction { - PressDirection::Down => { - // Send the Down event first - let event = - Pointer::new(pointer_id, location.clone(), Down { button, hit }); - commands.trigger_targets(event.clone(), hovered_entity); + commands.trigger_targets(event, hovered_entity); // Also insert the press into the state state .pressing - .insert(hovered_entity, (event.pointer_location, now)); + .insert(hovered_entity, (location.clone(), now, hit)); } - PressDirection::Up => { - // If this pointer previously pressed the hovered entity, first send a Click event - if let Some((_location, press_instant)) = - state.pressing.get(&hovered_entity) + } + PressDirection::Up => { + // Emit Click and Up events on all the previously hovered entities. + for (hovered_entity, hit) in previous_hover_map + .get(&pointer_id) + .iter() + .flat_map(|h| h.iter().map(|(entity, data)| (*entity, data.clone()))) + { + // If this pointer previously pressed the hovered entity, emit a Click event + if let Some((_, press_instant, _)) = state.pressing.get(&hovered_entity) { commands.trigger_targets( Pointer::new( @@ -466,77 +431,132 @@ pub fn pointer_events( ), hovered_entity, ); - // Also clear the state - state.pressing.clear(); - state.dragging.clear(); - state.dragging_over.clear(); } - }; - } - } - // Moved - PointerAction::Moved { delta } => { - for (hovered_entity, hit) in hover_map - .get(&pointer_id) - .iter() - .flat_map(|h| h.iter().map(|(entity, data)| (*entity, data.to_owned()))) - { - // Send drag events to entities being dragged or dragged over - for button in PointerButton::iter() { - let state = pointer_state.entry((pointer_id, button)).or_default(); - - // Emit a DragStart the first time the pointer moves while pressing an entity - for (location, _instant) in state.pressing.values() { - if state.dragging.contains_key(&hovered_entity) { - continue; // this entity is already logged as being dragged + + // Then emit the drop events. + for (drag_target, drag) in state.dragging.drain() { + // Emit DragDrop + for (dragged_over, hit) in state.dragging_over.iter() { + commands.trigger_targets( + Pointer::new( + pointer_id, + location.clone(), + DragDrop { + button, + dropped: drag_target, + hit: hit.clone(), + }, + ), + *dragged_over, + ); } - state.dragging.insert( - hovered_entity, - DragEntry { - start_pos: location.position, - latest_pos: location.position, - }, - ); + // Emit DragEnd commands.trigger_targets( Pointer::new( pointer_id, location.clone(), - DragStart { + DragEnd { button, - hit: hit.clone(), + distance: drag.latest_pos - drag.start_pos, }, ), - hovered_entity, + drag_target, ); + // Emit DragLeave + for (dragged_over, hit) in state.dragging_over.iter() { + commands.trigger_targets( + Pointer::new( + pointer_id, + location.clone(), + DragLeave { + button, + dragged: drag_target, + hit: hit.clone(), + }, + ), + *dragged_over, + ); + } } - // Emit a Drag event to the dragged entity when it is dragged over another entity. - for (dragged_entity, drag) in state.dragging.iter_mut() { - let drag_event = Drag { - button, - distance: location.position - drag.start_pos, - delta: location.position - drag.latest_pos, - }; - drag.latest_pos = location.position; - let target = *dragged_entity; - let event = Pointer::new(pointer_id, location.clone(), drag_event); - commands.trigger_targets(event, target); + // Finally, we can clear the state of everything relating to presses or drags. + state.pressing.clear(); + state.dragging.clear(); + state.dragging_over.clear(); + } + } + } + // Moved + PointerAction::Moved { delta } => { + // Triggers during movement even if not over an entity + for button in PointerButton::iter() { + let state = pointer_state.entry((pointer_id, button)).or_default(); + + // Emit DragEntry and DragStart the first time we move while pressing an entity + for (press_target, (location, _, hit)) in state.pressing.iter() { + if state.dragging.contains_key(press_target) { + continue; // This entity is already logged as being dragged } + state.dragging.insert( + *press_target, + DragEntry { + start_pos: location.position, + latest_pos: location.position, + }, + ); + commands.trigger_targets( + Pointer::new( + pointer_id, + location.clone(), + DragStart { + button, + hit: hit.clone(), + }, + ), + *press_target, + ); + } - // Emit a DragOver to the hovered entity when dragging a different entity over it. - for drag_target in state.dragging.keys() - .filter( - |&&drag_target| hovered_entity != drag_target, /* can't drag over itself */ - ) + // Emit Drag events to the entities we are dragging + for (drag_target, drag) in state.dragging.iter_mut() { + let drag_event = Drag { + button, + distance: location.position - drag.start_pos, + delta: location.position - drag.latest_pos, + }; + drag.latest_pos = location.position; + let event = Pointer::new(pointer_id, location.clone(), drag_event); + commands.trigger_targets(event, *drag_target); + + // Emit corresponding DragOver to the hovered entities + for (hovered_entity, hit) in hover_map + .get(&pointer_id) + .iter() + .flat_map(|h| h.iter().map(|(entity, data)| (*entity, data.to_owned()))) + .filter(|(hovered_entity, _)| *hovered_entity != *drag_target) { commands.trigger_targets( - Pointer::new(pointer_id, location.clone(), DragOver { button, dragged: *drag_target, hit: hit.clone() }), + Pointer::new( + pointer_id, + location.clone(), + DragOver { + button, + dragged: *drag_target, + hit: hit.clone(), + }, + ), hovered_entity, ); } } + } - // Always send Move event + for (hovered_entity, hit) in hover_map + .get(&pointer_id) + .iter() + .flat_map(|h| h.iter().map(|(entity, data)| (*entity, data.to_owned()))) + { + // Emit Move events to the entities we are hovering commands.trigger_targets( Pointer::new( pointer_id, @@ -593,6 +613,13 @@ pub fn pointer_events( ); continue; }; + + // Always send Out events + commands.trigger_targets( + Pointer::new(pointer_id, location.clone(), Out { hit: hit.clone() }), + hovered_entity, + ); + // Possibly send DragLeave events for button in PointerButton::iter() { let state = pointer_state.entry((pointer_id, button)).or_default(); @@ -612,12 +639,6 @@ pub fn pointer_events( ); } } - - // Always send Out events - commands.trigger_targets( - Pointer::new(pointer_id, location.clone(), Out { hit: hit.clone() }), - hovered_entity, - ); } } }