[Merged by Bors] - Disable UI node `Interaction` when `ComputedVisibility` is false by alice-i-cecile · Pull Request #5361 · bevyengine/bevy

alice-i-cecile · 2022-07-18T04:10:47Z

Objective

UI nodes can be hidden by setting their Visibility property. Since #5310 was merged, this is now ergonomic to use, as visibility is now inherited.

However, UI nodes still receive (and store) interactions when hidden, resulting in surprising hidden state (and an inability to otherwise disable UI nodes.

Solution

Fixes #5360.

I've updated the ui_focus_system to accomplish this in a minimally intrusive way, and updated the docs to match.

NOTE: I have not added automated tests to verify this behavior, as we do not currently have a good testing paradigm for bevy_ui. I'm not thrilled with that by any means, but I'm not sure fixing it is within scope.

Paths not taken

Separate `Disabled` component

This is a much larger and more controversial change, and not well-scoped to UI.
Furthermore, it is extremely rare that you want hidden UI elements to function: the most common cases are for things like changing tabs, collapsing elements or so on.
Splitting this behavior would be more complex, and substantially violate user expectations.

A separate limbo world

Mentioned in the linked issue. Super cool, but all of the problems of the Disabled component solution with a whole new RFC-worth of complexity.

Using change detection to reduce the amount of redundant work

Adds a lot of complexity for questionable performance gains. Likely involves a complete refactor of the entire system.

We simply don't have the tests or benchmarks here to justify this.

Changelog

UI nodes are now always in an Interaction::None state while they are hidden (via the ComputedVisibility component).

alice-i-cecile · 2022-07-18T04:11:50Z

@undersquire, I'd love to have your review (and ideally hands-on-testing) for this PR :)

crates/bevy_ui/src/focus.rs

undersquire · 2022-07-18T05:05:04Z

Working perfectly on my end, especially now that Visibility affects children too. Not sure if there are any other tests that can be done.

Open! is sent when I click my Inventory button, and shows the Close button. Close! is sent when I click the Close button, and hides itself.

crates/bevy_ui/src/focus.rs

superdump · 2022-07-18T05:37:29Z

crates/bevy_ui/src/focus.rs

+                        if let Some(mut interaction) = interaction {
+                            if *interaction != Interaction::None {
+                                *interaction = Interaction::None;
+                            }
+                        }
+
+                        return None;


This would be simpler and skipping the nested branching to just assign Interaction::None, as it is small data-wise, should likely be faster:

Suggested change

if let Some(mut interaction) = interaction {

if *interaction != Interaction::None {

*interaction = Interaction::None;

}

}

return None;

interaction.as_mut().map(|interaction| *interaction = Interaction::None);

return None;

EDIT: Oh, it's an Option, OK. Modified suggestion above.

I'm not sure this has the right change detection behavior: that's why this code is so convoluted. I don't want this triggering Interaction change detection every update.

Co-authored-by: Robert Swain <robert.swain@gmail.com>

crates/bevy_ui/src/focus.rs

Co-authored-by: Federico Rinaldi <gisquerin@gmail.com>

Nilirad

LGTM. I consider @undersquire's experience proof that this PR works.

alice-i-cecile · 2022-07-18T14:26:53Z

Adding to the milestone now that we have approvals; I'd like to enable more serious experiments with bevy_ui during 0.8 and this is an important building block.

mockersf · 2022-07-18T15:19:57Z

As this just change the focus, setting an entity as not visible doesn't remove it from the layout computation.

The proper way to hide a UI node without despawning the entity is to set it's Style component display field to Display::None. https://docs.rs/bevy/latest/bevy/ui/struct.Style.html#structfield.display

alice-i-cecile · 2022-07-18T16:24:53Z

Does that propagate downwards correctly?

I really dislike that this is the way to control whether or not to hide UI nodes (monolithic components are bad and the discoverability is terrible), but that's a debate for another issue.

sixfold-origami · 2022-07-18T16:29:06Z

I'm mostly in agreement with Alice here. I think the core problem is that there are multiple ways to remove elements, when there really should be only one. I think the right path forward is to close this PR, then open a new PR that unifies all of these things under the same API/field (disabling visible display, removing from the layout, and disabling interaction).

mockersf · 2022-07-18T16:33:09Z

Yes, I think this should be considered as a bug in Bevy, as the visibility will not act as expected.
We should align visibility and display to only one value. display is closer to html/css and the rest of the ui properties, visibility to everything else in Bevy. Do we want to keep both and sync them, or only one?

alice-i-cecile · 2022-07-18T16:36:09Z

Strongly feel that we should only have one (and that it should be visibility). Let me make an issue.

superdump · 2022-07-18T17:58:33Z

For what it’s worth, I feel like the consistency of using Visibility for UI as well is the better choice.

mockersf · 2022-07-18T18:10:14Z

from discussion in #5368 and looking how it works on the web, it actually makes sense to keep visibility and display separated. making an item invisible but keeping it in layout is actually useful to ensure layout doesn't change when showing/hiding elements

alice-i-cecile · 2022-07-18T18:43:08Z

@mockersf @Nilirad I've added some additional docs in my last commit to help make this distinction a little more clear :)

crates/bevy_ui/src/focus.rs

crates/bevy_ui/src/ui_node.rs

Co-authored-by: Federico Rinaldi <gisquerin@gmail.com>

Nilirad

👍🏻

Nilirad · 2022-07-19T14:13:02Z

I really would like to see an example that shows the difference between ComputedVisibility::is_visible and Display::None. I'll write an issue for that ASAP.

EDIT: Done: #5380

undersquire · 2022-07-19T17:22:44Z

I can try to test my menus using the Display::None and show the results here (comparing to the Visibility implementation). Should I use this PR branch or Bevy 0.7?

alice-i-cecile · 2022-07-19T17:49:49Z

Use this PR branch :) Ideally we'll turn that into a proper UI example down the line.

undersquire · 2022-07-19T20:31:52Z

I can confirm that I get the same effect by modifying the Style component's Display field. I am unsure what else changes compared to using the Visibility component.

alice-i-cecile · 2022-07-20T21:26:31Z

[5:25 PM] cart: Feels like an easy win worth squezing in

bors r+

# Objective UI nodes can be hidden by setting their `Visibility` property. Since #5310 was merged, this is now ergonomic to use, as visibility is now inherited. However, UI nodes still receive (and store) interactions when hidden, resulting in surprising hidden state (and an inability to otherwise disable UI nodes. ## Solution Fixes #5360. I've updated the `ui_focus_system` to accomplish this in a minimally intrusive way, and updated the docs to match. **NOTE:** I have not added automated tests to verify this behavior, as we do not currently have a good testing paradigm for `bevy_ui`. I'm not thrilled with that by any means, but I'm not sure fixing it is within scope. ## Paths not taken ### Separate `Disabled` component This is a much larger and more controversial change, and not well-scoped to UI. Furthermore, it is extremely rare that you want hidden UI elements to function: the most common cases are for things like changing tabs, collapsing elements or so on. Splitting this behavior would be more complex, and substantially violate user expectations. ### A separate limbo world Mentioned in the linked issue. Super cool, but all of the problems of the `Disabled` component solution with a whole new RFC-worth of complexity. ### Using change detection to reduce the amount of redundant work Adds a lot of complexity for questionable performance gains. Likely involves a complete refactor of the entire system. We simply don't have the tests or benchmarks here to justify this. ## Changelog - UI nodes are now always in an `Interaction::None` state while they are hidden (via the `ComputedVisibility` component).

bors · 2022-07-20T21:39:45Z

Pull request successfully merged into main.

Build succeeded:

…yengine#5361) # Objective UI nodes can be hidden by setting their `Visibility` property. Since bevyengine#5310 was merged, this is now ergonomic to use, as visibility is now inherited. However, UI nodes still receive (and store) interactions when hidden, resulting in surprising hidden state (and an inability to otherwise disable UI nodes. ## Solution Fixes bevyengine#5360. I've updated the `ui_focus_system` to accomplish this in a minimally intrusive way, and updated the docs to match. **NOTE:** I have not added automated tests to verify this behavior, as we do not currently have a good testing paradigm for `bevy_ui`. I'm not thrilled with that by any means, but I'm not sure fixing it is within scope. ## Paths not taken ### Separate `Disabled` component This is a much larger and more controversial change, and not well-scoped to UI. Furthermore, it is extremely rare that you want hidden UI elements to function: the most common cases are for things like changing tabs, collapsing elements or so on. Splitting this behavior would be more complex, and substantially violate user expectations. ### A separate limbo world Mentioned in the linked issue. Super cool, but all of the problems of the `Disabled` component solution with a whole new RFC-worth of complexity. ### Using change detection to reduce the amount of redundant work Adds a lot of complexity for questionable performance gains. Likely involves a complete refactor of the entire system. We simply don't have the tests or benchmarks here to justify this. ## Changelog - UI nodes are now always in an `Interaction::None` state while they are hidden (via the `ComputedVisibility` component).

alice-i-cecile added 2 commits July 17, 2022 23:58

Disable interaction when ComputedVisibility is false

99ebf4f

Guard against change detection false positives

03fc44e

alice-i-cecile requested review from IceSentry, aevyrie and sixfold-origami July 18, 2022 04:11

alice-i-cecile added C-Bug An unexpected or incorrect behavior A-UI Graphical user interfaces, styles, layouts, and widgets C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Jul 18, 2022

alice-i-cecile commented Jul 18, 2022

View reviewed changes

crates/bevy_ui/src/focus.rs Outdated Show resolved Hide resolved

Fix missing backtick

b36b5bb

alice-i-cecile requested review from inodentry and mockersf July 18, 2022 04:18

superdump requested changes Jul 18, 2022

View reviewed changes

Typo fix

72b1675

Co-authored-by: Robert Swain <robert.swain@gmail.com>

tim-blackbird approved these changes Jul 18, 2022

View reviewed changes

Nilirad reviewed Jul 18, 2022

View reviewed changes

crates/bevy_ui/src/focus.rs Outdated Show resolved Hide resolved

crates/bevy_ui/src/focus.rs Show resolved Hide resolved

alice-i-cecile and others added 2 commits July 18, 2022 08:54

Typo fix

99483d9

Co-authored-by: Federico Rinaldi <gisquerin@gmail.com>

Comment weird-looking if check

22b8e58

Nilirad approved these changes Jul 18, 2022

View reviewed changes

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jul 18, 2022

alice-i-cecile added this to the Bevy 0.8 milestone Jul 18, 2022

mockersf removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jul 18, 2022

alice-i-cecile removed this from the Bevy 0.8 milestone Jul 18, 2022

alice-i-cecile mentioned this pull request Jul 18, 2022

Setting the visibility of UI nodes is confusing and redundant #5368

Open

alice-i-cecile marked this pull request as draft July 18, 2022 16:56

mockersf approved these changes Jul 18, 2022

View reviewed changes

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jul 18, 2022

alice-i-cecile marked this pull request as ready for review July 18, 2022 18:35

Improve docs to note two distinct ways to hide elements

333c5e6

alice-i-cecile added this to the Bevy 0.8 milestone Jul 18, 2022

Nilirad reviewed Jul 19, 2022

View reviewed changes

crates/bevy_ui/src/focus.rs Outdated Show resolved Hide resolved

crates/bevy_ui/src/ui_node.rs Outdated Show resolved Hide resolved

crates/bevy_ui/src/ui_node.rs Outdated Show resolved Hide resolved

crates/bevy_ui/src/ui_node.rs Outdated Show resolved Hide resolved

Doc polish

776f22a

Co-authored-by: Federico Rinaldi <gisquerin@gmail.com>

Nilirad approved these changes Jul 19, 2022

View reviewed changes

Nilirad mentioned this pull request Jul 19, 2022

Create a UI example demonstrating the difference between node Visibility::is_visible and Display::None #5380

Closed

bors bot changed the title ~~Disable UI node Interaction when ComputedVisibility is false~~ [Merged by Bors] - Disable UI node Interaction when ComputedVisibility is false Jul 20, 2022

bors bot closed this Jul 20, 2022

Uh oh!

Conversation

alice-i-cecile commented Jul 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Paths not taken

Separate Disabled component

A separate limbo world

Using change detection to reduce the amount of redundant work

Changelog

Uh oh!

alice-i-cecile commented Jul 18, 2022

Uh oh!

Uh oh!

undersquire commented Jul 18, 2022

Uh oh!

Uh oh!

superdump Jul 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile Jul 18, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Nilirad left a comment

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile commented Jul 18, 2022

Uh oh!

mockersf commented Jul 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alice-i-cecile commented Jul 18, 2022

Uh oh!

sixfold-origami commented Jul 18, 2022

Uh oh!

mockersf commented Jul 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alice-i-cecile commented Jul 18, 2022

Uh oh!

superdump commented Jul 18, 2022

Uh oh!

mockersf commented Jul 18, 2022

Uh oh!

alice-i-cecile commented Jul 18, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Nilirad left a comment

Choose a reason for hiding this comment

Uh oh!

Nilirad commented Jul 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

undersquire commented Jul 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alice-i-cecile commented Jul 19, 2022

Uh oh!

undersquire commented Jul 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alice-i-cecile commented Jul 20, 2022

Uh oh!

bors bot commented Jul 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

alice-i-cecile commented Jul 18, 2022 •

edited

Loading

Separate `Disabled` component

superdump Jul 18, 2022 •

edited

Loading

mockersf commented Jul 18, 2022 •

edited

Loading

mockersf commented Jul 18, 2022 •

edited

Loading

Nilirad commented Jul 19, 2022 •

edited

Loading

undersquire commented Jul 19, 2022 •

edited

Loading

undersquire commented Jul 19, 2022 •

edited

Loading