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

Drag and DragEnd system ordering inconsistencies #294

Open
Vrixyz opened this issue Dec 29, 2023 · 1 comment
Open

Drag and DragEnd system ordering inconsistencies #294

Vrixyz opened this issue Dec 29, 2023 · 1 comment

Comments

@Vrixyz
Copy link
Contributor

Vrixyz commented Dec 29, 2023

Context

I want to implement a drag and drop, when dropping, I want to "snap" the position.

bug description

Sometimes, the snapping occurs incorrectly. this bug doesn't happen on all sessions, but is easily reproducible after a few relaunches.

DragEnd event sometimes occurs before Drag events: so logic in DragEnd gets overridden by logic in Drag.

Video description

drag.and.drop.fail.snap.mp4

More details

part of relevant code

fn round_to_nearest(value: f32) -> f32 {
    (value / multiple).round() * 50f32
}
...
// handle drag
On::<Pointer<Drag>>::listener_component_mut::<Transform>(|drag, transform| {
    transform.translation.x += drag.delta.x;
    transform.translation.y -= drag.delta.y;
}),
On::<Pointer<DragEnd>>::run(|event: ListenerMut<Pointer<DragEnd>>,
      mut transforms: Query<&mut Transform>,| {
    
    let Ok(mut transform) = transforms.get_mut(event.listener()) else {
        return;
    };
    // snap to position
    transform.translation.x = round_to_nearest(transform.translation.x, 60f32); // Make the square follow the mouse
    transform.translation.y = round_to_nearest(transform.translation.y, 60f32);
    // re-enable picking
    commands.entity(event.target()).insert(Pickable::default());
})

Trace image

Not sure why there are 2 drag events on the same frame too 🤔

image

full tracy trace file: drag_trace.zip

Code source:https://github.com/Vrixyz/rsword/blob/8fbe5713856d971dc2e71eaad4ac200085314dfb/src/game.rs#L121

Thoughts on fix

I looked a tiny bit into how we could fix that, and I guess we could avoid sending a Drag when we detect a DragEnd ?
Around https://github.com/Vrixyz/bevy_mod_picking/blob/1471a9f83435f3fdd43829b90391e6ab965e0e73/crates/bevy_picking_core/src/events.rs#L417-L418

Workaround

As a workaround, in user code I fire an event when a DragEnd is detected, so its logic is computed after last Drag inputs.

Code Screenshot

Screenshot 2024-01-01 at 21 33 39

@aevyrie
Copy link
Owner

aevyrie commented Mar 3, 2024

This is caused by bevy's lack of inter-frame event ordering. There is no way to know if a mouse up came before or after a mouse move, or a mouse down before or after a mouse move within the same frame. Additionally, all the events are split into independent events. Once the bevy issue is fixed, the plugin will also need to switch to a unified pointer event stream.

github-merge-queue bot pushed a commit to bevyengine/bevy that referenced this issue Sep 4, 2024
# Objective

Correctly order picking events. Resolves
#5984.

## Solution

Event ordering [very long standing
problem](aevyrie/bevy_mod_picking#294) with
mod picking, stemming from two related issues. The first problem was
that `Pointer<T>` events of different types couldn't be ordered, but we
have already gotten around that in the upstream by switching to
observers. Since observers run in the order they are triggered, this
isn't an issue.

The second problem was that the underlying event streams that picking
uses to create it's pointer interaction events *also* lacked ordering,
and the systems that generated the points couldn't interleave events.
This PR fixes that by unifying the event streams and integrating the
various interaction systems.

The concrete changes are as follows:
+ `bevy_winit::WinitEvent` has been moved to `bevy_window::WindowEvent`.
This provides a unified (and more importantly, *ordered*) input stream
for both `bevy_window` and `bevy_input` events.
+ Replaces `InputMove` and `InputPress` with `PointerInput`, a new
unified input event which drives picking and interaction. This event is
built to have drop-in forward compatibility with [winit's upcoming
pointer abstraction](rust-windowing/winit#3876).
I have added code to emulate it using the current winit input
abstractions, but this entire thing will be much more robust when it
lands.
+ Rolls `pointer_events` `send_click_and_drag_events` and
`send_drag_over_events` into a single system, which operates directly on
`PointerEvent` and triggers observers as output.

The PR also improves docs and takes the opportunity to
refactor/streamline the pointer event dispatch logic.

## Status & Testing

This PR is now feature complete and documented. While it is
theoretically possible to add unit tests for the ordering, building the
picking mocking for that will take a little while.

Feedback on the chosen ordering of events is within-scope.

## Migration Guide

For users switching from `bevy_mod_picking` to `bevy_picking`:
+ Instead of adding an `On<T>` component, use `.observe(|trigger:
Trigger<T>|)`. You may now apply multiple handlers to the same entity
using this command.
+ Pointer interaction events now have semi-deterministic ordering which
(more or less) aligns with the order of the raw input stream. Consult
the docs on `bevy_picking::event::pointer_events` for current
information. You may need to adjust your event handling logic
accordingly.
+ `PointerCancel` has been replaced with `Pointer<Cancled>`, which now
has the semantics of an OS touch pointer cancel event.
+ `InputMove` and `InputPress` have been merged into `PointerInput`. The
use remains exactly the same.
+ Picking interaction events are now only accessible through observers,
and no `EventReader`. This functionality may be re-implemented later.

For users of `bevy_winit`:
+ The event `bevy_winit::WinitEvent` has moved to
`bevy_window::WindowEvent`. If this was the only thing you depended on
`bevy_winit` for, you should switch your dependency to `bevy_window`.
+ `bevy_window` now depends on `bevy_input`. The dependencies of
`bevy_input` are a subset of the existing dependencies for `bevy_window`
so this should be non-breaking.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants