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

Make the NULL semantics of entity clear #16204

Open
iiYese opened this issue Nov 1, 2024 · 5 comments
Open

Make the NULL semantics of entity clear #16204

iiYese opened this issue Nov 1, 2024 · 5 comments
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible D-Straightforward Simple bug fixes and API improvements, docs, test and examples X-Controversial There is active debate or serious implications around merging this PR

Comments

@iiYese
Copy link
Contributor

iiYese commented Nov 1, 2024

What problem does this solve or what need does it fill?

Previously I mentioned this in discussions as "introducing" null semantics to Entity to not have to work with Option<Entity>. This was incorrect. Entity has always had null semantics but they are just hidden by the use of Option & misguides users.

People have encouraged the use of Option<Entity> to store entity IDs & have the notion that the Option being Some means you have a valid Entity ID. This is not correct. Even if you have an Entity id it is not guaranteed to be valid. It mightn't be in your query, may have been despawned or with future features might be disabled. Option isn't guaranteeing the entity exists it's only guaranteeing the ID existed on the other side of some API contract when it was produced. "Existed" is past tense meaning even if it is Some correctness concerned users should still be using fallible APIs to check their entities because if you've only checked the Option you've checked nothing. The use of Option isn't just redundant it can also misguide users who are new to bevy to think the following code should not panic:

if let Some(id) = id {
    world.entity(id)
}

What solution would you like?

Replace Entity::PLACEHOLDER with Entity::NULL & make it so no entity can have this id.

Additional context

This change would not mean that people have to check e != Entity::NULL because fallible APIs exist on Query, World etc. In practice having an Entity::NULL is the same as having an ID of an entity that exists that you can't do anything with. It doesn't stop people from using Option either.

@iiYese iiYese added A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible D-Straightforward Simple bug fixes and API improvements, docs, test and examples X-Controversial There is active debate or serious implications around merging this PR labels Nov 1, 2024
@alice-i-cecile
Copy link
Member

I prefer Entity::NULL over Entity::PLACEHOLDER, but would prefer to remove it entirely, or at least from the public API. Null-checking for data types is unidiomatic and frustrating.

@iiYese
Copy link
Contributor Author

iiYese commented Nov 1, 2024

but would prefer to remove it entirely, or at least from the public API

I'd be strongly against this & prefer the exact opposite.

Null-checking for data types is unidiomatic and frustrating.

The existence of Entity::NULL/Entity::PLACEHOLDER is not the thing that is responsible for null semantics. Those semantics are inherent to Entity, have always been there & will continue to be there even if this is removed. You cannot remove this fact. It's just the patterns perpetuated currently are lying about it. Entity::NULL is not the footgun Option<Entity> is.

There's nothing unidiomatic about this. It's just the meaning of Option<Entity> has been chronically misunderstood. Option<Entity> is more akin to Option<*const T> than it is to Option<T>. Pointers still have to be checked & so do Entitys.

@Arrekin
Copy link

Arrekin commented Nov 1, 2024

I totally agree with that. Consider that with Option<Entity> you always have to make 2 checks. First, whether it is None or not, and then second whether that Entity still exists in the world. Hence the Option gives us nothing besides unnecessary code checks because you will have to make that second check no matter what.

If the Entity were some form of smart pointer that conveys actual information about the world, I agree that having a NULL value would be incorrect. But since the Entity itself does not guarantee anything, that Option wrapper gives us nothing besides more useless code to write.

Option<Entity> means you have some Entity ID and then that ID can be valid or not. (ie 3 states you have to take care of)
Entity means you have ID that can be valid or not (ie 2 states you have to take care of).
The additional state in Option<Entity> is not useful. None is meaningfully the same as having an Entity that does not exist in the world. There may be cases where your API meaningfully says it accepts the Option version but no one forbids it. Let's just not force it on everyone when that one state will be useless to 99% of the developers.

As a developer, I really do want to have some form of a default Entity that I can use. If you would want really hard to have Option anyway, then make it transparent to the users, like Entity itself being the wrapper: Entity(Option<EntityRaw>). If this would sound like a bad idea to the Engine, then perhaps the same argument can be made as to why not force this solution(using Option) on the Engine users as well.

As that, I would even keep it as Entity::PLACEHOLDER since I like that name better than NULL. NULL suggests something missing/invalid, while PLACEHOLDER is a valid Entity, just one that is guaranteed to never be spawned in the world, so you can safely use it as an temporary intermediary value and it will error if you ever query for it(as would any non-existing Entity). We already have all we need, just this 2 things would make it perfect:

  • Derive Default with PLACEHOLDER as the value
  • Add a check in the spawn process to ensure this entity is never created in the world.

Perfection achieved : d

@BenjaminBrienen
Copy link
Contributor

I thought that cart said something about how he would prefer making it unnecessary to have PLACEHOLDER or None by improving the semantics of the ECS. Sorry if I'm misremembering.

@iiYese
Copy link
Contributor Author

iiYese commented Nov 1, 2024

If he did I'm pretty sure it's with the same notions I've highlighted that I take issue with.

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 D-Straightforward Simple bug fixes and API improvements, docs, test and examples X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

No branches or pull requests

4 participants