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

Use Entity::PLACHOLDER for dangling entities #9199

Closed
wants to merge 1 commit into from

Conversation

Shatur
Copy link
Contributor

@Shatur Shatur commented Jul 18, 2023

Objective

Prior to this (#7335), when mapping the scene, we used an EntityMapper that used World to establish a new mapping to a dead entity when the entity is not in the EntityMap.

But this means that entity mapping require mutable World access. And it may be not desirable for networking if you map entities often.

Solution

So in this PR I tried a simpler approach: just mark all invalid entities with Entity::PLACHOLDER because this entity is always considered invalid.

The only downside of it is that all dangling entities will correspond to the same entity instead of different entities, but I doubt that anyone want to do anything with these entities. This approach makes the code much simpler, avoids extra Vec allocation and allows mapping entities without mutable World access.

I also considered returning error instead, but this may not be desirable for scene filtering (hierarchy could contain dangling entities).


Changelog

Changed

Invalid entities on mapping now always point to Entity::PLACEHOLDER.

Migration Guide

MapEntities implementations should change from a &mut EntityMapper parameter to a &EntityMap parameter and return Result. Finally, they should switch from calling EntityMapper::get_or_reserve to calling EntityMap::get_or_placeholder.

Prior to this, when mapping the scene, we used an `EntityMapper`
that used `World` to establish a new mapping to a dead entity
when the entity is not in the `EntityMap`.

But this means that entity mapping require mutable `World` access. And
it may be not desirable for networking if you map entities often.
So in this PR I tried a simpler approach: just mark all
invalid entities with `Entity::PLACHOLDER` because this entity is always
considered invalid.

The only downside of it is that all dangling entities will correspond to the
same entity instead of different entities, but I doubt that anyone want
to do anything with these entities. This approach makes the code much
simpler, avoids extra `Vec` allocation and allows mapping entities
without mutable `World` access.
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Scenes Serialized ECS data stored on the disk labels Jul 18, 2023
@Shatur
Copy link
Contributor Author

Shatur commented Jul 18, 2023

I had a discussion with @Illiux and I think we should try another approach to make EntityMapper usable outside of scenes.

@Shatur Shatur closed this Jul 18, 2023
@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Jul 18, 2023
@rlidwka
Copy link
Contributor

rlidwka commented Jul 18, 2023

Rust team did a lot of work eliminating null from the language, but hey lets bring it back! :)

get_or_placeholder is a horrible choice imho, if you want to get that route, it'd be better to implement Default for Entity (which would return placeholder), and let users do unwrap_or_default themselves

@Shatur
Copy link
Contributor Author

Shatur commented Jul 18, 2023

Rust team did a lot of work eliminating null from the language, but hey lets bring it back! :)

We already map missing entities to invalid entities (to avoid pointing them to random entities). This PR just changes this mapping to specific one to avoid mutable world access. But it's a bad idea too because @Illiux explained it to me, having the ability to distinguish between them is important. This is why I closed this PR.

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 A-Scenes Serialized ECS data stored on the disk C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants