-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Make Entity construction more ergonomic #21246
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
Conversation
Otherwise creation with index = 1 and generation = 1 looks like this: ```rust let expected_entity = Entity::from_bits((1 ^ u32::MAX) as u64 | (1 << 32)); ``` I also removed outdated comment which left untouched after the generation rework (bevyengine#19121)
Make construction more convenient and doesn't require to pull `nonmax`.
For details see bevyengine/bevy#19121 bevyengine/bevy#18704 The niche is now in the index, which makes the compression logic even simpler. The index now represented by NonMaxU32, which internally represented as NonZeroU32 with all bits reversed, so we have to xor the bits. I opened a PR to make it more ergonomic and avoid us relying on the internal layout: bevyengine/bevy#21246
I don't see how |
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.
I'm comfortable making this pub
, and I really appreciate how much the new helper function improves things. The value for tests is evident!
Take a look at the diff: it makes writing tests a bit more ergonomic, since you have to unwrap the |
Yeah that's what I mean. You unwrap either way and type nearly the same amount of characters. You just shift the error from the type system guarantees to the function. Anyhoot, not gonna block on that if others like it :) |
I really like having one less set of parantheses :) Long method names are much easier to autocomplete! |
Fair point! |
Also |
okay that convinced me instantly that this is the right way to go haha |
# Objective Entity serialization is necessary for networking. Entities can exist inside components and events. After deserialization, we simply map remote entities to local entities. To serialize entities efficiently, we split them into index and generation, which benefits from varint serialization. #19121 and #18704 changed the entity layout, and I like the new layout a lot. We can now use the extra bit from the index to store whether the generation is zero or not, avoiding the need to serialize the generation entirely. However, constructing new entities requires relying on the internal layout, which is not very ergonomic. For example, here is how an entity with index = 1 and generation = 1 can be created: ```rust let expected_entity = Entity::from_bits((1 ^ u32::MAX) as u64 | (1 << 32)); ``` ## Solution - Make `Entity::from_raw_and_generation` public. While at it, I also removed outdated comment. - Add `EntityRow::from_raw_u32` to make the initialization nicer. ## Testing - It's a trivial change, but I re-used `EntityRow::from_raw_u32` in unit tests to simplify them. ## Notes I'd probably rename `Entity::from_raw_and_generation` into `Entity::from_row_and_generation` or `Entity::from_index_and_generation`.
# Objective Entity serialization is necessary for networking. Entities can exist inside components and events. After deserialization, we simply map remote entities to local entities. To serialize entities efficiently, we split them into index and generation, which benefits from varint serialization. #19121 and #18704 changed the entity layout, and I like the new layout a lot. We can now use the extra bit from the index to store whether the generation is zero or not, avoiding the need to serialize the generation entirely. However, constructing new entities requires relying on the internal layout, which is not very ergonomic. For example, here is how an entity with index = 1 and generation = 1 can be created: ```rust let expected_entity = Entity::from_bits((1 ^ u32::MAX) as u64 | (1 << 32)); ``` ## Solution - Make `Entity::from_raw_and_generation` public. While at it, I also removed outdated comment. - Add `EntityRow::from_raw_u32` to make the initialization nicer. ## Testing - It's a trivial change, but I re-used `EntityRow::from_raw_u32` in unit tests to simplify them. ## Notes I'd probably rename `Entity::from_raw_and_generation` into `Entity::from_row_and_generation` or `Entity::from_index_and_generation`.
For details see bevyengine/bevy#19121 bevyengine/bevy#18704 The niche is now in the index, which makes the compression logic even simpler. The index now represented by NonMaxU32, which internally represented as NonZeroU32 with all bits reversed, so we have to xor the bits. I opened a PR to make it more ergonomic and avoid us relying on the internal layout: bevyengine/bevy#21246
Objective
Entity serialization is necessary for networking. Entities can exist inside components and events. After deserialization, we simply map remote entities to local entities.
To serialize entities efficiently, we split them into index and generation, which benefits from varint serialization. #19121 and #18704 changed the entity layout, and I like the new layout a lot. We can now use the extra bit from the index to store whether the generation is zero or not, avoiding the need to serialize the generation entirely.
However, constructing new entities requires relying on the internal layout, which is not very ergonomic. For example, here is how an entity with index = 1 and generation = 1 can be created:
Solution
Entity::from_raw_and_generation
public. While at it, I also removed outdated comment.EntityRow::from_raw_u32
to make the initialization nicer.Testing
EntityRow::from_raw_u32
in unit tests to simplify them.Notes
I'd probably rename
Entity::from_raw_and_generation
intoEntity::from_row_and_generation
orEntity::from_index_and_generation
.