Doraku / DefaultEcs

Entity Component System framework aiming for syntax and usage simplicity with maximum performance for game development.
MIT No Attribution
658 stars 62 forks source link

Calling Has<T> after Dispose #169

Closed Helco closed 1 year ago

Helco commented 2 years ago

During work on #168 I found what seems to be a contradiction in one of the entity tests (Dispose_Should_remove_all_component). If one adds a line between the calls to Dispose and Has<bool> like world.CreateEntity().Set(true); the following assertion fails.

https://github.com/Doraku/DefaultEcs/blob/e4bf94d7c60e54209b6b89e86f861f7d22f35d22/source/DefaultEcs.Test/EntityTest.cs#L683-L684

The Has<T> method is not specially marked so one would assume it is not allowed to call after Dispose, as it is evidently not safe. The entity version is not checked thus the check is silently applied to another entity.

Doraku commented 2 years ago

yep, that's by design since internal entity ids are reused. You would be expected to check the IsAlive property before every call on an entity to be sure it has not been disposed, obviously not something you would do in release for performance.

Helco commented 2 years ago

I guessed that this was the intention, the test however is then relying on circumstantial behavior.

Doraku commented 2 years ago

Aren't all tests circumstantial by nature? You set a specific state and expect a determinist result. But this one is maybe a little too much "in the know" of the implementation details, as it then tries to create a new entity (which it expects to have the same internal id as the disposed entity) and checks that the component is really not there anymore. It's hard to test everything while still keeping most of the implementation out of the public api, especially when you aim for performance when it sometimes pulls the cursor far from the ease of use :/