Skip to content
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

Merged
merged 13 commits into from
Jan 20, 2025

Conversation

alice-i-cecile
Copy link
Member

@alice-i-cecile alice-i-cecile commented Jan 20, 2025

Objective

The existing RelationshipSourceCollection uses Vec 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:

  • Not all relationships require a stable ordering between the relationship sources (i.e. children). In cases where we a) have many such relations and b) don't care about the ordering between them, a hash set is likely a better datastructure than a Vec.
  • The number of children-like entities may be small on average, and a smallvec may be faster

Solution

  • Implement RelationshipSourceCollection for EntityHashSet, our custom entity-optimized HashSet.
    -Implement DoubleEndedIterator for EntityHashSet to make things compile.
    • This implementation was cursed and very surprising.
    • Instead, by moving the iterator type on RelationshipSourceCollection from an erased RPTIT to an explicit associated type we can add a trait bound on the offending methods!
  • Implement RelationshipSourceCollection for SmallVec

Testing

I've added a pair of new tests to make sure this pattern compiles successfully in practice!

Migration Guide

EntityHashSet and EntityHashMap are no longer re-exported in bevy_ecs::entity directly. If you were not using bevy_ecs / bevy's prelude, you can access them through their now-public modules, hash_set and hash_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 its entity_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.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jan 20, 2025
@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Jan 20, 2025
@alice-i-cecile alice-i-cecile changed the title Support using EntityHashSet in relations Support using other data structures in relations Jan 20, 2025
@alice-i-cecile alice-i-cecile changed the title Support using other data structures in relations Support non-Vec data structures in relations Jan 20, 2025
@alice-i-cecile alice-i-cecile added the D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes label Jan 20, 2025
Was enabled at project level, but not for the individual crate!
@alice-i-cecile alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jan 20, 2025
@alice-i-cecile alice-i-cecile marked this pull request as ready for review January 20, 2025 04:32
Copy link
Contributor

@bushrat011899 bushrat011899 left a 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>
Copy link
Contributor

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.

Copy link
Member Author

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.

Comment on lines +45 to +54
/// 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()
}

Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

@Victoronz Victoronz Jan 20, 2025

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.

crates/bevy_ecs/src/entity/hash_set.rs Outdated Show resolved Hide resolved
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 20, 2025
Copy link
Contributor

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?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 20, 2025
Merged via the queue into bevyengine:main with commit 5a9bc28 Jan 20, 2025
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants