Fix for Issue #4 on re-used Entity IDs #5
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
thygrrr
reviewed
Jan 26, 2024
| @@ -96,6 +96,10 @@ public void Despawn(Identity identity) | |||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | |||
| public void AddComponent(StorageType type, Identity identity, object data) | |||
| { | |||
| if (!IsAlive(identity)) | |||
There was a problem hiding this comment.
Suggest using an assertion instead.
Nevermind, the Library throws runtime exceptions as well.
thygrrr
reviewed
Jan 26, 2024
| @@ -147,13 +155,21 @@ public void AddComponent(StorageType type, Identity identity, object data) | |||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | |||
| public bool HasComponent(StorageType type, Identity identity) | |||
| { | |||
| if (identity == Identity.None) | |||
There was a problem hiding this comment.
Early out feels wierd here, current HEAD already checks for Identity.none - so this can be...
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool HasComponent(StorageType type, Identity identity)
{
var meta = Meta[identity.Id];
return meta.Identity != Identity.None
&& meta.Identity == identity
&& Tables[meta.TableId].Types.Contains(type);
}
|
This PR should not be merged because it breaks functionality for |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix for Issue #4
The first commit fixes the bug that
IsAlive(deadEntity)returned true when there was a new entity re-using its ID.The second commit goes through all (?) other places that might operate on a dead entity, and either throws, returns false, or returns an empty list; instead of just operating on whatever new entity is re-using its ID. In potentially performance-sensitive functions, I avoided calling IsAlive directly so as not to do an unnecessary lookup. It's probably unnecessary to check for equality on the Identity as opposed to just the Generation, but I wanted to be safe.