Skip UI updates when UI state unchanged by ickshonpe · Pull Request #8610 · bevyengine/bevy

ickshonpe · 2023-05-13T18:59:20Z

Objective

Updates to the node geometries, the UI stack, and clipping rects can be skipped entirely if the UI node entities, the window resolution, and the scale factor are all unchanged.

Solution

Add a flag to UiSurface needs_update that is set by ui_layout_system if it makes any changes to the UI layout tree. If the flag is not set, don't update the node geometry, UI stack, or clipping rects.

Changelog

Added a flag to UiSurface needs_update that is set by ui_layout_system if it makes any changes to the UI layout tree.
Added run_if conditions to ui_stack_system and update_clipping_system so they only run if needs_update is set.
ui_layout_system only does compute_window_layouts and the Node and Transform updates if needs_update is set.

Migration Guide

Adds a flag to `UiSurface` `needs_update` that is set by `ui_layout_system` if it makes any changes to the UI layout tree. If the flag is not set, updates are skipped for the taffy layout tree, node geometry, UI stack, and clipping rects.

github-actions · 2023-05-13T19:08:16Z

Example alien_cake_addict failed to run, please try running it locally and check the result.

mockersf · 2023-05-14T12:32:56Z

Example failed with

thread 'Compute Task Pool (0)' panicked at 'called `Option::unwrap()` on a `None` value', crates/bevy_ui/src/layout/mod.rs:171:64

Can you reproduce locally?

ickshonpe · 2023-05-16T09:44:24Z

Example failed with

thread 'Compute Task Pool (0)' panicked at 'called `Option::unwrap()` on a `None` value', crates/bevy_ui/src/layout/mod.rs:171:64

Can you reproduce locally?

No, works fine locally. Not tested it yet but I think I can see what's wrong. update_window is set to run on WindowResized events, but on some platforms window creation doesn't emit a WindowResized event.

… that when a new primary window is created, it can create a root taffy node corresponding to the window. * Added an `EventReader<WindowCreated>` parameter to `ui_layout_system`. * `ui_layout_system` performs a full update on recieving a `WindowCreated` event for the primary window entity.

…Ui Node entity.

…ethod.

ickshonpe · 2023-05-16T10:30:48Z

Should be good now.

alice-i-cecile · 2023-05-22T20:49:45Z

crates/bevy_ui/src/layout/mod.rs

 }

 impl UiSurface {
+    pub fn needs_update(&self) -> bool {


Doc strings: this is pub (and will help future readers).

alice-i-cecile · 2023-05-22T20:50:22Z

crates/bevy_ui/src/layout/mod.rs

-    /// Removes children from the entity's taffy node if it exists. Does nothing otherwise.
-    pub fn try_remove_children(&mut self, entity: Entity) {
+    /// Removes children from the entity's taffy node if it exists.
+    /// Returns false if no corresponding taffy node.


Suggested change

/// Returns false if no corresponding taffy node.

///

/// Returns false if no corresponding taffy node exists.

alice-i-cecile

Works locally for me as well. Skipping this work is absolutely worth it: in 99.9% of frames we won't need this.

I would probably use a run condition rather than an in-system check to see if it needs to be skipped, as that will prevent spawning a thread. Minor, but probably a bit nicer.

If you feel that the in-system check is better (it certainly involves a smaller change), just clean up the docs and I'll approve :)

ickshonpe · 2023-05-23T21:47:29Z

Works locally for me as well. Skipping this work is absolutely worth it: in 99.9% of frames we won't need this.

I would probably use a run condition rather than an in-system check to see if it needs to be skipped, as that will prevent spawning a thread. Minor, but probably a bit nicer.

If you feel that the in-system check is better (it certainly involves a smaller change), just clean up the docs and I'll approve :)

Yep I'm trying to keep to small and isolated changes with these PRs, I have quite a lot of them to put in.

I'm not quite sure what you mean about run conditions? I'm using run conditions for the ui_stack_system and update_clipping_system checks already and ui_layout_system needs to run to decide if any updates should be made or not.

alice-i-cecile · 2023-05-23T23:50:25Z

I'm not quite sure what you mean about run conditions? I'm using run conditions for the ui_stack_system and update_clipping_system checks already and ui_layout_system needs to run to decide if any updates should be made or not.

I was thinking that you could split out ui_layout system into more parts: one to check if updates should be made, and another to actually do the layout. Might not be faster though, as you'd need to walk the trees twice 🤔 Don't worry about it for this PR.

ickshonpe · 2023-05-24T08:31:44Z

I'm not quite sure what you mean about run conditions? I'm using run conditions for the ui_stack_system and update_clipping_system checks already and ui_layout_system needs to run to decide if any updates should be made or not.

I was thinking that you could split out ui_layout system into more parts: one to check if updates should be made, and another to actually do the layout. Might not be faster though, as you'd need to walk the trees twice 🤔 Don't worry about it for this PR.

Ah right. Yes ui_layout_system definitely needs to be split up.
I think moving out the windowing code is the first step, then we can run UI layout updates in isolation and write lots of tests.

djeedai · 2023-05-27T07:19:18Z

crates/bevy_ui/src/layout/mod.rs

            .collect::<Vec<taffy::node::Node>>();
-        self.taffy.set_children(*taffy_node, &child_nodes).unwrap();
+        if self.taffy.children(*taffy_node).unwrap() != child_nodes {
+            self.taffy.set_children(*taffy_node, &child_nodes).unwrap();


Why not bake that check into Taffy's set_children() directly and have Taffy itself tell us if any change occurred? That sounds better conceptually, and allows Taffy internally to optimize any Vec comparison if possible (can maybe run the Vec once looking for changes and applying them in one pass?).

Why not bake that check into Taffy's set_children() directly and have Taffy itself tell us if any change occurred? That sounds better conceptually, and allows Taffy internally to optimize any Vec comparison if possible (can maybe run the Vec once looking for changes and applying them in one pass?).

Probably what we'll do in the future is have our own implementation of Taffy's LayoutTree rather than using the Taffy built-in. With a custom LayoutTree impl the Taffy layout algorithm can access Bevy's parent-child tree directly and there would be no need to maintain a second internal tree with all the fragile synchronization logic that entails.

Ok thanks for the details!

changes:

805069c

Adds a flag to `UiSurface` `needs_update` that is set by `ui_layout_system` if it makes any changes to the UI layout tree. If the flag is not set, updates are skipped for the taffy layout tree, node geometry, UI stack, and clipping rects.

nicoburns added C-Performance A change motivated by improving speed, memory usage or compile times A-UI Graphical user interfaces, styles, layouts, and widgets labels May 14, 2023

ickshonpe added 5 commits May 16, 2023 11:01

Set needs_update when UiSurface::remove_entities finds a removed …

b44d213

…Ui Node entity.

Only set needs_update if a UI node entity is removed.

e02372e

Made needs_update field of UiSurface private and added a getter m…

aaeb464

…ethod.

cargo fmt --all

3588a57

alice-i-cecile reviewed May 22, 2023

View reviewed changes

alice-i-cecile requested changes May 22, 2023

View reviewed changes

* renamed needs_update to layout_updated

ee20813

cargo fmt --all

f25ef87

djeedai reviewed May 27, 2023

View reviewed changes

ickshonpe changed the title ~~Skip UI updates when UI nodes and window state unchanged~~ Skip UI updates when UI state unchanged Jun 15, 2023

ickshonpe mentioned this pull request Aug 29, 2023

Adding just a Node component to an empty entity causes a panic #9615

Closed

TimJentzsch added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Jul 15, 2024

BenjaminBrienen added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 31, 2024

	/// Returns false if no corresponding taffy node.
	///
	/// Returns false if no corresponding taffy node exists.

Uh oh!

Comments

Conversation

ickshonpe commented May 13, 2023

Objective

Solution

Changelog

Migration Guide

Uh oh!

github-actions bot commented May 13, 2023

Uh oh!

mockersf commented May 14, 2023

Uh oh!

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

Uh oh!

Uh oh!

ickshonpe commented May 16, 2023

Uh oh!

alice-i-cecile May 22, 2023

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile May 22, 2023

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

ickshonpe commented May 23, 2023

Uh oh!

alice-i-cecile commented May 23, 2023

Uh oh!

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

Uh oh!

Uh oh!

djeedai May 27, 2023

Choose a reason for hiding this comment

Uh oh!

ickshonpe May 27, 2023 • 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!

djeedai May 27, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ickshonpe commented May 16, 2023 •

edited

Loading

ickshonpe commented May 24, 2023 •

edited

Loading

ickshonpe May 27, 2023 •

edited

Loading