set spawn_despawn on the correct entity when despawning #21364
Merged
Conversation
This file contains hidden or 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
janis-bhm
added
C-Bug
|
The test only does anything when track_location is on, which doesn't appear to be the case for the tests in the CI? a bit unfortunate. |
alice-i-cecile
approved these changes
Oct 3, 2025
|
@janis-bhm can you incorporate the changes from #21359 into this PR please so we can merge it with passing tests? |
…nto ecs/fix-entity-spawn-despawn-on-despawn
alice-i-cecile
approved these changes
Oct 3, 2025
james7132
approved these changes
Oct 4, 2025
james7132
removed
the
S-Needs-Review
alice-i-cecile
added
the
S-Ready-For-Final-Review
mockersf
pushed a commit
that referenced
this pull request
Oct 4, 2025
# Objective Fixes #21293 Fixes #17314 to ensure that this is tested correctly. ## Solution when despawning an entity, previously the swapped in (archetype) or moved in entity (table) (which both require extra bookkeeping to update archetype or table rows) were marked as `spawned_or_despawned` by the location and tick that the to-be-removed entity was meant to be marked, while the to-be-removed entity wasn't marked. As pointed out by @akimakinai in [#19047](#19047), I've re-added the correct `mark_spawn_despawn` call to `despawn_with_caller`. ## Testing I've added a test `spawned_after_swap_remove` that ensures that despawning an entity by swapping doesn't effect other entities `spawned_or_despawned` location, and that it does effect the despawned entity's index's `spawned_or_despawned` location. Co-Authored By: waterwhisperer24@qq.com --------- Co-authored-by: WaterWhisperer <waterwhisperer24@qq.com>
chescock
reviewed
Oct 5, 2025
| world.flush(); | ||
|
|
||
| // SAFETY: `self.entity` is a valid entity index | ||
| unsafe { |
There was a problem hiding this comment.
I'm a little late here, but don't we still need to call mark_spawn_despawn() before flush() so that the value is updated before we run commands queued by any hooks?
...
Yeah, I just tested it, and it's possible to observe a stale value in a command, or to spawn a new entity that re-uses the index and have its spawed_by overwritten. It's very much an edge case, but let me go make a quick PR to change that ... #21397.
beicause
pushed a commit
to beicause/bevy
that referenced
this pull request
Oct 6, 2025
…1364) # Objective Fixes bevyengine#21293 Fixes bevyengine#17314 to ensure that this is tested correctly. ## Solution when despawning an entity, previously the swapped in (archetype) or moved in entity (table) (which both require extra bookkeeping to update archetype or table rows) were marked as `spawned_or_despawned` by the location and tick that the to-be-removed entity was meant to be marked, while the to-be-removed entity wasn't marked. As pointed out by @akimakinai in [bevyengine#19047](bevyengine#19047), I've re-added the correct `mark_spawn_despawn` call to `despawn_with_caller`. ## Testing I've added a test `spawned_after_swap_remove` that ensures that despawning an entity by swapping doesn't effect other entities `spawned_or_despawned` location, and that it does effect the despawned entity's index's `spawned_or_despawned` location. Co-Authored By: waterwhisperer24@qq.com --------- Co-authored-by: WaterWhisperer <waterwhisperer24@qq.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Objective
Fixes #21293
Fixes #17314 to ensure that this is tested correctly.
Solution
when despawning an entity, previously the swapped in (archetype) or moved in entity (table) (which both require extra bookkeeping to update archetype or table rows) were marked as
spawned_or_despawnedby the location and tick that the to-be-removed entity was meant to be marked, while the to-be-removed entity wasn't marked.As pointed out by @akimakinai in #19047, I've re-added the correct
mark_spawn_despawncall todespawn_with_caller.Testing
I've added a test
spawned_after_swap_removethat ensures that despawning an entity by swapping doesn't effect other entitiesspawned_or_despawnedlocation, and that it does effect the despawned entity's index'sspawned_or_despawnedlocation.Co-Authored By: waterwhisperer24@qq.com