-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Cleanup UI surface mappings - Follow up for #12698 #12719
Changes from all commits
0cfed79
5384d4e
7b90efb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
|
@@ -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}; | ||
|
@@ -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>, | ||
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>(); | ||
} | ||
|
@@ -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() | ||
} | ||
} | ||
|
@@ -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, | ||
} | ||
} | ||
|
@@ -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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that moving this outside the original |
||
} | ||
|
||
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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
} | ||
|
@@ -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(); | ||
|
@@ -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] | ||
|
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.
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 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.
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: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" usingTaffy.parent(user_root_node)
would probably also work. Looking up a parent in Taffy should be fast as it's just aVec
lookup.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.
After a massive 3 day deep dive through the layout system. I've boiled it down to this:
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:
UiSurface
into its own file sincelayout/mod
is getting pretty big.