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

CommandBuffer Pending Entity #163

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

martindevans
Copy link
Contributor

As briefly discussed in Discord, this modifies the CommandBuffer to not return fake Entity objects. Instead it returns a PendingEntity. This makes the API less confusing and prevents all the mistakes you could make.

Added a new test ("CommandBufferEntityErrors") which demonstrates all the possible errors this fixes:

  • Using the returned Entity with the world (fails type checking)
  • Creating an entity with one buffer, using it with another buffer (throws)
  • Creating an entity, calling playback, then trying to use the entity with this buffer (throws).

Breaking Change

Note that this is a breaking change: The Create() -> Entity method has become Create() -> PendingEntity.

Most code that is broken by this should either be extremely easy to fix (just change the type) or was already subtly broken and this more explicit break is a good thing.

…his prevents using the returned `Entity` in incorrect parts of the API.
@martindevans
Copy link
Contributor Author

martindevans commented Oct 27, 2023

To be honest I don't really like the name PendingEntity, but couldn't think of anything better.

@genaray
Copy link
Owner

genaray commented Oct 30, 2023

Thanks! Im gonna look at that one later ^^
Just wonder if there any drawbacks to this. E.g. having an PendingEntity might be a bit restrictive in certain scenarios. Im gonna check that

@martindevans
Copy link
Contributor Author

I don't think there's anything that is valid to do with the "Entity" returned by the CommandBuffer that the PendingEntity doesn't allow. If you do identify something a use-case I've missed I'll add something to support it.

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

Successfully merging this pull request may close these issues.

2 participants