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

Remove Interaction, and replace it with bevy_picking based fields on Button #15597

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

alice-i-cecile
Copy link
Member

@alice-i-cecile alice-i-cecile commented Oct 2, 2024

Objective

Fixes #15550. See that issue and the linked issues within it about why the existing Interaction component is inadequate.

Solution

  • remove Interaction and all engine systems that use it
  • add pressed and hovered fields to Button
  • updated these fields using observers powered by bevy_picking
  • migrate all existing examples
  • figure out what's going wrong with hovering in the button example
    • pointer presses are getting propagated correctly, but pointer hovering seems to be blocked by Text children
  • make Text entities not pickable by default
  • swap to HoverMap based detection for hovering to avoid blocking by children
  • remove FocusPolicy in favor of Pickable
  • remove RelativeCursorPosition. Widgets should compute this information themselves based on bevy_picking input.
  • fix slider widgets in the animation_graph example
  • consider how to improve the migration path for the existing dynamic-styling pattern

Note to reviewers: the previous solution has special-cased logic for hidden nodes to ensure that they can't be pressed. I would prefer not to implement that in this PR, as that's fundamentally a picking backend concern.

Testing

For basic functionality, run the button example.

For something more holistic, try the game_menu example.

Migration Guide

The Interaction component has been removed. Instead, use the new pressed and hovered fields on Button. Bear in mind that you can also use observers to listen for bevy_picking events such as Pointer<Down> and Pointer<Over> directly as well.

To migrate code that matches on Interaction to change some property, follow the example below:

Bevy 0.14:

outline.color = match *button {
    Interaction::Pressed => RED.into(),
    Interaction::Hovered => WHITE.into(),
    Interaction::None => Color::NONE,
};

Bevy 0.15:

outline.color = match (button.pressed, button.hovered) {
    (true, _) => RED.into(),
    (false, true) => WHITE.into(),
    (false, false) => Color::NONE,
};

NodeBundle (and their Bevy 0.15 equivalents) no longer track whether or not they are pressed / hovered by default. If you require this functionality, add the Button component to your entity.

The FocusPolicy component has been removed, and replaced with the universal Pickable component.

The RelativeCursorPosition component has been removed. If you need this information, you should compute it from bevy_picking events in your widget's logic. See the animation_graph example for a demonstration of how this can be done.

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-UI Graphical user interfaces, styles, layouts, and widgets X-Controversial There is active debate or serious implications around merging this PR M-Needs-Release-Note Work that should be called out in the blog due to impact S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged A-Picking Pointing at and selecting objects of all sorts labels Oct 2, 2024
@alice-i-cecile alice-i-cecile changed the title Remove Interaction, and replace it with a bevy_picking based alternative Remove Interaction, and replace it with bevy_picking based fields on Button Oct 2, 2024
@alice-i-cecile alice-i-cecile added S-Needs-Help The author needs help finishing this PR. S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed M-Needs-Release-Note Work that should be called out in the blog due to impact S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged S-Needs-Help The author needs help finishing this PR. labels Oct 2, 2024
@Leinnan
Copy link
Contributor

Leinnan commented Oct 2, 2024

From game maker perspective- how to most easily get the "is this button just released" info?

@alice-i-cecile alice-i-cecile added X-Contentious There are nontrivial implications that should be thought through and removed X-Controversial There is active debate or serious implications around merging this PR labels Oct 2, 2024
@alice-i-cecile
Copy link
Member Author

From game maker perspective- how to most easily get the "is this button just released" info?

I think that the best strategy is to use an observer listening to Pointer<Up> event, and check if it matches the entities you care about. Can you say more about why you want this information? Things like dragging should use the specialized bevy_picking APIs for that purpose.

@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Help The author needs help finishing this PR. and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged C-Feature A new feature, making something new possible labels Oct 2, 2024
@alice-i-cecile alice-i-cecile removed the S-Needs-Help The author needs help finishing this PR. label Oct 2, 2024
Copy link
Contributor

@MiniaczQ MiniaczQ left a comment

Choose a reason for hiding this comment

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

I like the idea in general,
not super sure about the separation (button has pressed & hovered) / naming (button) here.
There can be label widgets that get highlighted when hovered over, does that make them a button?
If not, should hovered be a separate component or should button have a better name

@alice-i-cecile
Copy link
Member Author

There can be label widgets that get highlighted when hovered over, does that make them a button?

The idea from @NthTensor is that each widget type should track its own interaction state independently, as its rules may differ. I agree with this design. I would have showcased it here, but uh, we only have one widget type 😅

@alice-i-cecile alice-i-cecile marked this pull request as draft October 2, 2024 19:42
@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 2, 2024
@alice-i-cecile alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Oct 3, 2024
@alice-i-cecile alice-i-cecile marked this pull request as ready for review October 3, 2024 16:00
@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 Oct 3, 2024
@alice-i-cecile alice-i-cecile added X-Controversial There is active debate or serious implications around merging this PR S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Contentious There are nontrivial implications that should be thought through labels Oct 3, 2024
Copy link
Contributor

github-actions bot commented Oct 3, 2024

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

Comment on lines +274 to +277
pickable: Pickable {
should_block_lower: true,
..Default::default()
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pickable: Pickable {
should_block_lower: true,
..Default::default()
},
pickable: Pickable::default(),

should_block_lower is true by default

Copy link
Member Author

Choose a reason for hiding this comment

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

Great; that means we can swap to a pure derive then.

@NthTensor
Copy link
Contributor

Pickable::default() is basically a no-op. We should discourage users from adding it imo, or make Pickable required by Node.

Comment on lines +63 to +71
let mut hovering_over_descendant = false;

for descendant in children_query.iter_descendants(entity) {
if map.contains_key(&descendant) {
hovering_over_descendant = true;
break;
}
}
hovering_over_descendant
Copy link
Member

@mnmaita mnmaita Oct 6, 2024

Choose a reason for hiding this comment

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

Isn't it possible to use any() with DescendantsIter?

@alice-i-cecile alice-i-cecile modified the milestones: 0.15, 0.16 Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Picking Pointing at and selecting objects of all sorts A-UI Graphical user interfaces, styles, layouts, and widgets C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove bevy_ui::Interaction in favor of a bevy_picking based solution
6 participants