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.

Sign up for GitHub

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

Jump to bottom

Remove ZIndex::Global #9877

Closed
wants to merge 7 commits into from
Closed

Remove ZIndex::Global #9877

wants to merge 7 commits into from

Conversation

ickshonpe
Copy link
Contributor

Objective

Remove ZIndex::Global

Fixes #9875

Solution

Change the ZIndex enum into a tuple struct wrapping an i32. This value represents the local z index, replacing what was ZIndex::Local.

ui_stack_system updates UiStack in a single walk of the layout tree.

ui_stack_system benchmark (Yellow is this PR):

cargo run --release --features trace_tracy --example many_buttons -- --no-text --no-borders
stack_system_walk

Changelog

  • ZIndex::Global is removed.
  • ZIndex is now a tuple struct wrapping an i32. This value represents the local z index, replacing what was ZIndex::Local.
    ui_stack_system updates UiStack in a single walk of the layout tree.

Migration Guide

ZIndex is now an 'i32tuple struct. This value represents the local z index, replacing what wasZIndex::Local. ZIndex::Global` has been removed.

@ickshonpe
* ZIndex::Global is removed.
2c7cf32 
* `ZIndex` is now a tuple struct wrapping an `i32`. This value represents the local z index, replacing what was `ZIndex::Local`.
* `ui_stack_system` does its update during a walk of the layout tree .
@ickshonpe
Fixed examples
9df08e8
@ickshonpe
Derive default for ZIndex.
07274d4 
Fixed formatting.
@ickshonpe
fixed semicolon-if-nothing-returned
dc1c762
@ickshonpe
Merge branch 'main' into remove-zindex-global
52bc28b
@james7132
Copy link
Member

This partially reverts #5877. Does #9637 address the issues @oceantume was running into so that this PR doesn't become a regression?

@oceantume
Copy link
Contributor

oceantume commented Sep 20, 2023 •
edited
Loading

This partially reverts #5877. Does #9637 address the issues @oceantume was running into so that this PR doesn't become a regression?

I went over the PR text quickly and didn't see a mention for explicitly defining the order in which root node render relative to each other. Since ECS doesn't guarantee ordering for nodes that aren't hierarchically related, wouldn't that still be an issue?

Sorry, I was a bit confused. Local zindex would still be possible so the above is not a problem. I would make sure that in the new system, local z-index still works for ordering root nodes against each other, but otherwise it should be fine on that side.

Regardless of above, this is a regression in the sense that global zindex allows a node anywhere in a tree to "escape" the tree and render above everything else in the app for example.

Whether this really is a desired feature is debatable though. On simple UIs it's generally not needed, but on the Web, this is often used for rendering deeply nested dropdowns and modals on top of everything else without having to add them to the root (body), and keeping the hierarchy which has effects other than just positioning and ordering (e.g. style inheritence, selectors, etc)

@james7132 james7132 added C-Performance A change motivated by improving speed, memory usage or compile times A-UI Graphical user interfaces, styles, layouts, and widgets X-Controversial There is active debate or serious implications around merging this PR labels Sep 20, 2023
@ickshonpe
Copy link
Contributor Author

Local zindex can still be used to sort the root nodes.

Regardless of above, this is a regression in the sense that global zindex allows a node anywhere in a tree to "escape" the tree and render above everything else in the app for example.

Whether this really is a desired feature is debatable though. On simple UIs it's generally not needed, but on the Web, this is often used for rendering deeply nested dropdowns and modals on top of everything else without having to add them to the root (body), and keeping the hierarchy which has effects other than just positioning and ordering (e.g. style inheritence, selectors, etc)

I hadn't really considered style inheritance. I can see there might be some advantages to keeping the hierarchy, though I'm not really sure. There's no need to rush into this I guess anyway.

@ickshonpe
Copy link
Contributor Author

Closed in favour of #9889

@ickshonpe ickshonpe closed this Sep 21, 2023
@ickshonpe ickshonpe mentioned this pull request Sep 21, 2023
Merged
@ickshonpe ickshonpe mentioned this pull request Jan 1, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Sep 30, 2024
@ickshonpe @oceantume
@alice-i-cecile @UkoeHB
Simplified ui_stack_system (#9889)
c5742ff 
# 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
```

<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>
robtfm pushed a commit to robtfm/bevy that referenced this pull request Oct 4, 2024
@ickshonpe @oceantume
@alice-i-cecile @UkoeHB @robtfm
Simplified ui_stack_system (bevyengine#9889)
3aca5c2 
# 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Performance A change motivated by improving speed, memory usage or compile times X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove ZIndex::Global
3 participants
@ickshonpe @james7132 @oceantume