Make entity generation a new type and remove identifier #19121
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ElliottjPierce
added
A-ECS
bushrat011899
approved these changes
May 7, 2025
|
|
||
| /// Returns the [`EntityGeneration`] that would result from this many more `versions` of the corresponding [`EntityRow`] from passing. | ||
| #[inline] | ||
| pub const fn after_versions(self, versions: u32) -> Self { |
There was a problem hiding this comment.
Future PR: might be nice to implement Iterator<Item = Self> for EntityGeneration. That gives an idiomatic next method.
atlv24
approved these changes
May 7, 2025
atlv24
added
S-Ready-For-Final-Review
manokara
reviewed
May 7, 2025
Co-authored-by: Mark Nokalt <[email protected]>
|
Benchmarks looks good, sparse set change detection was the only thing that was slowed down by anything notable. But I don't think that's a big deal. Other than that, I think this is generally an improvement in perf. (Less checks when changing the generation.) Also want to point out that we loose a niche on |
This was referenced May 8, 2025
andrewzhurov
pushed a commit
to andrewzhurov/bevy
that referenced
this pull request
May 17, 2025
…9121) # Objective This is a followup to bevyengine#18704 . There's lots more followup work, but this is the minimum to unblock bevyengine#18670, etc. This direction has been given the green light by Alice [here](bevyengine#18704 (comment)). ## Solution I could have split this over multiple PRs, but I figured skipping straight here would be easiest for everyone and would unblock things the quickest. This removes the now no longer needed `identifier` module and makes `Entity::generation` go from `NonZeroU32` to `struct EntityGeneration(u32)`. ## Testing CI --------- Co-authored-by: Mark Nokalt <[email protected]>
github-merge-queue bot
pushed a commit
that referenced
this pull request
May 29, 2025
# Objective Recently the `u32` `Entity::generation` was replaced with the new `EntityGeneration` in #19121. This made meanings a lot more clear, and prevented accidental misuse. One common misuse was assuming that `u32`s that were greater than others came after those others. Wrapping makes this assumption false. When `EntityGeneration` was created, it retained the `u32` ordering, which was useless at best and wrong at worst. This pr fixes the ordering implementation, so new generations are greater than older generations. Some users were already accounting for this ordering issue (which was still present in 0.16 and before) by manually accessing the `u32` representation. This made migrating difficult for avian physics; see [here](https://discord.com/channels/691052431525675048/749335865876021248/1377431569228103780). I am generally of the opinion that this type should be kept opaque to prevent accidental misuse. As we find issues like this, the functionality should be added to `EntityGeneration` directly. ## Solution Fix the ordering implementation through `Ord`. Alternatively, we could keep `Ord` the same and make a `cmp_age` method, but I think this is better, even though sorting entity ids may be *marginally* slower now (but more correct). This is a tradeoff. ## Testing I improved documentation for aliasing and ordering, adding some doc tests.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
This is a followup to #18704 . There's lots more followup work, but this is the minimum to unblock #18670, etc.
This direction has been given the green light by Alice here.
Solution
I could have split this over multiple PRs, but I figured skipping straight here would be easiest for everyone and would unblock things the quickest.
This removes the now no longer needed
identifiermodule and makesEntity::generationgo fromNonZeroU32tostruct EntityGeneration(u32).Testing
CI