DefaultEcs icon indicating copy to clipboard operation
DefaultEcs copied to clipboard

Calling Has<T> after Dispose

Open Helco opened this issue 2 years ago • 3 comments

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.

Helco avatar Sep 24 '22 16:09 Helco

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.

Doraku avatar Sep 24 '22 19:09 Doraku

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

Helco avatar Sep 25 '22 09:09 Helco

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 :/

Doraku avatar Sep 25 '22 14:09 Doraku