Enforce uniqueness in Children #1079
Conversation
| let desired_index = order.len() - 1; | ||
| let current_index = uniqueness[&entity]; | ||
| if current_index != desired_index { | ||
| self.swap(current_index, desired_index); |
There was a problem hiding this comment.
Example order is [1,2,3,4] and I push(2). Would the result be [1,4,3,2]?
There was a problem hiding this comment.
I would need to see some tests for this Children component. It is not entirely trivial anymore.
There was a problem hiding this comment.
There are a couple of new tests in bevy_transform/src/hierarchy/child_builder.rs, but you are correct they should be closer to the actual code.
There was a problem hiding this comment.
Also, you are right about the order. My goal is to have the entity be in the same place in the list whether or not it already existed in the set. So if you reinsert an existing entity it will get moved to a new position. I'm not certain that that's the right behavior, I think you can make a strong case for the entity retaining it's old position.
There was a problem hiding this comment.
I think its good to move the existing entity to the new position, but this should not jump an unrelated entity to another position. In the example I push(2) to the end but 4 jumps from the end to index 1 were the original entity 2 was before to make room for the new entity 2.
|
I talked this issue over with @payload and I think there are some interesting questions about what the best approach to this problem is. Their version potentially requires no allocations and it uses the same amount of memory to store Children as the current implementation. This version requires at least one allocation per Children struct and stores a HashMap as well is the current SmalVec, at least tripling the memory usage. Their version requires a scan through the existing children for every child added to the set which is potentially expensive for large sets. This version uses an incrementally built HashMap so it should have fairly stable performance as the number of children grows, however adding and removing children is more expensive per child so it will likely have worse performance when the sets are small. I think performance with small numbers of children will be fine either way, just because N is small so what matters is performance when N is large where I think this version is preferable (except in terms of memory usage). @payload points out an interesting case where the number of children per Children struct is small but the number of Children structs being created and destroyed is large, forcing this version to do many allocations. The best possible approach is probably to use a simple scan through a SmallVec when the number of children is small and switch to hashing if the number of children goes over some empirically determined threshold but I don't think the additional complexity of that is justified unless we see evidence of performance problems. |
Moxinilian
added
A-ECS
|
I finally brought over the tests from #1076 (thank you @payload) and fixed a bug they revealed (thank you again @payload). I removed one test which asserts a different behavior than what the current implementation (and this PR) have (insert_children_out_of_bounds_pushes_to_end) and two which I think are out of scope for this change (insert_children_adds_previous_parent_component and push_children_updates_previous_parent). All of those may be valuable to do outside fo this PR. |
alec-deason
force-pushed
the
make_children_unique
branch
from
859db08 to
f4c3dd7
Compare
|
Cart has mentioned that he's working on a fundamental redesign of this system so I suspect this PR is unnecessary but I'll leave it open until there's more news on that redesign. |
|
Cool cool. I should hopefully have something to share soon. The direction I'm currently headed in would involve adding alternative component storages for fast re-parenting. Its a rabbit hole, but it also opens up marker components as a viable pattern, so maybe worth investing in now. |
alice-i-cecile
added
the
A-UI
cart
added
the
S-Pre-Relicense
DJMcNab
removed
the
S-Pre-Relicense
alice-i-cecile
added
the
S-Needs-Design-Doc
| #[reflect(Component, MapEntities)] | ||
| pub struct Children(pub(crate) SmallVec<[Entity; 8]>); | ||
| pub struct Children { | ||
| order: SmallVec<[Entity; 8]>, |
There was a problem hiding this comment.
IMO we should consider pulling in smolset to do this for us, rather than worrying about enforcing uniqueness ourselves. I've been poking at this space for storing input maps too.
alice-i-cecile
added
A-Hierarchy
alice-i-cecile
added
X-Controversial
alice-i-cecile
added
the
S-Needs-Benchmarking
|
I'm closing this out now that #4197 has landed, which should fix this problem :) |
This changes the Children container to guarantee uniqueness while still keeping an order and allowing reordering.
I have taken the stance that if an Entity is re-inserted it should be moved to the position in the list it would have occupied had it actually been inserted, which potentially repositions other children. I'm not 100% sure what the least surprising behavior is. I think you could make a case that a re-inserted child should stay it it's old position instead.
This change makes operating on children slightly more expensive (except in the case of membership tests, which get cheaper). I haven't done any benchmarking but I suspect the performance changes only become relevant when an application is doing a very large amount of hierarchy manipulation. I've chosen simpler implementations in favor of more optimal one in several places so there is some clear opportunity for improvement.
This passes all existing tests and adds a small amount of additional testing specific to child duplication. It runs my personal project, which does quite a bit of hierarchy manipulation, without any problem.
@payload has a different solution to this problem in #1076 which includes more extensive tests. Even if we choose to use this version of the change I think it's worth bringing over some of @palyoad's tests.