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

Cleanup UI surface mappings - Follow up for #12698 #12719

Closed
wants to merge 3 commits into from
Closed

Cleanup UI surface mappings - Follow up for #12698 #12719

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0cfed79
Collapse UiSurface.camera_roots and camera_entity_to_taffy into camer…
StrikeForceZero Mar 25, 2024
5384d4e
Restore the structured output of print_ui_layout_tree
StrikeForceZero Mar 25, 2024
7b90efb
Remove RootNodePair and simplify camera-entity mapping in the UiSurface
StrikeForceZero Mar 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 19 additions & 13 deletions crates/bevy_ui/src/layout/debug.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,20 +12,26 @@ pub fn print_ui_layout_tree(ui_surface: &UiSurface) {
.iter()
.map(|(entity, node)| (*node, *entity))
.collect();
for (&entity, roots) in &ui_surface.camera_roots {
let mut camera_tree_output_map = HashMap::<Entity, Vec<String>>::new();
for (&(camera_entity, _), &implicit_viewport_node) in ui_surface.camera_root_to_viewport_taffy.iter() {
let mut out = String::new();
for root in roots {
print_node(
ui_surface,
&taffy_to_entity,
entity,
root.implicit_viewport_node,
false,
String::new(),
&mut out,
);
}
bevy_utils::tracing::info!("Layout tree for camera entity: {entity:?}\n{out}");
print_node(
ui_surface,
&taffy_to_entity,
camera_entity,
implicit_viewport_node,
false,
String::new(),
&mut out,
);
camera_tree_output_map
.entry(camera_entity)
.or_default()
.push(out);
}
for (camera_entity, tree_strings) in camera_tree_output_map.into_iter() {
let output = tree_strings.join("\n");
bevy_utils::tracing::info!("Layout tree for camera entity: {camera_entity:?}\n{output}");
}
}

Expand Down
142 changes: 77 additions & 65 deletions crates/bevy_ui/src/layout/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ pub mod debug;
use crate::{ContentSize, DefaultUiCamera, Node, Outline, Style, TargetCamera, UiScale};
use bevy_ecs::{
change_detection::{DetectChanges, DetectChangesMut},
entity::{Entity, EntityHashMap},
entity::{Entity, EntityHash, EntityHashMap},
event::EventReader,
query::{With, Without},
removal_detection::RemovedComponents,
Expand All @@ -16,7 +16,7 @@ use bevy_math::{UVec2, Vec2};
use bevy_render::camera::{Camera, NormalizedRenderTarget};
use bevy_transform::components::Transform;
use bevy_utils::tracing::warn;
use bevy_utils::{default, HashMap, HashSet};
use bevy_utils::{default, hashbrown, HashMap, HashSet};
use bevy_window::{PrimaryWindow, Window, WindowScaleFactorChanged};
use std::fmt;
use taffy::{tree::LayoutTree, Taffy};
Expand All @@ -41,25 +41,20 @@ impl LayoutContext {
}
}

#[derive(Debug, Clone, PartialEq, Eq)]
struct RootNodePair {
// The implicit "viewport" node created by Bevy
implicit_viewport_node: taffy::node::Node,
// The root (parentless) node specified by the user
user_root_node: taffy::node::Node,
}
type EntityTupleHashMap<V> = hashbrown::HashMap<(Entity, Entity), V, EntityHash>;

#[derive(Resource)]
pub struct UiSurface {
entity_to_taffy: EntityHashMap<taffy::node::Node>,
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>,
Copy link
Contributor Author

@StrikeForceZero StrikeForceZero Mar 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_taffy and restore RootNodePair. 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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. EntityHashMap<EntityHashMap<taffy::node::Node>>
  2. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 UiSurface into its own file since layout/mod is getting pretty big.
  • to clean some stuff up
  • implement the revised mapping structure
  • add more regression tests for edge cases I found affecting main

taffy: Taffy,
}

fn _assert_send_sync_ui_surface_impl_safe() {
fn _assert_send_sync<T: Send + Sync>() {}
_assert_send_sync::<EntityHashMap<taffy::node::Node>>();
_assert_send_sync::<EntityTupleHashMap<taffy::node::Node>>();
_assert_send_sync::<Taffy>();
_assert_send_sync::<UiSurface>();
}
Expand All @@ -68,7 +63,10 @@ impl fmt::Debug for UiSurface {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
f.debug_struct("UiSurface")
.field("entity_to_taffy", &self.entity_to_taffy)
.field("camera_roots", &self.camera_roots)
.field(
"camera_root_to_viewport_taffy",
&self.camera_root_to_viewport_taffy,
)
.finish()
}
}
Expand All @@ -79,8 +77,7 @@ impl Default for UiSurface {
taffy.disable_rounding();
Self {
entity_to_taffy: Default::default(),
camera_entity_to_taffy: Default::default(),
camera_roots: Default::default(),
camera_root_to_viewport_taffy: Default::default(),
taffy,
}
}
Expand Down Expand Up @@ -168,68 +165,83 @@ without UI components as a child of an entity with UI components, results may be
..default()
};

let camera_root_node_map = self.camera_entity_to_taffy.entry(camera_id).or_default();
let existing_roots = self.camera_roots.entry(camera_id).or_default();
let mut new_roots = Vec::new();
for entity in children {
let node = *self.entity_to_taffy.get(&entity).unwrap();
let root_node = existing_roots
.iter()
.find(|n| n.user_root_node == node)
.cloned()
.unwrap_or_else(|| {
if let Some(previous_parent) = self.taffy.parent(node) {
// remove the root node from the previous implicit node's children
self.taffy.remove_child(previous_parent, node).unwrap();
}

let viewport_node = *camera_root_node_map
.entry(entity)
.or_insert_with(|| self.taffy.new_leaf(viewport_style.clone()).unwrap());
self.taffy.add_child(viewport_node, node).unwrap();
let user_root_node = *self
.entity_to_taffy
.get(&entity)
.expect("root node was not inserted yet or was removed from entity_to_taffy map");

let implicit_viewport_node = *self
.camera_root_to_viewport_taffy
.entry((camera_id, entity))
.or_insert_with(|| self.taffy.new_leaf(viewport_style.clone()).unwrap());

if let Some(previous_parent) = self.taffy.parent(user_root_node) {
// remove the root node from the previous implicit node's children
self.taffy
.remove_child(previous_parent, user_root_node)
.unwrap();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

RootNodePair {
implicit_viewport_node: viewport_node,
user_root_node: node,
}
});
new_roots.push(root_node);
self.taffy
.add_child(implicit_viewport_node, user_root_node)
.unwrap();
}

self.camera_roots.insert(camera_id, new_roots);
}

/// Compute the layout for each window entity's corresponding root node in the layout.
pub fn compute_camera_layout(&mut self, camera: Entity, render_target_resolution: UVec2) {
let Some(camera_root_nodes) = self.camera_roots.get(&camera) else {
return;
};
for (&(camera_entity, _), &implicit_viewport_node) in
self.camera_root_to_viewport_taffy.iter()
{
if camera_entity != camera {
continue;
}

let available_space = taffy::geometry::Size {
width: taffy::style::AvailableSpace::Definite(render_target_resolution.x as f32),
height: taffy::style::AvailableSpace::Definite(render_target_resolution.y as f32),
};

let available_space = taffy::geometry::Size {
width: taffy::style::AvailableSpace::Definite(render_target_resolution.x as f32),
height: taffy::style::AvailableSpace::Definite(render_target_resolution.y as f32),
};
for root_nodes in camera_root_nodes {
self.taffy
.compute_layout(root_nodes.implicit_viewport_node, available_space)
.compute_layout(implicit_viewport_node, available_space)
.unwrap();
}
}

/// Removes each camera entity from the internal map and then removes their associated node from taffy
/// Removes specified camera entities by disassociating them from their associated `implicit_viewport_node`
/// in the internal map, and subsequently removes the `implicit_viewport_node`
/// from the `taffy` layout engine for each.
pub fn remove_camera_entities(&mut self, entities: impl IntoIterator<Item = Entity>) {
for entity in entities {
if let Some(camera_root_node_map) = self.camera_entity_to_taffy.remove(&entity) {
for (_, node) in camera_root_node_map.iter() {
self.taffy.remove(*node).unwrap();
}
}
self.camera_root_to_viewport_taffy.retain(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I originally wrote this earlier, I hadn't realized I was retaining inside a loop. I will need to circle back to one.

|&(camera_entity, _), implicit_viewport_node| {
if camera_entity != entity {
true
} else {
self.taffy.remove(*implicit_viewport_node).unwrap();
false
}
},
);
}
}

/// Removes each entity from the internal map and then removes their associated node from taffy
/// Removes the specified entities from the internal map while
/// removing their `implicit_viewport_node` from taffy,
/// and then subsequently removes their entry from `entity_to_taffy` and associated node from taffy
pub fn remove_entities(&mut self, entities: impl IntoIterator<Item = Entity>) {
for entity in entities {
self.camera_root_to_viewport_taffy.retain(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same goes for this one with the retain inside the loop.

|&(_, root_node_entity), implicit_viewport_node| {
if root_node_entity != entity {
true
} else {
self.taffy.remove(*implicit_viewport_node).unwrap();
false
}
},
);
if let Some(node) = self.entity_to_taffy.remove(&entity) {
self.taffy.remove(node).unwrap();
}
Expand Down Expand Up @@ -699,7 +711,7 @@ mod tests {

// no UI entities in world, none in UiSurface
let ui_surface = world.resource::<UiSurface>();
assert!(ui_surface.camera_entity_to_taffy.is_empty());
assert!(ui_surface.camera_root_to_viewport_taffy.is_empty());

// respawn camera
let camera_entity = world.spawn(Camera2dBundle::default()).id();
Expand All @@ -708,26 +720,26 @@ mod tests {
.spawn((NodeBundle::default(), TargetCamera(camera_entity)))
.id();

// `ui_layout_system` should map `camera_entity` to a ui node in `UiSurface::camera_entity_to_taffy`
// `ui_layout_system` should create a keypair (`camera_entity`, `root_node_entity`) in `UiSurface::camera_root_to_viewport_taffy`
ui_schedule.run(&mut world);

let ui_surface = world.resource::<UiSurface>();
assert!(ui_surface
.camera_entity_to_taffy
.contains_key(&camera_entity));
assert_eq!(ui_surface.camera_entity_to_taffy.len(), 1);
.camera_root_to_viewport_taffy
.contains_key(&(camera_entity, ui_entity)));
assert_eq!(ui_surface.camera_root_to_viewport_taffy.len(), 1);

world.despawn(ui_entity);
world.despawn(camera_entity);

// `ui_layout_system` should remove `camera_entity` from `UiSurface::camera_entity_to_taffy`
// `ui_layout_system` should remove (`camera_entity`, _) from `UiSurface::camera_root_to_viewport_taffy`
ui_schedule.run(&mut world);

let ui_surface = world.resource::<UiSurface>();
assert!(!ui_surface
.camera_entity_to_taffy
.contains_key(&camera_entity));
assert!(ui_surface.camera_entity_to_taffy.is_empty());
.camera_root_to_viewport_taffy
.contains_key(&(camera_entity, ui_entity)));
assert!(ui_surface.camera_root_to_viewport_taffy.is_empty());
}

#[test]
Expand Down
Loading