-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Support non-Vec data structures in relations #17447
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
Changes from all commits
feedfa5
241feb9
36efe63
e4add92
0955a15
9221e27
4df86f6
1a85648
6e22d27
e8f2df9
556cdf3
48b6b30
ea121ab
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 |
|---|---|---|
| @@ -1,9 +1,21 @@ | ||
| use crate::entity::Entity; | ||
| use crate::entity::{hash_set::EntityHashSet, Entity}; | ||
| use alloc::vec::Vec; | ||
| use smallvec::SmallVec; | ||
|
|
||
| /// The internal [`Entity`] collection used by a [`RelationshipTarget`](crate::relationship::RelationshipTarget) component. | ||
| /// This is not intended to be modified directly by users, as it could invalidate the correctness of relationships. | ||
| pub trait RelationshipSourceCollection { | ||
| /// The type of iterator returned by the `iter` method. | ||
| /// | ||
| /// This is an associated type (rather than using a method that returns an opaque return-position impl trait) | ||
| /// to ensure that all methods and traits (like [`DoubleEndedIterator`]) of the underlying collection's iterator | ||
| /// are available to the user when implemented without unduly restricting the possible collections. | ||
| /// | ||
| /// The [`SourceIter`](super::SourceIter) type alias can be helpful to reduce confusion when working with this associated type. | ||
| type SourceIter<'a>: Iterator<Item = Entity> | ||
|
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. This is definitely the right path forward. RPIT is great 90% of the time, but this is that 10% case. 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. It's a mouthful, but I really think that the added flexibility / correctness is worth it here. |
||
| where | ||
| Self: 'a; | ||
|
|
||
| /// Returns an instance with the given pre-allocated entity `capacity`. | ||
| fn with_capacity(capacity: usize) -> Self; | ||
|
|
||
|
|
@@ -14,7 +26,7 @@ pub trait RelationshipSourceCollection { | |
| fn remove(&mut self, entity: Entity); | ||
|
|
||
| /// Iterates all entities in the collection. | ||
| fn iter(&self) -> impl DoubleEndedIterator<Item = Entity>; | ||
| fn iter(&self) -> Self::SourceIter<'_>; | ||
|
|
||
| /// Returns the current length of the collection. | ||
| fn len(&self) -> usize; | ||
|
|
@@ -27,6 +39,8 @@ pub trait RelationshipSourceCollection { | |
| } | ||
|
|
||
| impl RelationshipSourceCollection for Vec<Entity> { | ||
| type SourceIter<'a> = core::iter::Copied<core::slice::Iter<'a, Entity>>; | ||
|
|
||
| fn with_capacity(capacity: usize) -> Self { | ||
| Vec::with_capacity(capacity) | ||
| } | ||
|
|
@@ -41,11 +55,134 @@ impl RelationshipSourceCollection for Vec<Entity> { | |
| } | ||
| } | ||
|
|
||
| fn iter(&self) -> impl DoubleEndedIterator<Item = Entity> { | ||
| fn iter(&self) -> Self::SourceIter<'_> { | ||
| <[Entity]>::iter(self).copied() | ||
| } | ||
|
|
||
| fn len(&self) -> usize { | ||
| Vec::len(self) | ||
| } | ||
| } | ||
|
|
||
| impl RelationshipSourceCollection for EntityHashSet { | ||
| type SourceIter<'a> = core::iter::Copied<crate::entity::hash_set::Iter<'a>>; | ||
|
|
||
| fn with_capacity(capacity: usize) -> Self { | ||
| EntityHashSet::with_capacity(capacity) | ||
| } | ||
|
|
||
| fn add(&mut self, entity: Entity) { | ||
| self.insert(entity); | ||
| } | ||
|
|
||
| fn remove(&mut self, entity: Entity) { | ||
| // We need to call the remove method on the underlying hash set, | ||
| // which takes its argument by reference | ||
| self.0.remove(&entity); | ||
| } | ||
|
|
||
| fn iter(&self) -> Self::SourceIter<'_> { | ||
| self.iter().copied() | ||
| } | ||
|
|
||
| fn len(&self) -> usize { | ||
| self.len() | ||
| } | ||
| } | ||
|
|
||
| impl<const N: usize> RelationshipSourceCollection for SmallVec<[Entity; N]> { | ||
| type SourceIter<'a> = core::iter::Copied<core::slice::Iter<'a, Entity>>; | ||
|
|
||
| fn with_capacity(capacity: usize) -> Self { | ||
| SmallVec::with_capacity(capacity) | ||
| } | ||
|
|
||
| fn add(&mut self, entity: Entity) { | ||
| SmallVec::push(self, entity); | ||
| } | ||
|
|
||
| fn remove(&mut self, entity: Entity) { | ||
| if let Some(index) = <[Entity]>::iter(self).position(|e| *e == entity) { | ||
| SmallVec::remove(self, index); | ||
| } | ||
| } | ||
|
|
||
| fn iter(&self) -> Self::SourceIter<'_> { | ||
| <[Entity]>::iter(self).copied() | ||
| } | ||
|
|
||
| fn len(&self) -> usize { | ||
| SmallVec::len(self) | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use crate as bevy_ecs; | ||
| use crate::prelude::{Component, World}; | ||
| use crate::relationship::RelationshipTarget; | ||
|
|
||
| #[test] | ||
| fn vec_relationship_source_collection() { | ||
| #[derive(Component)] | ||
| #[relationship(relationship_target = RelTarget)] | ||
| struct Rel(Entity); | ||
|
|
||
| #[derive(Component)] | ||
| #[relationship_target(relationship = Rel, despawn_descendants)] | ||
| struct RelTarget(Vec<Entity>); | ||
|
|
||
| let mut world = World::new(); | ||
| let a = world.spawn_empty().id(); | ||
| let b = world.spawn_empty().id(); | ||
|
|
||
| world.entity_mut(a).insert(Rel(b)); | ||
|
|
||
| let rel_target = world.get::<RelTarget>(b).unwrap(); | ||
| let collection = rel_target.collection(); | ||
| assert_eq!(collection, &alloc::vec!(a)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn entity_hash_set_relationship_source_collection() { | ||
| #[derive(Component)] | ||
| #[relationship(relationship_target = RelTarget)] | ||
| struct Rel(Entity); | ||
|
|
||
| #[derive(Component)] | ||
| #[relationship_target(relationship = Rel, despawn_descendants)] | ||
| struct RelTarget(EntityHashSet); | ||
|
|
||
| let mut world = World::new(); | ||
| let a = world.spawn_empty().id(); | ||
| let b = world.spawn_empty().id(); | ||
|
|
||
| world.entity_mut(a).insert(Rel(b)); | ||
|
|
||
| let rel_target = world.get::<RelTarget>(b).unwrap(); | ||
| let collection = rel_target.collection(); | ||
| assert_eq!(collection, &EntityHashSet::from([a])); | ||
| } | ||
|
|
||
| #[test] | ||
| fn smallvec_relationship_source_collection() { | ||
| #[derive(Component)] | ||
| #[relationship(relationship_target = RelTarget)] | ||
| struct Rel(Entity); | ||
|
|
||
| #[derive(Component)] | ||
| #[relationship_target(relationship = Rel, despawn_descendants)] | ||
| struct RelTarget(SmallVec<[Entity; 4]>); | ||
|
|
||
| let mut world = World::new(); | ||
| let a = world.spawn_empty().id(); | ||
| let b = world.spawn_empty().id(); | ||
|
|
||
| world.entity_mut(a).insert(Rel(b)); | ||
|
|
||
| let rel_target = world.get::<RelTarget>(b).unwrap(); | ||
| let collection = rel_target.collection(); | ||
| assert_eq!(collection, &SmallVec::from_buf([a])); | ||
| } | ||
| } | ||
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.
Are these not available via the
Derefto the inner hashbrown set?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.
Just relying on the
Derefimpl was failing for these, as the compiler was getting confused about the circular trait methods due to the same name / signature.Uh oh!
There was an error while loading. Please reload this page.
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.
ah, that is because of method resolution!
when
lenis an inherent method onself, thenself.len()in alen()trait method impl works, but if the inherentlen()is down a step in the deref chain, the trait method onselfresolves first, resulting in a cycle.The solution should be any of
self.0.len()/(*self).len()/self.deref().len()in the trait method impl.