Cleanup UI surface mappings - Follow up for #12698 #12719
Conversation
…a_and_root_node_pair This is to reduce redundancy and simplify the logic
Replaces the 'camera_and_root_node_pair' hashmap with 'camera_root_to_viewport_taffy'. Updating usages to directly reference 'implicit_viewport_node' and 'user_root_node', and removes the 'RootNodePair' struct.
| self.taffy.remove(*node).unwrap(); | ||
| } | ||
| } | ||
| self.camera_root_to_viewport_taffy.retain( |
There was a problem hiding this comment.
When I originally wrote this earlier, I hadn't realized I was retaining inside a loop. I will need to circle back to one.
| pub fn remove_entities(&mut self, entities: impl IntoIterator<Item = Entity>) { | ||
| for entity in entities { | ||
| self.camera_root_to_viewport_taffy.retain( |
There was a problem hiding this comment.
The same goes for this one with the retain inside the loop.
| // remove the root node from the previous implicit node's children | ||
| self.taffy | ||
| .remove_child(previous_parent, user_root_node) | ||
| .unwrap(); |
There was a problem hiding this comment.
I think that moving this outside the original unwrap_or_else means it eagerly removes the child from its previous parent and then immediately applies the new (or same one) right after, regardless of whether it needs to. It might be negligible, but still worth leaving a note.
| camera_entity_to_taffy: EntityHashMap<EntityHashMap<taffy::node::Node>>, | ||
| camera_roots: EntityHashMap<Vec<RootNodePair>>, | ||
| /// `HashMap<(Camera Entity, Root (parentless) Entity), implicit Viewport>` | ||
| camera_root_to_viewport_taffy: EntityTupleHashMap<taffy::node::Node>, |
There was a problem hiding this comment.
Few things here:
-
(Entity, Entity)as the key makes it harder to work with unless you iterate over the whole thing or already have both values. The benefit is that a viewport node can't be inserted without the camera entity and root node entity being in the map as the key. -
We could take this a step further and remove
entity_to_taffyand restoreRootNodePair. But that does affect a few bit more LOC. And not to mention, would make lookups even less optimal.
Conclusion; I think these are interesting approaches, but it might be better to create a management struct that maintains it / while preserving the speed as before. (I see how I can change a few calls to make it more atomic) Gonna explore more here unless someone has a good suggestion that I'm not seeing!
There was a problem hiding this comment.
I don't think EntityTupleHashMap<taffy::node::Node> makes sense as you want to be able to iterate over root nodes given just a camera node. I think either of:
EntityHashMap<EntityHashMap<taffy::node::Node>>EntityHashMap<Vec<RootNodePair>>
ought to work fine on their own. They're essentially equivalent with slightly different performance characteristics. 2. is likely faster for iterating and lookups with small numbers of root nodes per camera. 1. is likely faster for lookups with large numbers of root nodes per camera. This is unlikely to be a performance hotspot so we probably ought not to worry about it too much.
A third option of EntityHashMap<taffy::node::Node> where the Node stored is the "user root node" and you lookup the "implicit viewport node" using Taffy.parent(user_root_node) would probably also work. Looking up a parent in Taffy should be fast as it's just a Vec lookup.
There was a problem hiding this comment.
After a massive 3 day deep dive through the layout system. I've boiled it down to this:
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct UiNodeMeta {
pub(super) camera_entity: Option<Entity>,
pub(super) root_node_pair: RootNodePair,
}
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct RootNodePair {
// The implicit "viewport" node created by Bevy
pub(super) implicit_viewport_node: taffy::node::Node,
// The root (parentless) node specified by the user
pub(super) user_root_node: taffy::node::Node,
}
#[derive(Resource)]
pub struct UiSurface {
pub(super) entity_to_taffy: EntityHashMap<taffy::node::Node>,
// I'm not sure I like the name `meta` but can't think of anything else right now.
// maybe `relations`?
pub(super) ui_root_node_meta: EntityHashMap<UiNodeMeta>,
pub(super) camera_root_nodes: EntityHashMap<EntityHashSet>,
pub(super) taffy: Taffy,
}My goal in the next day or so is to break up the work I've been experimenting with and submit several incremental PR's with intentions to:
- document several areas, including the ones I unknowingly stomped over
- extract
UiSurfaceinto its own file sincelayout/modis getting pretty big. - to clean some stuff up
- implement the revised mapping structure
- add more regression tests for edge cases I found affecting main
ItsDoot
added
A-UI
|
closing in favor of #12804 |
Draft PR:
I still need to review some potential performance tradeoffs and whether bringing back theSee comment #12719 (comment)RootNodePairstructure to collapse it even further is an option or not.Objective
UiSurfaceSolution
EntityHashMap<(Entity, Entity), taffy::node::Node>, we reduce the redundancy and complexity of the internal mappings.