-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Interaction component refactor #7257
Conversation
Co-authored-by: Mike <mike.hsu@gmail.com>
crates/bevy_ui/src/focus.rs
Outdated
@@ -53,6 +53,27 @@ impl Default for Interaction { | |||
} | |||
} | |||
|
|||
/// Describes whether [`Interaction`] component should remain in the `Clicked` state after |
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.
/// Describes whether [`Interaction`] component should remain in the `Clicked` state after | |
/// Describes whether [`Interaction`] component should remain in the [`Pressed`](Interaction::Pressed) state after |
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.
The Interaction
state is called Clicked
though. Should I change the state name to Pressed
or keep the comment saying Clicked
and link the Interaction::Clicked
?
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.
Yes, I think we should rename it to (#5769)Interaction::Pressed
so that it makes more sense when we eventually start supporting Interaction::Released
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.
Or, hmm. On second thought no.
Lets keep the name as Interaction::Clicked
and do the rename in #7240 so that those two changes are kept together. Will be easier to review that way.
You added a new example but didn't update the readme. Please run |
You added a new example but didn't update the readme. Please run |
You added a new example but didn't update the readme. Please run |
How do I fix the |
Hey @Weibye, I see you self-requested a review quite some time ago. Are you still up to it? If you aren't, it's obviously alright, I'm just checking in case you forgot. |
@alice-i-cecile asked the discord for feedback on this PR. I have not used There are a lot of things to think about with interaction if you want what I would consider a production-grade solution. These features can be implemented in more than one way, but I describe them from the point of view of the system I built (which does not use the mainstream approach of event bubbling and pointer capture). Mandatory features
Nice-to-have featuresThese are useful features that can either be implemented on top of the mandatory stuff or aren't strictly necessary.
|
Talking it over some more, I don't think that this is the right next step. Like both @UkoeHB and @aevyrie (on Discord) have said, I think there's a lot of subtle complexity here and I think we need to take a step back and do some serious design rather than incrementally trying to fix the very real problems we have. I'm going to close this out, but if we (or you!) do that design work (in a Github Discussion or RFC) and the solution ends up looking like this, please feel free to reopen or remake this PR. |
This PR's original objective was only to fix #7239. The original solution was to introduce a new
InteractionPolicy
component that would regulate theInteraction
component behaviour.Objective
Decouples the
Hovered
and theClicked
state of theInteraction
component. Also fixes #7239.Solution
Interaction
component has been removedInteraction
's purpose is now fulfilled by thePressed
and theRelativeCursorPosition
componentsPressed
component contains a bool value saying whether the node is currently pressed. It also contains apressed_policy
field that regulates the behaviour of the component when the cursor leaves the node bounds without letting go of the mouse button (see Ui Button is still considered pressed when holding down, but moving cursor out of the button Rect #7239)RelativeCursorPosition
component is already available in Bevy. It replaces theHovered
state with itsmouse_over
methodInteraction
component, I introduced anInteractionState
enum, which is not a component, but contains the same variants as theInteraction
component. To get the node'sInteractionState
the user can callinteraction_state
method on(&Pressed, &RelativeCursorPosition)
.InteractionStateChangedFilter
, which looks for changes in bothPressed
andRelativeCursorPosition
components, although it's not as performant as the previousChanged<Interaction>
, because it also fires then the cursor movesChangelog
Pressed
componentPressPolicy
struct used inside thePressed
componentPressPolicy
worksInteractionState
enumInteractionStateHandler
trait implemented for(&Pressed, &RelativeCursorPosition)
ButtonBundle
replacing theInteraction
component withPressed
andRelativeCursorPosition
componentsInteraction
componentInteraction
componentMigration guide
Interaction
, you will now have to query forPressed
andRelativeCursorPosition
and call theinteraction_state
method on(&Pressed, &RelativeCursorPosition)
Clicked
state fromInteraction
you can query only for thePressed
componentHovered
state fromInteraction
you can query only for theRelativeCursorPosition
component and call itsmouse_over
method