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

Re-implemented event system #43

Merged
merged 7 commits into from
Dec 27, 2021
Merged

Conversation

MrGVSV
Copy link
Collaborator

@MrGVSV MrGVSV commented Dec 27, 2021

Objective

The current event system is limited in that it:

  • Does not properly target nodes
  • Does not support event propagation

For example, in the following tree:

+-- Window
|  +-- Element
|  |  +-- Button

Hovering over the Button will also count as hovering over the Element and Window.

We also can't just filter by z-index or tree depth alone, since events like MouseIn should apply over all appropriate nodes in the tree (whether stacked atop each other or not).

Solution

With this PR, we now find the target node rather than sending the event to every node along the way to the actual target. Doing this allows us to better control which nodes receive the event. It also allows us to properly propagate the event up the tree and stop propagation when requested.

Issues

In order to properly find the target of a mouse event, we need to look through every node. This is due to issues regarding self-directed and overflowing elements. This prevents us from recursively narrowing down on the target widget, since we don't know if the parent actually contains it or not.

I haven't tested it on a huge tree, so it might not be a huge issue, but it's not great. If this is too big of an issue, then maybe a different method could be considered. One fix could be to revert back to the previous system and combine it with the functionality of WidgetManager::get_nodes_under. Doing that should allow us to do everything in a single iteration of the node tree (hopefully?).

Marking this as a draft for now until I can take a closer look at this potential fix.


Edit: After trying out the idea of performing all checks within a single tree iteration, I managed to get it working but found one small issue. Bevy might send multiple CursorMoved events in a single frame. If we were to process each one, we'd have to add some complicated logic to switch between active current_mouse_position values (which, in any case, may be incorrect for other non-CursorMoved events on the same widget anyways due to event ordering).

Therefore, I opted to skip all but the last CursorMoved event. This means we'll miss some for really fast cursor movements, but I think it should generally be okay. Let me know if this is not an ideal solution, though!

kayak_core/src/widget_manager.rs Outdated Show resolved Hide resolved
kayak_core/src/widget_manager.rs Outdated Show resolved Hide resolved
Comment on lines +272 to +289
/// Events are processed in three phases: Capture, Target, Propagate. These phases are based on their
/// associated [W3 specifications](https://www.w3.org/TR/uievents/#dom-event-architecture).
///
/// ## Capture:
/// Currently, we do not support the Capture Phase. This is because the current event handling system is
/// made to handle events as a single enum. To achieve proper capturing, widgets would need to be able to
/// register separate event handlers so that specific ones could be captured while others would not. It
/// should generally be okay to skip this as it's not a common use-case.
///
/// ## Target:
/// The Target Phase simply identifies the target for an event so that we can generate the propagation path
/// for it.
///
/// ## Propagate:
/// The Propagate Phase (also known as the Bubble Phase) is where we bubble up the tree from the target node,
/// firing the bubbled event along the way. At any point, the bubbling can be stopped by calling
/// [`event.stop_propagation()`](Event::stop_propagation). Not every event can be propagated, in which case,
/// they will only fire for their specified target.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can remove this from the documentation and just put it as a separate comment if that is preferred.

@MrGVSV
Copy link
Collaborator Author

MrGVSV commented Dec 27, 2021

The latest change now moves all of the event management to a separate EventDispatcher struct on KayakContext. This was mainly a code organization thing. But doing this also makes it much easier to control what event data is shared with the context (and hopefully avoid confusion on the purpose of certain fields when contributing to KayakContext).

If this is too big a change or something that isn't wanted, let me know, and I can revert it!

@MrGVSV MrGVSV marked this pull request as ready for review December 27, 2021 10:24
Copy link
Owner

@StarArawn StarArawn left a comment

Choose a reason for hiding this comment

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

This cleans things up quite a bit! I think I like everything except the part about searching every node. :( Can we add an issue and perhaps in the future we can fix that? I'm fine with merging this as is for now though.

@MrGVSV
Copy link
Collaborator Author

MrGVSV commented Dec 27, 2021

This cleans things up quite a bit! I think I like everything except the part about searching every node. :( Can we add an issue and perhaps in the future we can fix that? I'm fine with merging this as is for now though.

Thanks! Yeah I’m not a fan either. Thankfully I did manage to get it down from searching every node on every event, to just a single search, like we had before with down_iter. But I think we can do better if we can figure out a clever way of determining which children might contain a point for overflowing/self-directed nodes. But yeah, that can be figured out later haha, so I'll just open an issue about it for now.

@StarArawn StarArawn merged commit f2bb5c4 into StarArawn:main Dec 27, 2021
@MrGVSV MrGVSV deleted the event-system branch December 27, 2021 18:19
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

Successfully merging this pull request may close these issues.

2 participants