-
-
Notifications
You must be signed in to change notification settings - Fork 4k
EntityGeneration
ordering
#19421
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
EntityGeneration
ordering
#19421
Conversation
I do think we need better docs here, and supporting the use case of older vs newer generation comparison should be done, however I think we should be careful about adding meaning to this But those fields are both But if we make the default order of |
I don't actually think this is an issue. If we openly state "There is a default order, but it has no guaranteed meaning" for So I don't see an issue here; though, granted, it is unconventional. That said, you do make a compelling argument. I started writing an "I agree" comment before I realized the "There is a default order, but it has no guaranteed meaning" stance itself made room for this. So if others disagree, I'd be happy to back track and make |
I've looked at the docs for The question of "What do we do about sorted tables/iteration order/entity order" seems to accumulate an ever-growing pile of implementation quirks to deal with! Even if the discrepancy is a non-issue right now, I suspect it will resurface there later. It would seem that we'll then have to live with either an inconsistency or a performance hit somewhere. That being said, I wonder how much actual cost it would carry to switch over the comparison behavior of |
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.
Ultimately, I think we can do this, given we both the public non-guarantee, and an internal word of caution is documented.
It might lead to some unfortunate performance hits to maintain consistency later on, but I think the proper generation behavior/semantics should come before that.
Finally, if it does become an issue, we can still decide otherwise down the line!
I could not agree more. We may need to revisit this later. The choice is between fast and meaningless or slow(er) and meaningful. And we'll have to solidify and document that choice at some point. I would personally think "fast and meaningless" is better. That way, if someone wants a particular ordering, they can absolutely do that by getting the inner row and generation, and the cost of that would be transparent. But that's just me. Something to think about... |
…lone method (#19432) # Objective #19421 implemented `Ord` for `EntityGeneration` along the lines of [the impl from slotmap](https://docs.rs/slotmap/latest/src/slotmap/util.rs.html#8): ```rs /// Returns if a is an older version than b, taking into account wrapping of /// versions. pub fn is_older_version(a: u32, b: u32) -> bool { let diff = a.wrapping_sub(b); diff >= (1 << 31) } ``` But that PR and the slotmap impl are different: **slotmap impl** - if `(1u32 << 31)` is greater than `a.wrapping_sub(b)`, then `a` is older than `b` - if `(1u32 << 31)` is equal to `a.wrapping_sub(b)`, then `a` is older than `b` - if `(1u32 << 31)` is less than `a.wrapping_sub(b)`, then `a` is equal or newer than `b` **previous PR impl** - if `(1u32 << 31)` is greater than `a.wrapping_sub(b)`, then `a` is older than `b` - if `(1u32 << 31)` is equal to `a.wrapping_sub(b)`, then `a` is equal to `b`⚠️ - if `(1u32 << 31)` is less than `a.wrapping_sub(b)`, then `a` is newer than `b`⚠️ This ordering is also not transitive, therefore it should not implement `PartialOrd`. ## Solution Fix the impl in a standalone method, remove the `Partialord`/`Ord` implementation. ## Testing Given the first impl was wrong and got past reviews, I think a new unit test is justified.
…lone method (bevyengine#19432) # Objective bevyengine#19421 implemented `Ord` for `EntityGeneration` along the lines of [the impl from slotmap](https://docs.rs/slotmap/latest/src/slotmap/util.rs.html#8): ```rs /// Returns if a is an older version than b, taking into account wrapping of /// versions. pub fn is_older_version(a: u32, b: u32) -> bool { let diff = a.wrapping_sub(b); diff >= (1 << 31) } ``` But that PR and the slotmap impl are different: **slotmap impl** - if `(1u32 << 31)` is greater than `a.wrapping_sub(b)`, then `a` is older than `b` - if `(1u32 << 31)` is equal to `a.wrapping_sub(b)`, then `a` is older than `b` - if `(1u32 << 31)` is less than `a.wrapping_sub(b)`, then `a` is equal or newer than `b` **previous PR impl** - if `(1u32 << 31)` is greater than `a.wrapping_sub(b)`, then `a` is older than `b` - if `(1u32 << 31)` is equal to `a.wrapping_sub(b)`, then `a` is equal to `b`⚠️ - if `(1u32 << 31)` is less than `a.wrapping_sub(b)`, then `a` is newer than `b`⚠️ This ordering is also not transitive, therefore it should not implement `PartialOrd`. ## Solution Fix the impl in a standalone method, remove the `Partialord`/`Ord` implementation. ## Testing Given the first impl was wrong and got past reviews, I think a new unit test is justified.
Objective
Recently the
u32
Entity::generation
was replaced with the newEntityGeneration
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 theu32
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.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 acmp_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.