-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Support non-Vec data structures in relations #17447
Support non-Vec data structures in relations #17447
Conversation
EntityHashSet
in relations… by moving the trait bound to methods that need it
Was enabled at project level, but not for the individual crate!
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.
Nice!
/// 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 comment
The 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 comment
The 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.
/// Returns the number of elements in the set. | ||
pub fn len(&self) -> usize { | ||
self.0.len() | ||
} | ||
|
||
/// Returns `true` if the set contains no elements. | ||
pub fn is_empty(&self) -> bool { | ||
self.0.is_empty() | ||
} | ||
|
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 Deref
to 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 Deref
impl was failing for these, as the compiler was getting confused about the circular trait methods due to the same name / signature.
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 len
is an inherent method on self
, then self.len()
in a len()
trait method impl works, but if the inherent len()
is down a step in the deref chain, the trait method on self
resolves first, resulting in a cycle.
The solution should be any of self.0.len()
/(*self).len()
/self.deref().len()
in the trait method impl.
It looks like your PR is a breaking change, but you didn't provide a migration guide. Could you add some context on what users should update when this change get released in a new version of Bevy? |
Objective
The existing
RelationshipSourceCollection
usesVec
as the only possible backing for our relationships. While a reasonable choice, benchmarking use cases might reveal that a different data type is better or faster.For example:
Vec
.smallvec
may be fasterSolution
RelationshipSourceCollection
forEntityHashSet
, our custom entity-optimizedHashSet
.-
ImplementDoubleEndedIterator
forEntityHashSet
to make things compile.RelationshipSourceCollection
from an erased RPTIT to an explicit associated type we can add a trait bound on the offending methods!RelationshipSourceCollection
forSmallVec
Testing
I've added a pair of new tests to make sure this pattern compiles successfully in practice!
Migration Guide
EntityHashSet
andEntityHashMap
are no longer re-exported inbevy_ecs::entity
directly. If you were not usingbevy_ecs
/bevy
'sprelude
, you can access them through their now-public modules,hash_set
andhash_map
instead.Notes to reviewers
The
EntityHashSet::Iter
type needs to be public for this impl to be allowed. I initially renamed it to something that wasn't ambiguous and re-exported it, but as @Victoronz pointed out, that was somewhat unidiomatic.In 1a85648, I instead made the
entity_hash_set
public (and itsentity_hash_set
) sister public, and removed the re-export. I prefer this design (give me module docs please), but it leads to a lot of churn in this PR.Let me know which you'd prefer, and if you'd like me to split that change out into its own micro PR.