From 8d1b660227ea949702ea6a206f4fe8b0f2d19ab7 Mon Sep 17 00:00:00 2001
From: John Mitchell <6656977+StarArawn@users.noreply.github.com>
Date: Sat, 29 Apr 2023 10:29:12 -0400
Subject: [PATCH] Tree shenanigans fixed..
---
docs/kayak_children.md | 83 +++++++++
kayak_ui_macros/src/widget.rs | 6 +-
src/children.rs | 4 +
src/context.rs | 332 +++++++++++++++++++---------------
src/tree.rs | 19 +-
src/widget_context.rs | 26 ++-
src/widgets/app.rs | 1 -
7 files changed, 317 insertions(+), 154 deletions(-)
create mode 100644 docs/kayak_children.md
diff --git a/docs/kayak_children.md b/docs/kayak_children.md
new file mode 100644
index 0000000..a7afecb
--- /dev/null
+++ b/docs/kayak_children.md
@@ -0,0 +1,83 @@
+## Intro
+1. We have a simple tree that stores entities.
+2. Nodes in the tree need to have consistent identifiers(I.E. entity id's).
+
+## Problems
+1. Children passing - Children are entities that are passed from higher up in the tree to some place lower in the tree. Normally children are directly attached to their parent, but because of our tree structure we often need to spawn entities ahead of time because the parent has not yet spawned. Example xml tree:
+
+Root:
+```xml
+<a>
+ <b>
+ <c />
+ </b>
+</a>
+```
+B:
+```xml
+<d>
+ {children}
+</d>
+```
+
+Here the widget B is wrapped around C but the actual children are a child of D.
+
+How can we handle children?
+
+### 1. Closures - A function that can be passed down the tree.
+Pros:
+- Essentially differed adding and spawning of widgets until later.
+- No need to worry about if the entity ID is correct in the tree just pull it out.
+
+Cons:
+- Closures require ownership to move data into them. This might seem small but remember we have a tree again so quite quickly it becomes challenging:
+```rust
+// Imagine some data that has clone only.
+let some_data = SomeData { foo };
+parent.children = Children::new(move ||
+ let some_data = some_data.clone();
+ let child_1 = create_widget();
+ child_1.children = Children::new(move || {
+ // We might need to clone again if child_2 has children that need this data?
+ // let some_data = some_data.clone();
+ // let child_2 = create_widget(some_data.foo); // OOPS error.
+ let child_2 = create_widget(some_data.foo);
+ })
+);
+```
+This is a pretty massive con in my opinion and one we should try to avoid.
+
+## Alternatives
+
+### 2. Ordered tree
+Another solution is to keep track of where the entity id's are spawned and make them consistent at their spawn point. Then we don't need to pass data down the tree only entity id's.
+
+Process:
+1. For each widget spawned add it to a new tree with it's parent set to the widget that spawned it.
+2. Make sure this ordered tree stays in sync with additions of entities.
+3. Pass children down as just id's that get added to the tree in their actual parent widget.
+
+So for our same example above with(a, b, c, d). Our tree looks like:
+
+```
+├── A
+| └── B
+| └── D
+| └── C
+```
+
+Our ordered tree looks like:
+```
+├── A
+| ├── B
+| └── D
+| └── C
+```
+
+Now with a lack of closures we only need to clone, copy, or move data when we specifically need to. When we re-render the tree a second time we look at our ordered tree first to see if we have a matching entity in the slot we are trying to render to.
+
+This sounds easy in practice but there are also some big cons:
+
+Cons:
+- Complexity increased.
+- Can cause issues if your tree and ordered tree get out of sync.s
\ No newline at end of file
diff --git a/kayak_ui_macros/src/widget.rs b/kayak_ui_macros/src/widget.rs
index b00c1ab..e6fda17 100644
--- a/kayak_ui_macros/src/widget.rs
+++ b/kayak_ui_macros/src/widget.rs
@@ -73,7 +73,6 @@ impl Widget {
(
entity_id.clone(),
quote! {{
- let parent_org = parent_id;
#props
#widget_block
#entity_id
@@ -184,8 +183,6 @@ impl Widget {
// });
let start = if !only_children {
quote! {
- let parent_id_old = parent_id;
- let parent_id = Some(#entity_id);
let mut children = KChildren::new();
}
} else {
@@ -198,7 +195,6 @@ impl Widget {
quote! {
// #prop_ident.children.despawn(&mut commands);
#prop_ident.children = children;
- let parent_id = parent_id_old;
}
} else {
quote! {}
@@ -217,7 +213,7 @@ impl Widget {
}
let props = quote! {
- let #entity_id = widget_context.spawn_widget(&mut commands, #entity_key, parent_org);
+ let #entity_id = widget_context.spawn_widget(&mut commands, #entity_key, parent_id);
let mut #prop_ident = #name {
#assigned_attrs
..Default::default()
diff --git a/src/children.rs b/src/children.rs
index d670c06..454e704 100644
--- a/src/children.rs
+++ b/src/children.rs
@@ -49,6 +49,10 @@ impl KChildren {
}
}
+ pub fn contains_entity(&self, entity: Entity) -> bool {
+ self.inner.iter().any(|e| *e == entity)
+ }
+
/// Processes all widgets and adds them to the tree.
pub fn process(
&self,
diff --git a/src/context.rs b/src/context.rs
index bfb1023..ab370a5 100644
--- a/src/context.rs
+++ b/src/context.rs
@@ -286,7 +286,8 @@ impl KayakRootContext {
if let Some(child) = child {
log::trace!(
- "Reusing widget entity {:?} with parent: {:?}!",
+ "Reusing child {} as widget entity {:?} with parent: {:?}!",
+ index,
child.index(),
parent_id.unwrap().index()
);
@@ -334,6 +335,15 @@ impl KayakRootContext {
tree.add(WrappedIndex(entity.unwrap()), parent_id.map(WrappedIndex))
}
}
+ } else if let Ok(mut tree) = self.order_tree.try_write() {
+ // let root_node = tree.root_node;
+ // if entity.map(WrappedIndex) != root_node {
+ // tree.add(entity.map(WrappedIndex).unwrap(), root_node);
+ // // Don't forget to advance indices or weird stuff can happen.
+ // if let Some(parent_entity) = root_node {
+ // self.get_and_add_index(parent_entity.0);
+ // }
+ // }
}
}
entity.unwrap()
@@ -780,7 +790,7 @@ fn update_widgets(
if should_update_children {
if let Ok(mut tree) = tree.write() {
let mut _had_removal = false;
- let diff = tree.diff_children(&widget_context, *entity, UPDATE_DEPTH);
+ let diff = tree.diff_children(&widget_context, *entity, 0);
for (_index, child, _parent, changes) in diff.changes.iter() {
if changes
.iter()
@@ -799,17 +809,141 @@ fn update_widgets(
}
}
- // if _had_removal {
- // tree.dump();
+ // let had_change = diff.has_changes();
+ // if had_change {
+ // tree.dump_at(*entity);
// dbg!(&diff);
// }
+ // Children of this node need to be despawned.
+ let mut despawn_list = Vec::default();
+ 'outer: for (_index, changed_entity, parent, changes) in diff.changes.iter() {
+ if changes.iter().any(|change| *change == Change::Inserted) {
+ if let Some(mut entity_commands) = world.get_entity_mut(changed_entity.0) {
+ entity_commands.insert(Mounted);
+ entity_commands.set_parent(parent.0);
+ }
+ if world.get_entity(changed_entity.0).is_some() {
+ if let Some(mut entity_commands) = world.get_entity_mut(parent.0) {
+ entity_commands.add_child(changed_entity.0);
+ }
+ }
+ } else if changes
+ .iter()
+ .any(|change| matches!(change, Change::Deleted))
+ {
+ // Remove from order tree.
+ // This is safe to do right away.
+ // if let Ok(mut order_tree) = order_tree.try_write() {
+ // order_tree.remove(*changed_entity);
+ // }
+ // If the child exists as a child of one of the children we do not need to remove it.
+ // TODO: This is kinda of expensive we should think of a way of making this faster..
+ if let Ok(order_tree) = order_tree.try_read() {
+ for child in order_tree.child_iter(*parent) {
+ if let Some(entity_ref) = world.get_entity(child.0) {
+ if let Some(children) = entity_ref.get::<KChildren>() {
+ if children.contains_entity(changed_entity.0) {
+ trace!("Caught an entity that was marked as deleted but wasn't! {:?}", changed_entity.0);
+ // Don't despawn changed entity because it exists as a child passed via props
+ continue 'outer;
+ }
+ }
+ }
+ }
+ }
+ for child in tree.down_iter_at(*changed_entity, true) {
+ trace!("Trying to remove: {:?} with parent {:?}", child.0, tree.parent(child));
+ // Due to a bug in bevy we need to remove the parent manually otherwise we'll panic later.
+ if let Some(mut entity_mut) = world.get_entity_mut(child.0) {
+ entity_mut.remove_parent();
+ }
+ if let Some(parent) = tree.parent(child) {
+ despawn_list.push((parent.0, child.0));
+ }
+
+ if let Ok(mut order_tree) = order_tree.try_write() {
+ order_tree.remove(child);
+ }
+ }
+ }
+ }
+
tree.merge(&widget_context, *entity, diff, UPDATE_DEPTH);
- // if _had_removal {
- // tree.dump();
+ // if had_change {
+ // tree.dump_at(*entity);
// }
+ for (parent, entity) in despawn_list.drain(..) {
+ // Clear out keyed entity.
+ if let (Ok(mut unique_ids), Ok(mut unique_ids_parents)) =
+ (unique_ids.try_write(), unique_ids_parents.try_write())
+ {
+ if let Some(parent) = unique_ids_parents.get(&entity) {
+ if let Some(keyed_hashmap) = unique_ids.get_mut(&parent) {
+ let possible_key = keyed_hashmap
+ .iter()
+ .find(|(_, keyed_entity)| **keyed_entity == entity)
+ .map(|(key, _)| key.clone());
+ if let Some(key) = possible_key {
+ keyed_hashmap.remove(&key);
+ unique_ids_parents.remove(&entity);
+ log::trace!("Removing key {key}, for entity: {:?}", entity);
+ }
+ }
+ }
+ }
+
+ // Remove state entity
+ if let Some(state_entity) = widget_state.remove(entity) {
+ if let Some(entity_mut) = world.get_entity_mut(state_entity) {
+ entity_mut.despawn_recursive();
+ }
+ }
+
+ // Remove widget entity
+ if let Some(entity_mut) = world.get_entity_mut(entity) {
+ log::trace!(
+ "Removing entity! {:?} - {:?} with parent {:?}",
+ entity.index(),
+ entity_mut.get::<WidgetName>(),
+ parent.index(),
+ // entity.index()
+ );
+ entity_mut.despawn();
+
+ // Also remove all cloned widget entities
+ if let Ok(cloned_widget_entities) = cloned_widget_entities.try_read() {
+ if let Some(entity) = cloned_widget_entities.get(&entity) {
+ world.despawn(*entity);
+ }
+ }
+ }
+ }
+
+ // if should_update_children {
+ if let Ok(cloned_widget_entities) = cloned_widget_entities.try_read() {
+ if let Some(target_entity) = cloned_widget_entities.get(&entity.0) {
+ if let Some(styles) = world.entity(entity.0).get::<KStyle>().cloned() {
+ if let Some(mut entity) = world.get_entity_mut(*target_entity) {
+ entity.insert(styles);
+ }
+ }
+ if let Some(styles) = world.entity(entity.0).get::<ComputedStyles>().cloned() {
+ if let Some(mut entity) = world.get_entity_mut(*target_entity) {
+ entity.insert(styles);
+ }
+ }
+ if let Some(children) = world.entity(entity.0).get::<KChildren>().cloned() {
+ if let Some(mut entity) = world.get_entity_mut(*target_entity) {
+ entity.insert(children);
+ }
+ }
+ }
+ }
+
+ // Mark nodes left as dirty.
for child in widget_context.child_iter(*entity) {
if let Some(mut entity_commands) = world.get_entity_mut(child.0) {
entity_commands.insert(DirtyNode);
@@ -849,25 +983,49 @@ fn update_widgets(
} else {
// In this case the entity we are trying to process no longer exists.
// The approach taken here removes said entities from the tree.
- let mut despawn_list = Vec::default();
- if let Ok(mut tree) = tree.write() {
- for child in tree.down_iter_at(*entity, true) {
- despawn_list.push(child.0);
- if let Ok(mut order_tree) = order_tree.try_write() {
- // had_removal = true;
- log::trace!(
- "Removing entity! {:?} inside of: {:?}",
- child.0.index(),
- entity.0.index()
- );
- order_tree.remove(child);
+
+ // If the child exists as a child of one of the children we do not need to remove it.
+ // TODO: This is kinda of expensive we should think of a way of making this faster..
+ let mut contained_in_children = false;
+ if let Ok(order_tree) = order_tree.try_read() {
+ if let Some(parent) = order_tree.parent(*entity) {
+ trace!("Had parent for: {}", entity.0.index());
+ for child in order_tree.child_iter(parent) {
+ if let Some(entity_ref) = world.get_entity(child.0) {
+ if let Some(children) = entity_ref.get::<KChildren>() {
+ if children.contains_entity(entity.0) {
+ trace!("Caught an entity that was marked as dangling but wasn't! {:?}", entity.0);
+ contained_in_children = true;
+ // Don't despawn changed entity because it exists as a child passed via props
+ break;
+ }
+ }
+ }
}
}
+ }
+
+ if !contained_in_children {
+ let mut despawn_list = Vec::default();
+ if let Ok(mut tree) = tree.write() {
+ for child in tree.down_iter_at(*entity, true) {
+ despawn_list.push(child.0);
+ if let Ok(mut order_tree) = order_tree.try_write() {
+ // had_removal = true;
+ log::trace!(
+ "Removing dangling entity! {:?} inside of: {:?}",
+ child.0.index(),
+ entity.0.index()
+ );
+ order_tree.remove(child);
+ }
+ }
- for entity in despawn_list.drain(..) {
- tree.remove(WrappedIndex(entity));
- if let Some(entity_mut) = world.get_entity_mut(entity) {
- entity_mut.despawn();
+ for entity in despawn_list.drain(..) {
+ tree.remove(WrappedIndex(entity));
+ if let Some(entity_mut) = world.get_entity_mut(entity) {
+ entity_mut.despawn();
+ }
}
}
}
@@ -1055,13 +1213,13 @@ fn update_widget(
commands.entity(entity.0).remove::<Mounted>();
- let diff = if let Ok(tree) = tree.read() {
- tree.diff_children(&widget_context, entity, 0)
- } else {
- panic!("Failed to acquire read lock.");
- };
+ // let diff = if let Ok(tree) = tree.read() {
+ // tree.diff_children(&widget_context, entity, 0)
+ // } else {
+ // panic!("Failed to acquire read lock.");
+ // };
- log::trace!("Entity: {:?}, Diff: {:?}", entity.0, &diff);
+ // log::trace!("Entity: {:?}, Diff: {:?}", entity.0, &diff);
// Always mark widget dirty if it's re-rendered.
// Mark node as needing a recalculation of rendering/layout.
@@ -1069,121 +1227,11 @@ fn update_widget(
command_queue.apply(world);
- // Children of this node need to be despawned.
- let mut despawn_list = Vec::default();
-
- for (_index, changed_entity, parent, changes) in diff.changes.iter() {
- if changes.iter().any(|change| *change == Change::Inserted) {
- if let Some(mut entity_commands) = world.get_entity_mut(changed_entity.0) {
- entity_commands.insert(Mounted);
- entity_commands.set_parent(parent.0);
- }
- if world.get_entity(changed_entity.0).is_some() {
- if let Some(mut entity_commands) = world.get_entity_mut(parent.0) {
- entity_commands.add_child(changed_entity.0);
- }
- }
- } else if changes
- .iter()
- .any(|change| matches!(change, Change::Deleted))
- {
- if let Ok(tree) = tree.try_read() {
- for child in tree.down_iter_at(*changed_entity, true) {
- // Due to a bug in bevy we need to remove the parent manually otherwise we'll panic later.
- world.entity_mut(child.0).remove_parent();
-
- // let children = if let Some(parent) = tree.get_parent(child) {
- // world.entity(parent.0).get::<KChildren>()
- // } else {
- // None
- // };
-
- let parent = tree.parent(child).unwrap();
-
- // if let Some(children) = children {
- // for child in children.iter() {
- // despawn_list.push((parent.0, *child));
- // }
- // } //else {
-
- despawn_list.push((parent.0, child.0));
-
- // }
- if let Ok(mut order_tree) = order_tree.try_write() {
- order_tree.remove(child);
- }
- }
- }
- }
- }
-
- for (parent, entity) in despawn_list.drain(..) {
- // Clear out keyed entity.
- if let (Ok(mut unique_ids), Ok(mut unique_ids_parents)) =
- (unique_ids.try_write(), unique_ids_parents.try_write())
- {
- if let Some(parent) = unique_ids_parents.get(&entity) {
- if let Some(keyed_hashmap) = unique_ids.get_mut(&parent) {
- let possible_key = keyed_hashmap
- .iter()
- .find(|(_, keyed_entity)| **keyed_entity == entity)
- .map(|(key, _)| key.clone());
- if let Some(key) = possible_key {
- keyed_hashmap.remove(&key);
- unique_ids_parents.remove(&entity);
- log::trace!("Removing key {key}, for entity: {:?}", entity);
- }
- }
- }
- }
-
- // Remove state entity
- if let Some(state_entity) = widget_state.remove(entity) {
- if let Some(entity_mut) = world.get_entity_mut(state_entity) {
- entity_mut.despawn_recursive();
- }
- }
-
- // Remove widget entity
- if let Some(entity_mut) = world.get_entity_mut(entity) {
- log::trace!(
- "Removing entity! {:?} - {:?} with parent {:?}",
- entity.index(),
- entity_mut.get::<WidgetName>(),
- parent.index(),
- // entity.index()
- );
- entity_mut.despawn();
-
- // Also remove all cloned widget entities
- if let Ok(cloned_widget_entities) = cloned_widget_entities.try_read() {
- if let Some(entity) = cloned_widget_entities.get(&entity) {
- world.despawn(*entity);
- }
- }
- }
- }
+ // if let Ok(tree) = order_tree.try_read() {
+ // tree.dump_all();
+ // }
- // if should_update_children {
- if let Ok(cloned_widget_entities) = cloned_widget_entities.try_read() {
- if let Some(target_entity) = cloned_widget_entities.get(&entity.0) {
- if let Some(styles) = world.entity(entity.0).get::<KStyle>().cloned() {
- if let Some(mut entity) = world.get_entity_mut(*target_entity) {
- entity.insert(styles);
- }
- }
- if let Some(styles) = world.entity(entity.0).get::<ComputedStyles>().cloned() {
- if let Some(mut entity) = world.get_entity_mut(*target_entity) {
- entity.insert(styles);
- }
- }
- if let Some(children) = world.entity(entity.0).get::<KChildren>().cloned() {
- if let Some(mut entity) = world.get_entity_mut(*target_entity) {
- entity.insert(children);
- }
- }
- }
- }
+
// for (_, child_entity, _, changes) in diff.changes.iter() {
// // Clone to entity.
// if changes.iter().any(|change| *change == Change::Deleted) {
diff --git a/src/tree.rs b/src/tree.rs
index d89a74b..757c531 100644
--- a/src/tree.rs
+++ b/src/tree.rs
@@ -554,7 +554,7 @@ impl Tree {
for (id, node, parent_node, change) in changes.changes.iter() {
match change.as_slice() {
[Change::Deleted] => {
- // self.parents.remove(node);
+ self.parents.remove(node);
if children_a.get(*id).is_some() {
children_a[*id] = WrappedIndex(Entity::from_raw(0));
}
@@ -633,6 +633,21 @@ impl Tree {
}
}
+ /// Sometimes we need to see the entire tree even dangling nodes.
+ /// This function will display everything.
+ pub fn dump_all(&self) {
+ let mut children = self.children.iter().collect::<Vec<_>>();
+ children.sort_by(|(a, _), (b, _)| a.0.index().partial_cmp(&b.0.index()).unwrap());
+
+ for (parent, children) in children.iter() {
+ println!("[{}]", parent.0.index());
+ for child in children.iter() {
+ println!(" [{}]", child.0.index());
+ }
+ println!("");
+ }
+ }
+
/// Dumps a section of the tree's current state to the console (starting from a specific index)
///
/// To dump the entire tree, use [dump] instead.
@@ -750,7 +765,7 @@ impl<'a> Iterator for DownwardIterator<'a> {
}
}
- self.current_node
+ None
}
}
diff --git a/src/widget_context.rs b/src/widget_context.rs
index b61c910..146af55 100644
--- a/src/widget_context.rs
+++ b/src/widget_context.rs
@@ -25,7 +25,7 @@ pub struct KayakWidgetContext {
layout_cache: Arc<RwLock<LayoutCache>>,
pub(crate) index: Arc<RwLock<HashMap<Entity, usize>>>,
widget_state: WidgetState,
- order_tree: Arc<RwLock<Tree>>,
+ pub(crate) order_tree: Arc<RwLock<Tree>>,
pub camera_entity: Option<Entity>,
// Unique id's store entity id's related to a key rather than the child tree.
// This lets users get a unique entity. The first Entity is the parent widget.
@@ -233,7 +233,8 @@ impl KayakWidgetContext {
if let Some(child) = child {
log::trace!(
- "Reusing widget entity {:?} with parent: {:?}!",
+ "Reusing widget at index: {}, entity {:?} with parent: {:?}!",
+ index,
child.index(),
parent_id.unwrap().index()
);
@@ -300,6 +301,18 @@ impl KayakWidgetContext {
if let Some(parent) = parent {
assert!(parent != entity, "Parent cannot equal entity!");
}
+
+ // Sometimes we need to remove the old entity from the tree.
+ if let Ok(mut old_tree) = self.old_tree.write() {
+ if let Some(parent) = parent.map(WrappedIndex) {
+ if let Some(old_parent) = old_tree.parent(WrappedIndex(entity)) {
+ if old_parent != parent {
+ old_tree.remove(WrappedIndex(entity));
+ }
+ }
+ }
+ }
+
if let Ok(mut tree) = self.new_tree.write() {
tree.add(WrappedIndex(entity), parent.map(WrappedIndex));
}
@@ -337,9 +350,14 @@ impl KayakWidgetContext {
/// Dumps the tree to the console in a human readable format.
/// This is relatively slow to do if the tree is large
/// so avoid doing unless necessary.
- pub fn dbg_tree(&self) {
+ pub fn dbg_tree(&self, entity: Entity) {
+ println!("New:");
if let Ok(tree) = self.new_tree.read() {
- tree.dump()
+ tree.dump_at(WrappedIndex(entity));
+ }
+ println!("Old:");
+ if let Ok(tree) = self.old_tree.read() {
+ tree.dump_at(WrappedIndex(entity));
}
}
diff --git a/src/widgets/app.rs b/src/widgets/app.rs
index 3020746..370891c 100644
--- a/src/widgets/app.rs
+++ b/src/widgets/app.rs
@@ -111,7 +111,6 @@ pub fn app_render(
})
.with_style(app_style)
.into();
-
let parent_id = Some(entity);
rsx! {
<ClipBundle
--
GitLab