Simplified `ui_stack_system` by ickshonpe · Pull Request #9889 · bevyengine/bevy

ickshonpe · 2023-09-21T13:52:03Z

Objective

ui_stack_system generates a tree of StackingContexts which it then flattens to get the UiStack.

But there's no need to construct a new tree. We can query for nodes with a global ZIndex, add those nodes to the root nodes list and then build the UiStack from a walk of the existing layout tree, ignoring any branches that have a global Zindex.

Fixes #9877

Solution

Split the ZIndex enum into two separate components, ZIndex and GlobalZIndex

Query for nodes with a GlobalZIndex, add those nodes to the root nodes list and then build the UiStack from a walk of the existing layout tree, filtering branches by Without<GlobalZIndex> so we don't revisit nodes.

cargo run --profile stress-test --features trace_tracy --example many_buttons

(Yellow is this PR, red is main)

Changelog

Zindex

The ZIndex enum has been split into two separate components ZIndex (which replaces ZIndex::Local) and GlobalZIndex (which replaces ZIndex::Global). An entity can have both a ZIndex and GlobalZIndex, in comparisons ZIndex breaks ties if two GlobalZIndex values are equal.

ui_stack_system

Instead of generating a tree of StackingContexts, query for nodes with a GlobalZIndex, add those nodes to the root nodes list and then build the UiStack from a walk of the existing layout tree, filtering branches by Without<GlobalZIndex so we don't revisit nodes.

Migration Guide

The ZIndex enum has been split into two separate components ZIndex (which replaces ZIndex::Local) and GlobalZIndex (which replaces ZIndex::Global). An entity can have both a ZIndex and GlobalZIndex, in comparisons ZIndex breaks ties if two GlobalZindex values are equal.

…ering away those nodes.

clean up and cargo fmt

…ZIndex` and `GlobalZIndex` respectively.

oceantume · 2023-09-21T17:02:05Z

That makes sense to me. While both are kinds of zindex, they have different implications, both conceptually and in the implementation.

I'm curious to know what maintainers think of the naming, i.e. should ZIndex be renamed to LocalZIndex so that it's in direct contrast with GlobalZIndex?

oceantume

There are a few minor missing changes I commented about, but otherwise this looks good to me. This makes ZIndex a simpler component and "hides" global z-index behavior which is probably a good thing since it's an escape hatch and isn't meant to be used in most UI's. Unlike the previous PR, this is not a feature regression so it's less controversial in my opinion (as long as the API change is deemed uncontroversial as well).

crates/bevy_ui/src/ui_node.rs

crates/bevy_ui/src/stack.rs

crates/bevy_ui/src/ui_node.rs

crates/bevy_ui/src/stack.rs

examples/ui/z_index.rs

Co-authored-by: Gabriel Bourgeois <gabriel.bourgeoisv4si@gmail.com>

crates/bevy_ui/src/stack.rs

…stem`

ickshonpe · 2023-09-23T09:55:34Z

That makes sense to me. While both are kinds of zindex, they have different implications, both conceptually and in the implementation.

I'm curious to know what maintainers think of the naming, i.e. should ZIndex be renamed to LocalZIndex so that it's in direct contrast with GlobalZIndex?

My thinking is that keeping ZIndex for the local ZIndex is lower friction. It's shorter, many users will already be familiar with the equivalent-ish CSS z-index property and those that aren't will get the z-index entry on mdn etc if they search for it.

I'm not bothered about the naming though, if there is a consensus in favour of LocalZIndex it would be fine.

…equal.

atlv24

This makes a lot of sense and uses the ECS better. Im in favor of the current naming (ZIndex + GlobalZIndex)

crates/bevy_ui/src/stack.rs

UkoeHB · 2024-09-22T20:46:08Z

crates/bevy_ui/src/stack.rs

+                        .ok()
+                        .map(|zindex| (*entity, zindex.map(|zindex| zindex.0).unwrap_or(0)))
+                })
+                .collect();


No need to collect, use a shared vec sourced from a local and clear -> extend here.

UkoeHB · 2024-09-22T20:53:03Z

crates/bevy_ui/src/stack.rs

+                        .map(|zindex| (*entity, zindex.map(|zindex| zindex.0).unwrap_or(0)))
+                })
+                .collect();
+            radsort::sort_by_key(&mut z_children, |k| k.1);


Not clear that radsort is better than std::sort here either, since most nodes will have only a handful of children. To use std::sort_unstable you can add an index for each child and sort by (z_index, child_index).

Another reviewer said to try using radsort and it seemed to be an improvement with my benchmarks but I forgot that those benchmarks were with very large numbers of children which isn't the typical usage case.

UkoeHB · 2024-09-22T21:00:54Z

crates/bevy_ui/src/stack.rs

+    for (i, entity) in ui_stack.uinodes.iter().enumerate() {
+        if let Ok(mut node) = update_query.get_mut(*entity) {
+            node.bypass_change_detection().stack_index = i as u32;
+        }
    }


The traversal_stack is never inserted to ui_stack. Am I missing something here? Also how are children supposed to be inserted relative to their parents within the global stack?

I'm puzzled how this passes CI, it appears to have serious bugs.

traversal_stack is used to navigate the tree, not insert the nodes.

Abandoned the iterative version using stack traversal anyway, and went back to walking the tree recursively, copying your stack of cached buffers approach from main to avoid the vec allocations. I don't understand why the iterative version is so much less efficient though. It just has one buffer for storing and sorting the children it reuses, which seems so much better.

UkoeHB

Looks correct now, minor doc comments. Also would like to see an updated perf graph.

crates/bevy_ui/src/stack.rs

crates/bevy_ui/src/ui_node.rs

Co-authored-by: UkoeHB <37489173+UkoeHB@users.noreply.github.com>

# Objective `ui_stack_system` generates a tree of `StackingContexts` which it then flattens to get the `UiStack`. But there's no need to construct a new tree. We can query for nodes with a global `ZIndex`, add those nodes to the root nodes list and then build the `UiStack` from a walk of the existing layout tree, ignoring any branches that have a global `Zindex`. Fixes bevyengine#9877 ## Solution Split the `ZIndex` enum into two separate components, `ZIndex` and `GlobalZIndex` Query for nodes with a `GlobalZIndex`, add those nodes to the root nodes list and then build the `UiStack` from a walk of the existing layout tree, filtering branches by `Without<GlobalZIndex>` so we don't revisit nodes. ``` cargo run --profile stress-test --features trace_tracy --example many_buttons ``` <img width="672" alt="ui-stack-system-walk-split-enum" src="https://github.com/bevyengine/bevy/assets/27962798/11e357a5-477f-4804-8ada-c4527c009421"> (Yellow is this PR, red is main) --- ## Changelog `Zindex` * The `ZIndex` enum has been split into two separate components `ZIndex` (which replaces `ZIndex::Local`) and `GlobalZIndex` (which replaces `ZIndex::Global`). An entity can have both a `ZIndex` and `GlobalZIndex`, in comparisons `ZIndex` breaks ties if two `GlobalZIndex` values are equal. `ui_stack_system` * Instead of generating a tree of `StackingContexts`, query for nodes with a `GlobalZIndex`, add those nodes to the root nodes list and then build the `UiStack` from a walk of the existing layout tree, filtering branches by `Without<GlobalZIndex` so we don't revisit nodes. ## Migration Guide The `ZIndex` enum has been split into two separate components `ZIndex` (which replaces `ZIndex::Local`) and `GlobalZIndex` (which replaces `ZIndex::Global`). An entity can have both a `ZIndex` and `GlobalZIndex`, in comparisons `ZIndex` breaks ties if two `GlobalZindex` values are equal. --------- Co-authored-by: Gabriel Bourgeois <gabriel.bourgeoisv4si@gmail.com> Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> Co-authored-by: UkoeHB <37489173+UkoeHB@users.noreply.github.com>

ickshonpe added 4 commits September 21, 2023 12:15

Query for nodes with zindex global separately then walk the tree filt…

4957851

…ering away those nodes.

fixed irrefutable pattern,

d03630b

clean up and cargo fmt

Moved queries around to see if performance difference.

35db93a

Made Zindex::Local and ZIndex::Global into separate components, `…

5c2e189

…ZIndex` and `GlobalZIndex` respectively.

ickshonpe mentioned this pull request Sep 21, 2023

Remove ZIndex::Global #9875

Closed

Fixed tests

229ad8a

ickshonpe mentioned this pull request Sep 21, 2023

Remove ZIndex::Global #9877

Closed

ickshonpe added 2 commits September 21, 2023 18:11

Added doc comments for GlobalZIndex

805cafe

Fixed ZIndex in examples

c909250

oceantume suggested changes Sep 22, 2023

View reviewed changes

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

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

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

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

examples/ui/z_index.rs Show resolved Hide resolved

ickshonpe and others added 2 commits September 22, 2023 19:06

Update crates/bevy_ui/src/ui_node.rs

549511b

Co-authored-by: Gabriel Bourgeois <gabriel.bourgeoisv4si@gmail.com>

Update crates/bevy_ui/src/ui_node.rs

6af7a3b

Co-authored-by: Gabriel Bourgeois <gabriel.bourgeoisv4si@gmail.com>

james7132 reviewed Sep 23, 2023

View reviewed changes

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

ickshonpe added 2 commits September 23, 2023 10:11

Fixed the type names in the test comments

b9dd7ec

Moved the update_stack_recursively function outside of `ui_stack_sy…

0111471

…stem`

Added a test to validate ordering by ZIndex when GlobalZIndex is …

6d71e50

…equal.

atlv24 approved these changes Sep 23, 2023

View reviewed changes

oceantume approved these changes Sep 24, 2023

View reviewed changes

ickshonpe changed the title ~~simplified ui_stack_system~~ Simplified ui_stack_system Sep 24, 2023

nicoburns added A-UI Graphical user interfaces, styles, layouts, and widgets C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Sep 26, 2023

james7132 reviewed Oct 1, 2023

View reviewed changes

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

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

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

ickshonpe mentioned this pull request Jan 1, 2024

Decouple BackgroundColor from UiImage #11165

Merged

ickshonpe added 4 commits September 9, 2024 00:08

Merge branch 'main' into ui-stack-system-walk

a769641

update stack indices

a13c2a5

use radixsort to sort the rootnodes

e93b479

use radsort to sort children

9cebb03

alice-i-cecile added this to the 0.15 milestone Sep 21, 2024

alice-i-cecile added S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 21, 2024

villor mentioned this pull request Sep 22, 2024

Add UI GhostNode #15341

Merged

21 tasks

UkoeHB suggested changes Sep 22, 2024

View reviewed changes

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-Benchmarking This set of changes needs performance benchmarking to double-check that they help labels Sep 27, 2024

alice-i-cecile removed this from the 0.15 milestone Sep 27, 2024

ickshonpe added 2 commits September 30, 2024 12:57

Merge branch 'main' into ui-stack-system-walk

8c96e19

cache root nodes

1623770

alice-i-cecile requested a review from UkoeHB September 30, 2024 12:12

alice-i-cecile 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-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Sep 30, 2024

ickshonpe added 10 commits September 30, 2024 13:16

Remove radix sort, cache z_children

9e9839a

z_children vec, sort the traversal stack instead

3df9c3a

Removed unused radsort dependency

3b57640

walk the stack recursively

92dfd81

removed unused

1290d0c

try smallvec

b0c27ac

removed traversal stack

158c9c4

Don't need to use Reverse for recursive version.

99d9a28

cache vecs used to contain the sorted children

6a4734e

allow too_many_arguments for ui_stack_system

36504a9

UkoeHB suggested changes Sep 30, 2024

View reviewed changes

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

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

Documentation fixes

a5e01e4

Co-authored-by: UkoeHB <37489173+UkoeHB@users.noreply.github.com>

alice-i-cecile enabled auto-merge September 30, 2024 18:31

alice-i-cecile added this pull request to the merge queue Sep 30, 2024

Merged via the queue into bevyengine:main with commit c5742ff Sep 30, 2024

Uh oh!

Conversation

ickshonpe commented Sep 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Changelog

Migration Guide

Uh oh!

oceantume commented Sep 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oceantume left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ickshonpe commented Sep 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atlv24 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

UkoeHB Sep 22, 2024

Choose a reason for hiding this comment

Uh oh!

UkoeHB Sep 22, 2024

Choose a reason for hiding this comment

Uh oh!

ickshonpe Sep 30, 2024

Choose a reason for hiding this comment

Uh oh!

UkoeHB Sep 22, 2024

Choose a reason for hiding this comment

Uh oh!

UkoeHB Sep 22, 2024

Choose a reason for hiding this comment

Uh oh!

ickshonpe Sep 30, 2024

Choose a reason for hiding this comment

Uh oh!

ickshonpe Sep 30, 2024 • 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!

UkoeHB left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ickshonpe commented Sep 21, 2023 •

edited

Loading

oceantume commented Sep 21, 2023 •

edited

Loading

ickshonpe commented Sep 23, 2023 •

edited

Loading

ickshonpe Sep 30, 2024 •

edited

Loading