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 event ordering #14862

Merged
merged 28 commits into from
Sep 4, 2024
Merged

Conversation

NthTensor
Copy link
Contributor

@NthTensor NthTensor commented Aug 22, 2024

Objective

Correctly order picking events. Resolves #5984.

Solution

Event ordering very long standing problem 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. 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.

@NthTensor NthTensor added A-Input Player input via keyboard, mouse, gamepad, and more A-Windowing Platform-agnostic interface layer to run your app in A-Picking Pointing at and selecting objects of all sorts C-Bug An unexpected or incorrect behavior S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 22, 2024
Copy link
Contributor

@tychedelia tychedelia left a comment

Choose a reason for hiding this comment

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

It's a little hard to review in combination with the upstream pr, as they're getting rid of most of these events. In that way, it might be confusing for our users if they can read a CursorEntered event from WindowEvent but then also a PointerEntered derived from that same event. Upstreaming picking is essential so moving it forward is probably worth the cost, but it would be nice to know when their changes will land, so hopefully it doesn't need to be visible to users. It would make me nervous to do a release and churn on those changes.

Substantively, the unification strategy makes a lot of sense, and code looks good so far, with the caveat that I'm not familiar with picking internals.

crates/bevy_winit/src/state.rs Outdated Show resolved Hide resolved
crates/bevy_picking/src/pointer.rs Show resolved Hide resolved
@NthTensor NthTensor marked this pull request as ready for review August 30, 2024 18:42
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Aug 30, 2024
@BD103 BD103 added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Aug 30, 2024
Copy link
Contributor

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

@NthTensor NthTensor mentioned this pull request Aug 31, 2024
16 tasks
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 agree with the core architectural choices, and this is looking good.

I've left a few small notes on docs, and InputPlugin needs to be renamed :) Once that's done I'm good to merge this.

@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 Sep 4, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 4, 2024
Merged via the queue into bevyengine:main with commit 82128d7 Sep 4, 2024
31 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-Windowing Platform-agnostic interface layer to run your app in C-Bug An unexpected or incorrect behavior M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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.

Input event order information is lost
4 participants