-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Reduce steady-state allocations in ui_stack_system #12413
Conversation
For some reason my rust analyzer is erroring on the |
Do a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good, though I'd like to see some actual benchmarks. many_buttons
and the other UI stress tests are probably the best scenarios to check.
I'm not too happy with the idea of a Vec per non-leaf node being allocated here, even in the cached steady state. Ideally we would just use one giant Vec and retain ranges, but this can be attempted in a follow up PR.
Yeah that would be ideal, issues to consider:
I will try these. |
345f289
to
cb428c4
Compare
crates/bevy_ui/src/stack.rs
Outdated
@@ -15,6 +15,24 @@ pub struct UiStack { | |||
pub uinodes: Vec<Entity>, | |||
} | |||
|
|||
#[derive(Default)] | |||
pub struct StackingContextCache { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs docs if it's public. I would probably add docs for internal users and doc(hidden) this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's public because the system is public even though it isn't actually exported. The visibility controls in this module don't make any sense.
EDIT: Added a commit to clean up visibility in this module.
It looks like your PR is a breaking change, but you didn't provide a migration guide. Could you add some context on what users should update when this change get released in a new version of Bevy? |
@james7132 I don't think this was actually breaking: the types weren't publicly exported despite the |
Objective
Solution
ui_stack_system
.Follow-Up
sort_by_key
is potentially-allocating. It might be worthwhile to include the child index as part of the sort-key and use unstable sort.