Store taffy node keys in components #8297

ickshonpe · 2023-04-03T18:50:56Z

Objective

Store the taffy node keys in a component on the UI node entities instead of using the UiSurface::entity_to_taffy map to look them up.

Changelog

UiKey:

UiKey is a new component with a single field: taffy_id: taffy::node::Node and a method get to retrieve its value.
UiKey::default() creates a UiKey with a null taffy id value. This should be the only way for users to instantiate a UiNodeId component. If multiple entities with the same non-null UiKey::taffy_id exist, the UI layout will not update correctly.
All UI nodes must have a UiKey component to be included in the UI layout.

NodeBundle, ImageBundle, TextBundle, ButtonBundle:

The UiKey component has been added to each of these bundles.

UiSurface:

The upsert_node method has been split into separate methods: insert_node and set_style.
Removed get_layout. The Taffy layout is accessed directly instead.
Most methods now accept a taffy::node::Node parameter instead since entity_to_taffy lookups are no longer needed.

LayoutError

Removed as no longer used, neither error should ever occur.

ui_layout_system

Removal detection is performed first before updates. Otherwise, the UI node entity tree and the Taffy layout tree could get out of sync if a user removes and then, later in the same frame, reinserts a component used by the UI.

Migration Guide

UiKey is a new component that is managed automatically by ui_layout_system and should only be instantiated with UiKey::default(). It is used to identify a UI node in the UI's internal layout tree.

UiKey has been added to each of the UI node bundles.

* Added new components "TaffyNode" and "TaffyParent" * Added `insert_node` method to `FlexSurface` * Added `make_measure` function to `flex` module. * `flex_node_system` stores taffy nodes keys in the "TaffyNode" and "TaffyParent" components instead of looking them up using the `entity_to_taffy` map.

ickshonpe · 2023-04-03T18:53:56Z

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

crates/bevy_ui/src/flex/mod.rs

github-actions · 2023-04-03T21:03:25Z

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

* added `primary_node` and `physical_to_logical_factor` fields to `FlexSurface`. * added the `Transforms` variant to the `UiSystem` enum. * split off the transforms update from `flex_node_system` into a separate function `update_ui_node_transform` and scheduled it to run after `UiSystem::Flex` and before `TransformSystem::TransformPropagate`.

* removed `TaffyParent` component, added `parent` field to `TaffyNode` component * removed `PrimaryNode` field from `FlexSurface`

mockersf · 2023-04-04T06:36:40Z

Is it possible to remove the entity_to_taffy hashmap? Otherwise it would mean storing the same information twice

ickshonpe · 2023-04-04T07:10:48Z

Is it possible to remove the entity_to_taffy hashmap? Otherwise it would mean storing the same information twice

I don't think so, it's needed for handling children and removing orphaned taffy nodes.
Maybe if Taffy could be made to store the layout data directly in the Bevy UI entities instead of using SlotMap.

nicoburns · 2023-04-04T09:29:24Z

Is it possible to remove the entity_to_taffy hashmap? Otherwise it would mean storing the same information twice

I don't think so, it's needed for handling children and removing orphaned taffy nodes. Maybe if Taffy could be made to store the layout data directly in the Bevy UI entities instead of using SlotMap.

DioxusLabs/taffy#326 (or more likely, parts of this PR split out into their own PRs) will allow this. It's supposed to be possible with the current version of the LayoutTree trait, but in practice the current version has things like SlotMap keys in the trait function signatures which make it impractical.

With that approach all Taffy data (input styles, parent-children relationships, and and output layouts) would be stored by Bevy (however it chose to do so). Care would need to be taken to ensure performance parity though, as Taffy's SlotMaps are backed by Vecs and are therefore very fast to access. Not sure if it's possible to get Vec backed storage in bevy_ecs that only applies to UI nodes?

nicoburns · 2023-04-04T09:41:28Z

Although to this specific issue there might be a simpler solution. My understanding is that a Bevy component typically is a HashMap, so a Query of a component containing the Taffy node key ought to function the same as a HashMap containing the mapping I think.

* Renamed `Node` to `NodeSize` * Renamed `TaffyNode` to `Node` and removed its `Parent` field. * Split off parts of `flex_node_system` into separate systems: `synchronise_children`, `insert_new_ui_nodes_into_layout` and `clean_up_removed_ui_nodes`.

github-actions · 2023-04-13T13:47:32Z

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

github-actions · 2023-04-13T13:48:05Z

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

* Renamed `flex_node_system` to `update_ui_layout`. * Replaced the `window_nodes` map field of `UiSurface` with a single `window_node` node field.

* Removed redundant `Node` component removal. * Query for `Node` components of root nodes instead of `Entity` so that `set_window_children` doesn't have to look up the keys. * Added a a simple test for node insertion and removal.

…ield and `get` method. Edited the doc comment for `insert_node`.

…t-merge

…e `layout` module. Renamed some variables to be consistant with the name change. `UiSurface::insert_node` reuses the previous taffy node associated to the given `entity`, if one exists.

ickshonpe · 2023-06-14T21:31:51Z

Improved the implementation and reduced it down to just the minimum necessary changes without any extra systems. Should be ready to merge, but I'll make some new tests for UiSurface.

Changed the ordering of parameters for the `from_style` and `LayoutContext::new` functions to put the most important parameter first. `UiSurface`: Added a test `uisurface_insert_and_remove_nodes`.

Type alias for `taffy::node::Node`. `UiSurface::get_key` Method that can be used to lookup taffy nodes from `entity_to_taffy` but takes and returns a value rather than a borrow.

…ckshonpe/bevy into store-taffy-keys-in-components

nicoburns · 2023-06-19T17:44:26Z

@ickshonpe What's the motivation for this change? Performance? If so it would be good to get some perf numbers vs. main for the latest version.

mockersf · 2023-06-19T18:25:09Z

It's increasing performances at the cost of memory. Not sure it's worth it in this case, as the gain is 0.2ms in an extreme UI case...

cart · 2023-07-08T00:09:32Z

I do like the idea of breaking this out from a centralized hashmap (and perf wins are good), but having multiple sources of truth that need syncing gives me some pause. I'll need to give this some thought, which sadly will need to wait until 0.12.

ickshonpe changed the title ~~Store taffy node keys in the UI entities instead of looking them up~~ Store taffy node keys in UI node entities instead of looking them up Apr 3, 2023

ickshonpe changed the title ~~Store taffy node keys in UI node entities instead of looking them up~~ Store taffy node keys in components Apr 3, 2023

james7132 reviewed Apr 3, 2023

View reviewed changes

crates/bevy_ui/src/flex/mod.rs Outdated Show resolved Hide resolved

Remove TaffyNode and TaffyParent components when Node is removed.

460c438

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 Apr 3, 2023

alice-i-cecile added this to the 0.11 milestone Apr 3, 2023

ickshonpe added 2 commits April 3, 2023 23:25

changes:

0fd7b89

* removed `TaffyParent` component, added `parent` field to `TaffyNode` component * removed `PrimaryNode` field from `FlexSurface`

ickshonpe added 3 commits April 13, 2023 14:14

changes:

584d6c7

* Renamed `Node` to `NodeSize` * Renamed `TaffyNode` to `Node` and removed its `Parent` field. * Split off parts of `flex_node_system` into separate systems: `synchronise_children`, `insert_new_ui_nodes_into_layout` and `clean_up_removed_ui_nodes`.

Merge branch 'main' into store-taffy-keys-in-components

2efb1f3

cargo fmt

6739af2

ickshonpe added 9 commits April 13, 2023 16:08

handle window events for ui in a separate system

feb61f7

fix scale factor changed check

b4a6a7a

cargo fmt

f7dbefb

move window update back into flex_node_system

d2a7eac

* Renamed FlexSuface to UiSurface.

183677a

* Renamed `flex_node_system` to `update_ui_layout`. * Replaced the `window_nodes` map field of `UiSurface` with a single `window_node` node field.

fixed tests

c93106f

Cargo fmt --all

accd08e

doc comment and clippy fixes

51c69be

changes:

a189982

* Removed redundant `Node` component removal. * Query for `Node` components of root nodes instead of `Entity` so that `set_window_children` doesn't have to look up the keys. * Added a a simple test for node insertion and removal.

ickshonpe added 6 commits June 14, 2023 15:20

Changed UiNodeId from a tuple struct to a stuct with a taffy_id f…

79ddbcd

…ield and `get` method. Edited the doc comment for `insert_node`.

Merge branch 'store-taffy-keys-in-components' into taffy-key-componen…

5156726

…t-merge

Renamed UiNodeId to UiKey, and moved its struct declaration to th…

97be1c4

…e `layout` module. Renamed some variables to be consistant with the name change. `UiSurface::insert_node` reuses the previous taffy node associated to the given `entity`, if one exists.

cargo fmt --all

afc397a

Changed some paramter names to be more descriptive.

997fc32

Removed LayoutError as it's no longer used.

68c150f

ickshonpe added 6 commits June 16, 2023 11:06

layout module:

73f2596

Changed the ordering of parameters for the `from_style` and `LayoutContext::new` functions to put the most important parameter first. `UiSurface`: Added a test `uisurface_insert_and_remove_nodes`.

Taffykey:

9546431

Type alias for `taffy::node::Node`. `UiSurface::get_key` Method that can be used to lookup taffy nodes from `entity_to_taffy` but takes and returns a value rather than a borrow.

Taffykey:

e8388a5

Type alias for `taffy::node::Node`. `UiSurface::get_key` Method that can be used to lookup taffy nodes from `entity_to_taffy` but takes and returns a value rather than a borrow.

Merge branch 'store-taffy-keys-in-components' of https://github.com/i…

e654987

…ckshonpe/bevy into store-taffy-keys-in-components

cargo fmt

5fc76a1

moved window handling in ui_layout_system outside of the update logic.

7deab35

alice-i-cecile requested review from nicoburns and james7132 June 19, 2023 16:08

Merge branch 'main' into store-taffy-keys-in-components

f029505

alice-i-cecile requested review from alice-i-cecile and mockersf June 19, 2023 16:10

alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Jun 19, 2023

cart modified the milestones: 0.11, 0.12 Jul 8, 2023

ickshonpe added 2 commits August 21, 2023 22:54

Merge branch 'main' into store-taffy-keys-in-components

866aa9f

Cleaned up left over comments from merge

b67c30c

ickshonpe mentioned this pull request Aug 29, 2023

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

Closed

alice-i-cecile removed this from the 0.12 milestone Sep 29, 2023

BenjaminBrienen added D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jan 23, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Store taffy node keys in components #8297

Store taffy node keys in components #8297

Uh oh!

ickshonpe commented Apr 3, 2023 •

edited

Loading

Uh oh!

ickshonpe commented Apr 3, 2023

Uh oh!

Uh oh!

github-actions bot commented Apr 3, 2023

Uh oh!

mockersf commented Apr 4, 2023

Uh oh!

ickshonpe commented Apr 4, 2023 •

edited

Loading

Uh oh!

nicoburns commented Apr 4, 2023

Uh oh!

nicoburns commented Apr 4, 2023 •

edited

Loading

Uh oh!

github-actions bot commented Apr 13, 2023

Uh oh!

github-actions bot commented Apr 13, 2023

Uh oh!

ickshonpe commented Jun 14, 2023

Uh oh!

nicoburns commented Jun 19, 2023

Uh oh!

mockersf commented Jun 19, 2023 •

edited

Loading

Uh oh!

cart commented Jul 8, 2023

Uh oh!

Uh oh!

Uh oh!

Store taffy node keys in components #8297

Are you sure you want to change the base?

Store taffy node keys in components #8297

Uh oh!

Conversation

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

Uh oh!

Objective

Changelog

Migration Guide

Uh oh!

ickshonpe commented Apr 3, 2023

Uh oh!

Uh oh!

github-actions bot commented Apr 3, 2023

Uh oh!

mockersf commented Apr 4, 2023

Uh oh!

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

Uh oh!

Uh oh!

nicoburns commented Apr 4, 2023

Uh oh!

nicoburns commented Apr 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Apr 13, 2023

Uh oh!

github-actions bot commented Apr 13, 2023

Uh oh!

ickshonpe commented Jun 14, 2023

Uh oh!

nicoburns commented Jun 19, 2023

Uh oh!

mockersf commented Jun 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cart commented Jul 8, 2023

Uh oh!

Uh oh!

ickshonpe commented Apr 3, 2023 •

edited

Loading

ickshonpe commented Apr 4, 2023 •

edited

Loading

nicoburns commented Apr 4, 2023 •

edited

Loading

mockersf commented Jun 19, 2023 •

edited

Loading