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

Togglable bit flag for Identifier #11475

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Bluefinger
Copy link
Contributor

@Bluefinger Bluefinger commented Jan 22, 2024

Objective

  • Provide the building block to enable togglable components using IDs.
  • Add another piece to the relations story.

Solution

This PR focuses on adding to the Identifier spec/type the ability to set and modify the IS_TOGGLABLE flag bit, for the primary purpose of building out the ability to "toggle" components assigned to a given entity (once they too are back by Identifiers/Entities). The PR does NOT include wiring this in as a full feature, but is instead focusing on just the Identifier bits.

The ability to toggle components on/off on an Entity allows the ability to have "temporary" removals instead of full removal, which would necessitate an archetype move. However, checking whether each component is toggled in a query would kill query perf, so this has to be a strictly opt-in feature. With this, a component that is set to be togglable would exist in an archetype for such, and would have a bitset to check against. And since the ID's are different, plain versions of that component would be distinguishable from togglable versions, so the checking of the bitset only applies to those that have been set to be such.

Note: Adding an additional flag to Identifier means that available generation space for Identifier/Entity is now 2^30 - 1, or 1,073,741,823 generations. The flags occupy the two most significant bits on the Identifier's high component. Due to the increasing complexity of managing the bit flags, I've included bitflags crate to wire some of the logic in and also to enable const-able operations.

The generation increment method has been modified to account for this, and also incorporates a micro-optimisation that seems only to be triggered for 30-bit values and less, as the same code for a 31-bit value resolves to the same ASM output. The optimisation now allows for one less instruction being emitted: https://rust.godbolt.org/z/vnnhr5cod

inc_entity_generation_by:
        and     edi, 1073741823
        and     esi, 1073741823
        lea     eax, [rsi + rdi]
        cmp     eax, 1073741824
        sbb     eax, -1
        and     eax, 1073741823
        ret

Some other optimisations include removing panic! paths from methods that will never panic, and preventing the inlining of panic pathways as a further optimisation for others (if something is panicking, it doesn't matter if that path is "slow").


Changelog

Added

  • Togglable bit flag for Identifier.

Changed

  • Micro-optimisations for Identifier/Entity methods

Migration Guide

Entity/Identifier layout has changed, so that the flags occupy 2 bits and the high value is now only 30 bits, so all code/assets relying on specific layout of Entity will need to be migrated/updated to the new layout.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Jan 22, 2024
@Bluefinger Bluefinger force-pushed the disable-bit-identifier branch 2 times, most recently from 243d2bb to 6bba679 Compare February 26, 2024 07:50
Copy link
Contributor

@NiseVoid NiseVoid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems mostly fine besides the review point. I'm not sure if the IS_HIDDEN name is the way to go here, discussion on discord suggests that the functionality enabled by the bit could be different than the name suggests. Or maybe you had some other idea for how it could be implemented?

crates/bevy_ecs/src/entity/mod.rs Show resolved Hide resolved
@Bluefinger Bluefinger changed the title Hidden bit flag for Identifier Togglable bit flag for Identifier Mar 17, 2024
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Mar 17, 2024

This is a building block for #11090, correct? Can you say more about how this builds towards relations (#3742)?

@alice-i-cecile
Copy link
Member

I'm having some difficulties understanding the semantics here. If an entity is togglable (once we've wired this up), what does that mean? Should this be on/off or enabled/disabled instead?

The "hidden" naming was much clearer to me, but I agree that we shouldn't use that naming: it's very confusing when rendering visibility enters play.

@Bluefinger
Copy link
Contributor Author

This is a building block for #11090, correct? Can you say more about how this builds towards relations (#3742)?

After some discussions on Discord, I think it became clearer that for entity disabling, that requires a different mechanism using a marker component. This PR would be more useful for component disabling, but being able to apply it conditionally so that only components that have their ID with the bit set would have their bitset checked for being disabled or not. A similar mechanism could be used to do opt-in change detection, though that probably needs another bit reserved (or maybe reusing this bit, but that I think @maniwani has a better idea on this). Overall, this would allow some features to enable/disable components on entities (useful for syncing/rollback/etc for networking and other uses).

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-Feature A new feature, making something new possible M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants