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

Add stack index to Node #9853

Merged
merged 11 commits into from
Oct 31, 2023
Merged

Add stack index to Node #9853

merged 11 commits into from
Oct 31, 2023

Conversation

ickshonpe
Copy link
Contributor

@ickshonpe ickshonpe commented Sep 19, 2023 •
edited
Loading

Objective

If we add the stack index to Node then we don't need to walk the UiStack repeatedly during extraction.

Solution

Add a field stack_index to Node.
Update it in ui_stack_system.
Iterate queries directly in the UI's extraction systems.

Benchmarks

cargo run --profile stress-test --features trace_tracy --example many_buttons -- --no-text --no-borders

frames (yellow this PR, red main):

frames-per-second

ui_stack_system:
ui-stack-system

extract schedule:
extract-schedule

Changelog

  • Added the field stack_index to Node.
  • ui_stack_system updates Node::stack_index after a new UiStack is generated.
  • The UI's extraction functions iterate a query directly rather than walking the UiStack and doing lookups.

@ickshonpe
Added stack_index field to Node
d8b73bf 
Extraction reads the `stack_index`  from `Node` instead of walking `UiStack`
`ui_stack_system` updates `Node`'s `stack_index` after generating a new `UiStack`.
@ickshonpe
Added doc comment for stack_index field
0133fd5
@ickshonpe
cargo fmt
9f04e27
@ickshonpe
Merge branch 'main' into node-stack-index
c0594e4
@ickshonpe
removed unneeded import
d3f4cf3
@ickshonpe
changed Node::stack_index into a u32
4739f60
@alice-i-cecile alice-i-cecile added C-Performance A change motivated by improving speed, memory usage or compile times A-UI Graphical user interfaces, styles, layouts, and widgets labels Sep 19, 2023
@alice-i-cecile
Copy link
Member

I'm on board with this, but could you label which graph is from which scenario? It's hard to follow.

@ickshonpe
Copy link
Contributor Author

I'm on board with this, but could you label which graph is from which scenario? It's hard to follow.

Yep I ran the benchmarks with the wrong settings for this initially as well. These ones should be a lot clearer.

@alice-i-cecile 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 Oct 29, 2023
@alice-i-cecile
Copy link
Member

@ickshonpe can you resolve comments? I'll merge this ASAP after.

@TimJentzsch
Copy link
Contributor

Sorry, I think my other PR caused merge conflicts for this.
Should be easy to resolve though, you just have to replace instances of color.0.a() == 0.0 by color.0.is_fully_transparent().

@ickshonpe
Copy link
Contributor Author

@ickshonpe can you resolve comments? I'll merge this ASAP after.

Should be ready now

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 31, 2023
Merged via the queue into bevyengine:main with commit 563d6e3 Oct 31, 2023
ameknite pushed a commit to ameknite/bevy that referenced this pull request Nov 6, 2023
@ickshonpe @ameknite
Add stack index to Node (bevyengine#9853)
fce4e13 
# Objective

If we add the stack index to `Node` then we don't need to walk the
`UiStack` repeatedly during extraction.

## Solution

Add a field `stack_index`  to `Node`.
Update it in `ui_stack_system`.
Iterate queries directly in the UI's extraction systems.

### Benchmarks 
```
cargo run --profile stress-test --features trace_tracy --example many_buttons -- --no-text --no-borders
```

frames (yellow this PR, red main):

<img width="447" alt="frames-per-second"
src="https://github.com/bevyengine/bevy/assets/27962798/385c0ccf-c257-42a2-b736-117542d56eff">

`ui_stack_system`:
<img width="585" alt="ui-stack-system"
src="https://github.com/bevyengine/bevy/assets/27962798/2916cc44-2887-4c3b-a144-13250d84f7d5">

extract schedule:
<img width="469" alt="extract-schedule"
src="https://github.com/bevyengine/bevy/assets/27962798/858d4ab4-d99f-48e8-b153-1c92f51e0743">

---

## Changelog

* Added the field `stack_index` to `Node`.
* `ui_stack_system` updates `Node::stack_index` after a new `UiStack` is
generated.
* The UI's extraction functions iterate a query directly rather than
walking the `UiStack` and doing lookups.
@ickshonpe ickshonpe mentioned this pull request Jan 1, 2024
Merged
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
@ickshonpe
Add stack index to Node (bevyengine#9853)
7a88a55 
# Objective

If we add the stack index to `Node` then we don't need to walk the
`UiStack` repeatedly during extraction.

## Solution

Add a field `stack_index`  to `Node`.
Update it in `ui_stack_system`.
Iterate queries directly in the UI's extraction systems.

### Benchmarks 
```
cargo run --profile stress-test --features trace_tracy --example many_buttons -- --no-text --no-borders
```

frames (yellow this PR, red main):

<img width="447" alt="frames-per-second"
src="https://github.com/bevyengine/bevy/assets/27962798/385c0ccf-c257-42a2-b736-117542d56eff">

`ui_stack_system`:
<img width="585" alt="ui-stack-system"
src="https://github.com/bevyengine/bevy/assets/27962798/2916cc44-2887-4c3b-a144-13250d84f7d5">

extract schedule:
<img width="469" alt="extract-schedule"
src="https://github.com/bevyengine/bevy/assets/27962798/858d4ab4-d99f-48e8-b153-1c92f51e0743">

---

## Changelog

* Added the field `stack_index` to `Node`.
* `ui_stack_system` updates `Node::stack_index` after a new `UiStack` is
generated.
* The UI's extraction functions iterate a query directly rather than
walking the `UiStack` and doing lookups.
@ickshonpe ickshonpe mentioned this pull request Sep 9, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Sep 9, 2024
@ickshonpe
Retrieve the stack_index from Node in extract_ui_material_nodes…
4de9ede 
… instead of walking `UiStack` (#15104)

# Objective

 `ExtractedUiMaterialNode` is still walking the whole `UiStack`. 

more info: #9853

## Solution

Retrieve the `stack_index` from the `Node` component instead.
Also changed the `stack_index` field of `ExtractedUiMaterialNode` to
`u32`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kristoff3r kristoff3r kristoff3r left review comments

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@TimJentzsch TimJentzsch TimJentzsch approved these changes

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 S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ickshonpe @alice-i-cecile @TimJentzsch @kristoff3r