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

RFC: Entity ID, a component like you and me #234

Open
adamreichold opened this issue Jan 2, 2022 · 9 comments
Open

RFC: Entity ID, a component like you and me #234

adamreichold opened this issue Jan 2, 2022 · 9 comments
Labels
enhancement New feature or request

Comments

@adamreichold
Copy link
Collaborator

adamreichold commented Jan 2, 2022

Currently, entity ID are stored separately from other components and are yielded by queries unconditionally. An alternative design point would be to store entity ID like other components, i.e. add Entity as the only component of the initial archetype created at

hecs/src/world.rs

Line 1205 in 4840f93

archetypes: vec![Archetype::new(Vec::new())],

This would enable user code to decide whether queries return the entity ID or not by including them in or omitting them from their query specifications like

let find_nearest = world.query::<(&Entity, &Position)>();
let update_position = world.query::<(&mut Position, &Velocity)>();

and reliably remove the look-up of the generation in the query iterator at

generation: unsafe { self.meta.get_unchecked(id as usize).generation },

I think the main trade-offs here are the implicitness of making sure entity ID are consistent and safe from manipulation via user code and the increased storage requirements as storing Entity as a component requires 8 bytes versus 4 bytes for storing the ID without the generation out of band at

entities: Box<[u32]>,

@Ralith
Copy link
Owner

Ralith commented Jan 2, 2022

I'd be happy to have this; I agree it would make the API more pleasantly uniform.

making sure entity ID are consistent and safe from manipulation via user code

I think this could be addressed by making the actual component type private, and exposing it only via a special impl Query for Entity implementation whose associated Fetch yields Entity by value, similar to how Satisfies yields a bool by value. Queries would therefore look like (Entity, &Position) rather than (&Entity, &Position), preserving consistency with the result type.

the increased storage requirements as storing Entity as a component requires 8 bytes versus 4 bytes for storing the ID without the generation out of band

This could be mitigated by passing the &[EntityMeta] through Fetch::execute for the benefit of <Entity as Query>::Fetch, and ignoring it in all other cases. Feels like a hack, but it should optimize out gracefully at least, and it's not like that trait's particularly tidy to begin with...

@Ralith Ralith added the enhancement New feature or request label Jan 2, 2022
@adamreichold
Copy link
Collaborator Author

This could be mitigated by passing the &[EntityMeta] through Fetch::execute for the benefit of ::Fetch, and ignoring it in all other cases. Feels like a hack, but it should optimize out gracefully at least, and it's not like that trait's particularly tidy to begin with...

I do wonder whether those additional bytes are worth it though to avoid the indirection via &[EntityMeta]? I guess it depends on how much the extra space and copying costs compared to the (possibly fragmented due iterating in archetype order) access to the meta-data?

@adamreichold
Copy link
Collaborator Author

I think this could be addressed by making the actual component type private, and exposing it only via a special impl Query for Entity implementation whose associated Fetch yields Entity by value, similar to how Satisfies yields a bool by value. Queries would therefore look like (Entity, &Position) rather than (&Entity, &Position), preserving consistency with the result type.

This does sound rather elegant! In rs-ecs I just ended up asserting the type ID against Entity whenever I would hand out a mutable reference to any component (depending on this being optimised out without dynamically typed access to components).

One thing I am still mulling about though is whether this could be circumvented via Archetype::component_types which could be used to infer the type ID of the private entity ID type? But then again, messing up the entity ID yielded by queries should not break memory safety AFAIU, only make accessing components using these ID fail. So this seems more like a usability feature and if user code goes out of its way to break it, so be it?

@Ralith
Copy link
Owner

Ralith commented Jan 2, 2022

I guess it depends on how much the extra space and copying costs compared to the (possibly fragmented due iterating in archetype order) access to the meta-data?

Good point! I have to imagine that maintaining a non-fragmented copy would be better as far as iteration is concerned, and I don't imagine it would significantly increase costs elsewhere. The extra 4 bytes per entity strikes me as a very marginal cost, since entities will typically have considerably more data than that anyway, so I suppose I favor your original concept there, especially given the greater simplicity.

Archetype::component_types

I don't think the current API allows users to do much with a TypeId but no knowledge of the actual type, though that's subject to change under #69. We could also carefully hide its existence with special cases, but I think I prefer the simplicity of documenting the issue and letting it be the user's responsibility not to go out of their way to make a mess.

Another approach would be to use the public Entity type, but assert that they're never queried by (mutable?) reference or otherwise modified, which should optimize out reliably at least for static types. This might actually be preferable regardless, as otherwise a user might accidentally query &Entity and be surprised by silently getting zero results.

@adamreichold
Copy link
Collaborator Author

This might actually be preferable regardless, as otherwise a user might accidentally query &Entity and be surprised by silently getting zero results.

Indeed! I think this could be quite the paper cut and I also like the increased uniformity of accessing Entity compared to other components (even though I always have to do at least one round trip through compiler adding missing & or * when using this. 😃).

I will try to prepare a PR implementing this: storing the complete Entity type as a component which can be queries as any other and using asserts to avoid handing out mutable references to it. Discussing concrete code is probably nicer in any case.

@Ralith
Copy link
Owner

Ralith commented Jan 2, 2022

Sounds good; thanks!

@adamreichold
Copy link
Collaborator Author

I have been trying this on and off for some time now, but I have to admit that I am struggling to make it work. The problem is keeping the various sets of TypeId/TypeInfo synchronized behind the scenes so that Entity gets added to each archetype but component bundles never contain that type.

I think all acceleration structure should use the unadorned bundles so that I would expect to only fix up the sets of TypeInfo whenever a new archetype or insert target is created, but this does not seem to work out ATM.

Due to the complications of figuring this out, I fear that maintainability would suffer under the proposal and I am not sure that this would be worth it.

Will keep trying...

@Ralith
Copy link
Owner

Ralith commented Jan 7, 2022

Can you elaborate on the difficulty there? Off the cuff I'd think you could get away with adding Entity at the proper position in Archetype::new, and asserting using with_ids in ArchetypeSet::get_insert_target and World::remove_target, but I'm guessing you've tried that.

@Ralith
Copy link
Owner

Ralith commented Oct 10, 2023

One complication is that adding Entity in Archetype::new causes it to be duplicated when an archetype is constructed e.g. to handle insertion of a new component on an existing entity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants