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

Add object-oriented EntityId methods #725

Open
marceline-cramer opened this issue Aug 21, 2023 · 3 comments · May be fixed by #773
Open

Add object-oriented EntityId methods #725

marceline-cramer opened this issue Aug 21, 2023 · 3 comments · May be fixed by #773
Assignees
Labels
enhancement New feature or request topic:api API functionality, including both host and guest

Comments

@marceline-cramer
Copy link
Contributor

marceline-cramer commented Aug 21, 2023

As game codebases scale, I begin to find this extremely noisy and hard to read:

// e is an EntityId
entity::get_component(e, component());
entity::despawn_recursive(e);
entity::add_child(e, child);

I think that a much more readable (and idiomatic if you're looking at Ambient packages as objects) alternative is this:

// e is an EntityId
e.get(component());
e.despawn_recursive();
e.add_child(child);

This would apply for all other functions on EntityId in the entity module such as has_component().

@marceline-cramer marceline-cramer added enhancement New feature or request topic:api API functionality, including both host and guest labels Aug 21, 2023
@philpax
Copy link
Contributor

philpax commented Aug 21, 2023

This is something we've discussed a fair bit, but my opinion remains that we should not do this because it implies the object referred to the ID always exists, which isn't the case - that is, during that sequence of calls, e can be despawned at any point, and the successive calls will not really make any sense.

I think the form in which we're most likely to see this is #406, in which we can guarantee that an entity exists while we operate on it, which allows us to offer a richer API.

Another idea is reference-counted entities where having an id to them keeps them alive, but I'm not sure if that will happen any time soon, either.

@marceline-cramer
Copy link
Contributor Author

marceline-cramer commented Aug 21, 2023

Right now, guest-side code is single-threaded, so it's impossible for entities to be despawned while a guest is working with them unless the guest despawns them itself. We can help mitigate the ambiguity of whether entities are alive or not by making .despawn() and .despawn_recursive() take ownership of the entity IDs.

No, this will not solve issues of questionable ID validity, but as it stands, the entity module's functions already have that ambiguity AND the code clutter of calling those functions. I can think of a few times while making Flowerpot that entity ownership shenanigans caused an error that would have been less likely to happen if I had been paying attention to that instead of writing the same long function names repeatedly.

Adding a guard to make sure that entities are alive is as simple as this:

if !entity::exists(e) {
    return;
}

But I think that it's a lot more conducive to habitual error checking if it's as simple as this, instead:

if !e.exists() {
    return;
}

@philpax
Copy link
Contributor

philpax commented Aug 23, 2023

Right now, guest-side code is single-threaded, so it's impossible for entities to be despawned while a guest is working with them unless the guest despawns them itself. We can help mitigate the ambiguity of whether entities are alive or not by making .despawn() and .despawn_recursive() take ownership of the entity IDs.

You can have multiple EntityIds for the same entity, so consuming one of them on despawn won't be able to invalidate the other ones. This is also a problem with async:

let id = Entity::new().spawn();
run_async(async {
  sleep(5.0).await;
  id.set(blah(), 5); // `id` could have been despawned in the meantime
});

There's also the possibility that we expose multiple ECS worlds to the guest (#230 maybe?), so that you need to have a reference to the world in order to do anything. This would fairly easily generalise from our existing API - the functions would end up being methods on a world handle - but it ends up being more unclear if we move the methods to the ID.

For reference, here's the discussion we had about this on the Discord that led to #406: https://discord.com/channels/894505972289134632/1078283561540530216/1108329570916118609

I would like to offer a sleeker API - for the same reasons you mention - but I'm also afraid of implying that the IDs represent the entity, instead of merely pointing to one.

I've seen people struggle with the distinction between entity handles and entities before, as well as entity lifetimes, so I want to make it as obvious as possible, even if it comes at a bit of an ergonomics cost.

That being said, one change that I'm not opposed to and that we should probably consider is to rename the methods to make them less verbose (i.e. drop the _component suffixes where possible), as mentioned in that Discord conversation.

I'm also not that opposed to adding EntityId::exists, because that doesn't let you actually interact with the components of the entity in any meaningful way (which would perpetuate the handle=entity semantic misunderstanding).

@philpax philpax linked a pull request Aug 31, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request topic:api API functionality, including both host and guest
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants