Search Unity

How to safely destroy entities?

Discussion in 'Entity Component System' started by peaj_metric, May 19, 2022.

  1. peaj_metric

    peaj_metric

    Joined:
    Sep 15, 2014
    Posts:
    146
    This is really the biggest pain point I currently have with ECS.

    How can I safely destroy entities and assert they are not being referenced by any ECB or even System.

    ECBSystems fail if a command destroys an entity before another one tries to do anything with it.
    It does not matter if this happens in the same ECBSystem or via multiple Systems.

    My current solution is to use 2 ECBSystems.
    1) BeginFixedStepSimulationEntityCommandBufferSystem to destroy Entities
    2) BeginSimulationEntityCommand buffer for everything else

    I am using BeginFixedStepSimulationEntityCommandBufferSystem to destroy Entities to ensure that Entities returned from physics queries have not been destroyed in between physics steps.

    This mostly works to mitigate the problem.
    Mostly is not enough so. Since every fail will result in a game crash.

    It just happens that sometimes stuff is destroyed from the main thread.
    Or in this case the ParentSystem seems to catch an entity which has already been destroyed:

    Code (CSharp):
    1. ArgumentException: All entities passed to EntityManager must exist. One of the entities has already been destroyed or was never created. Entity (748:11) was previously destroyed by Unity.Entities.BeginFixedStepSimulationEntityCommandBufferSystem. This command was requested from EnemyEntityDeleteSystem.
    2. Unity.Entities.EntityComponentStore.AssertEntitiesExist (Unity.Entities.Entity* entities, System.Int32 count) (at Library/PackageCache/com.unity.entities@0.50.1-preview.2/Unity.Entities/EntityComponentStoreDebug.cs:273)
    3. Unity.Entities.EntityManager.AddComponent (Unity.Collections.NativeArray`1[T] entities, Unity.Entities.ComponentType componentType) (at Library/PackageCache/com.unity.entities@0.50.1-preview.2/Unity.Entities/EntityManager.cs:888)
    4. Unity.Transforms.ParentSystem.UpdateChangeParents (Unity.Entities.SystemState& state) (at Library/PackageCache/com.unity.entities@0.50.1-preview.2/Unity.Transforms/ParentSystem.cs:317)
    5. Unity.Transforms.ParentSystem.OnUpdate (Unity.Entities.SystemState& state) (at Library/PackageCache/com.unity.entities@0.50.1-preview.2/Unity.Transforms/ParentSystem.cs:418)
    6. Unity.Transforms.ParentSystem.__codegen__OnUpdate (System.IntPtr self, System.IntPtr state) (at <03123a18c5ac42fb99a9907c3248ab1d>:0)
    7. Unity.Entities.SystemBaseRegistry+<>c__DisplayClass10_0.<SelectBurstFn>b__0 (System.IntPtr system, System.IntPtr state) (at Library/PackageCache/com.unity.entities@0.50.1-preview.2/Unity.Entities/SystemBaseRegistry.cs:246)
    8. UnityEngine.Debug:LogException(Exception)
    9. Unity.Debug:LogException(Exception) (at Library/PackageCache/com.unity.entities@0.50.1-preview.2/Unity.Entities/Stubs/Unity/Debug.cs:19)
    10. Unity.Entities.<>c__DisplayClass10_0:<SelectBurstFn>b__0(IntPtr, IntPtr) (at Library/PackageCache/com.unity.entities@0.50.1-preview.2/Unity.Entities/SystemBaseRegistry.cs:250)
    11. System.Object:wrapper_native_0000021EB0511340(IntPtr, SafetyErrorDetails&)
    12. Unity.Entities.ComponentSystemGroup:UpdateSystem(WorldUnmanaged&, SystemHandleUntyped) (at Library/PackageCache/com.unity.entities@0.50.1-preview.2/Unity.Entities/ComponentSystemGroup.cs:541)
    13. Unity.Entities.ComponentSystemGroup:UpdateAllSystems() (at Library/PackageCache/com.unity.entities@0.50.1-preview.2/Unity.Entities/ComponentSystemGroup.cs:577)
    14. Unity.Entities.ComponentSystemGroup:OnUpdate() (at Library/PackageCache/com.unity.entities@0.50.1-preview.2/Unity.Entities/ComponentSystemGroup.cs:523)
    15. Unity.Entities.ComponentSystem:Update() (at Library/PackageCache/com.unity.entities@0.50.1-preview.2/Unity.Entities/ComponentSystem.cs:114)
    16. Unity.Entities.ComponentSystemGroup:UpdateAllSystems() (at Library/PackageCache/com.unity.entities@0.50.1-preview.2/Unity.Entities/ComponentSystemGroup.cs:583)
    17. Unity.Entities.ComponentSystemGroup:OnUpdate() (at Library/PackageCache/com.unity.entities@0.50.1-preview.2/Unity.Entities/ComponentSystemGroup.cs:523)
    18. Unity.Entities.ComponentSystem:Update() (at Library/PackageCache/com.unity.entities@0.50.1-preview.2/Unity.Entities/ComponentSystem.cs:114)
    19. Unity.Entities.DummyDelegateWrapper:TriggerUpdate() (at Library/PackageCache/com.unity.entities@0.50.1-preview.2/Unity.Entities/ScriptBehaviourUpdateOrder.cs:426)
    20.  
    So how do you destroy entities safely?
     
  2. brunocoimbra

    brunocoimbra

    Joined:
    Sep 2, 2015
    Posts:
    679
    I use a IsDestroyed component (which just contians a bool) and I have a job that Destroy everything that contains this component and its value is set to true at the end of the frame (which is basically how standard Unity works anyway).

    To avoid structural changes, I leave this component in all my prefabs that I expect to be destroyed at some point.

    With the enabled feature that is coming I will be able to turn it into a simple "DestroyTag" and enable it when I need to trigger it.
     
    Last edited: May 20, 2022
    Lukas_Kastern likes this.
  3. peaj_metric

    peaj_metric

    Joined:
    Sep 15, 2014
    Posts:
    146
    Thats an interesting approach.

    However wouldnt that force every System to query for the IsDestroyed component and read its state.
    Even if that would work for my own systems, I cannot change Unitys existing Systems to respect that component.
    It may be possible to "fix" this by also adding a disabled component to the entity as this is respected by the other systems.
    Also it could still happen that some System queues a scturctral change in an ECB and the IsDestroyed flag is only set later this frame.
    So the key here would be to wait several frames between setting the flag and destroying the entity.

    This still feels a lot like a workaround rather than a solution.
    Also you might want some entities to be destroyed immidiately.
    Otherwise you would need additional systems e.g. to hide meshes to make it seem instant.

    The following might work (besides the timing problem):

    1) Add IsDestroyed tag (or set flag)
    2) A System adds the Disabled tag to all entities with IsDestroyed tag via ECB
    3) Another System destroys all entities that have both IsDestroyed and Disabled tags via the same ECB

    This should ensure that at least one full frame happens between starting and completing the destruction and no system should queue any commands for this entitiy as it is invisible (disabled) through the whole frame.

    But this still feels really hacky. And to me it seems to be a fundamental problem with this ECS framework.
     
    koonm likes this.
  4. brunocoimbra

    brunocoimbra

    Joined:
    Sep 2, 2015
    Posts:
    679
    While this would solve some potential issues, I think this is too much trouble considering that component enabling/disabling is coming soon and will fix that for me. So far no Unity system gave me trouble due not using the Disabled tag on my about to be destroyed entities, but once we have that new feature I can also put the Disabled tag "disabled" alongside the IsDestroyed tag in the begging and just toggle them as needed.
     
  5. hippocoder

    hippocoder

    Digital Ape

    Joined:
    Apr 11, 2010
    Posts:
    29,723
    But why isn't the pattern just checking if the entity version has changed? This is a similar load to storing and checking an extra boolean. Then your Entity which accesses another later on will know it's not the same Entity. Then you can ECB destroy whatever, whenever, and not need to destroy things at specific times.
     
  6. Arnold_2013

    Arnold_2013

    Joined:
    Nov 24, 2013
    Posts:
    287
    TL;DR; without OrderLast = true, a scheduling after the EndFixedStepSimulationEntityCommandBufferSystem is just ignored. This gave me some issues with destroyed entities.


    Only using the main tread [.Run(), and .Complete() for physics] worked well for me with a DestroyTag component. But scheduling everything correctly with ecb.DestroyEntity() is hard for me. Might just be lack of knowledge on my part. I have the feeling that correct dependencies and scheduling should result in destroy commands running at the right time and this is something a developer should be able to do. (or else pay the performance cost for checking each entity before use as an alternative. Unity checking this for us would also cost performance (I guess).)

    One related thing I don't understand is how to visualize all the dependencies to check for errors. Currently I use the profiler timeline and just look for issues. Here I noticed the EndFixedStepSimulationEntityCommandBufferSystem scheduling not being respected.

    Code (CSharp):
    1. [UpdateInGroup(typeof(FixedStepSimulationSystemGroup))]
    2. [UpdateBefore(typeof(DestorySystem))]
    3. [UpdateAfter(typeof(EndFixedStepSimulationEntityCommandBufferSystem))]
    4. public partial class SyncEffectDataSystem : SystemBase
    When I use this I expect the SyncEffectDataSystem to show up between the EndFixedStepSimulationEntityCommandBufferSystem and the DestorySystem (within the same frame/tick of the profiler)

    But is shows up before the physics (and EndFixedStepSimulationEntityCommandBufferSystem ),
    upload_2022-5-31_11-12-41.png


    Now since the EndFixedStepSimulationEntityCommandBufferSystem is in "OrderLast = true", it only goes to the right spot on the profiler when this OrderLast = true is added. Once found I can live with this behavior, but an error message or something would have been nice. Or a tool to check what gameSystems/ecbSystems are really linked as dependencies.

    Edit: There was a warning I should have seen "Ignoring invalid [UpdateAfter(Unity.Entities.EndFixedStepSimulationEntityCommandBufferSystem)] attribute on SyncEffectDataSystem because OrderFirst/OrderLast has higher precedence."

    upload_2022-5-31_11-17-33.png


    With most of my systems it would not matter if its run and the 'end of frame N' or at the 'beginning of frame N+1', but with Destroying entities resulting in hard crashes it is very important to somehow know exactly when something is destroyed.
     
    Last edited: May 31, 2022
    apkdev likes this.
  7. peaj_metric

    peaj_metric

    Joined:
    Sep 15, 2014
    Posts:
    146
    Can you expand on that? Where and how should the entity version be checked?
     
  8. hippocoder

    hippocoder

    Digital Ape

    Joined:
    Apr 11, 2010
    Posts:
    29,723
    The Entity struct contains a version, which changes if the Entity is recycled for any reason and it exists to determine if the Entity id is really the same one or not. It may not be necessary in many cases. But if you are keeping refs around, then perhaps it is useful to determine if the Entity you want still exists. I don't know if Unity has a special check in place for this yet for comparisons.
     
  9. peaj_metric

    peaj_metric

    Joined:
    Sep 15, 2014
    Posts:
    146
    Yeah thats basically what EntityManager.Exists does:

    Code (CSharp):
    1.  
    2.         public bool Exists(Entity entity)
    3.         {
    4.             int index = entity.Index;
    5.  
    6.             ValidateEntity(entity);
    7.  
    8.             var versionMatches = m_VersionByEntity[index] == entity.Version;
    9.             var hasChunk = m_EntityInChunkByEntity[index].Chunk != null;
    10.  
    11.             return versionMatches && hasChunk;
    12.         }
    But that would mean you would need to add checks to each and every entity reference access in your code like @Arnold_2013 mentioned.

    It also has these problems:
    1) EntityManager is Main Thread only
    2) You cannot add these checks to Unitys Systems (e.g. Parenting System)
    3) You cannot add these checks to ECBs (which are the main cause of this problem)

    Technically it might be possible to write a custom ECBSystem that filters its ECBs for destroyed entities before executing the ecb via EntityManager.Exists as they run on main thread anyway.

    Not sure how big the overhead would be. But that might actually be a sane thing for unity to implement...
     
    hippocoder likes this.
  10. Arnold_2013

    Arnold_2013

    Joined:
    Nov 24, 2013
    Posts:
    287
    I think most of these issues can be avoided by using the HasComponent<T>(someEntity), this returns false on destroyed components, so I have not checked but I assume this means it will also check the version. Inside scheduled jobs this is replaced by ComponentDataFromEntity<T>.

    Instead of using the main thread .Exists(), you could do a HasCompontent<Translation>(someEntity) [or some other very common type, should you just want to know about existence]. Most of the time you will know a type that can be checked to determine existence of a specific entity.

    It does come at a performance cost, but better than a crash :)

    https://docs.unity3d.com/Packages/c...i/Unity.Entities.SystemBase.HasComponent.html
     
    hippocoder likes this.
  11. peaj_metric

    peaj_metric

    Joined:
    Sep 15, 2014
    Posts:
    146
    Yes you are right thats basically the equivalent to EntityManager.Exists to use in jobs.
    This eliminates the first problem.
    However the others still stand:

    1) EntityManager is Main Thread only
    2) You cannot add these checks to Unitys Systems (e.g. Parenting System)
    3) You cannot add these checks to ECBs (which are the main cause of this problem)
     
  12. WAYNGames

    WAYNGames

    Joined:
    Mar 16, 2019
    Posts:
    992
    Split your ecb use the first ecb of the frame to destroy entities and use others for structural change.
    That way all structural change happen before the end of the frame and the destruction happen before any job can register ecb commands.

    For event system if you are using something relying on native container like native stream or tertle's event system, you'll have to make sure events are consumed before the end of the frame or otherwise have an existence check in your consumer system.
    Same thing for entity that you would have a reference to in a component like a target for example.
     
    Last edited: Jun 1, 2022
  13. hippocoder

    hippocoder

    Digital Ape

    Joined:
    Apr 11, 2010
    Posts:
    29,723
    I found designing for Unity's default ECB meant I coded similar to a streaming / networked game, and that's actually a lot less headache in the long run. So I avoid using my own ECBs where possible.
     
  14. peaj_metric

    peaj_metric

    Joined:
    Sep 15, 2014
    Posts:
    146
    I think the key point here is to actually use an ECB that runs at the end of the current frame for structural changes.
    That might actually work to prevent this problem.

    I usally use BeginInitializationEntityCommandBufferSystem for sturctual changes and
    BeginFixedStepSimulationEntityCommandBufferSystem for destruction.

    You just have to make sure that no commands will be queued between the Structural change buffer and the destroctuin buffer.

    I figured its safer to destroy during FixedStep to ensure that entities returned from physics queries have not been destroyed between physics updates.

    So I guess I will have to find a replacement for BeginInitializationEntityCommandBufferSystem
     
  15. calabi

    calabi

    Joined:
    Oct 29, 2009
    Posts:
    232
  16. lclemens

    lclemens

    Joined:
    Feb 15, 2020
    Posts:
    761
    I used to have all kinds of problems with errors from accessing destroyed entities until I watched a Turbo Makes Games interview video where they discussed this rule :

    - Use ecb-begin to destroy entities
    - Use ecb-end for all other operations (add/remove/set/has/get/etc).

    That way any destroy operations will happen at the beginning of frame 2, allowing the systems in frame 1 to procede safely. Then in frame 2, all the entities marked for destruction will be destroyed before any of the systems run so their queries will never even know about the entities that were destroyed.

    This simple rule really made destruction much easier.
     
    chantey and Razmot like this.
  17. Arnold_2013

    Arnold_2013

    Joined:
    Nov 24, 2013
    Posts:
    287
    I can see this working. But in my case I use a DestroyTag to cleanup 'dependent entities'. When an Entity (A) has certain components with Entity fields and it has a DestroyTag then I also add a DestroyTag to these 'dependent Entities*' (B). (or some other cleanup logic).
    *I say 'dependent entities', but I don't mean the linked entity group. But in my case a 'Turret Entity' follows a 'Tower Entity', when the tower is destroyed the turret should also be destroyed, but they are different prefabs.

    So with this methode I would :
    - Set DestroyTag (A) on ecb-end
    - next frame
    - Run CleanupSystem before the DestroySystem -> This sets DestoryTag on ecb-end for the 'dependent Entity' (B)
    - Run DestorySystem -> Destorys on ecb-begin entity (A)

    so the dependent entity (B) would live and interact with 1 frame when the essential entity (A) is already destroyed, which could lead to errors.

    So should I design all the code to ignore entity (B) if entity (A) is destroyed?
    Or should I add another ecb-late sync point so the cleanupSystem can run between ecb-end and ecb-late?
    Or should I not set the DestroyTag on entity (B) in cleanupSystem but do a direct ecb.Destory() on ecb-begin?
    Or ???

    The first option is bad for performance, but works with many layers of 'dependent entities'. The other two options work as long as the 'dependent entities' don't go to deep.

    Currently my solution to this is CleanupSystem + first option. When entity (B) needs entity (A) it will check if (A) is still valid and if its not it will add the DestroyTag via ecb-end to (B) and further ignore it. If entity (B) did not need entity (A) this frame (or ever) the CleanupSystem will add a DestroyTag to (B) while (A) is alive with the DestroyTag.
     
  18. peaj_metric

    peaj_metric

    Joined:
    Sep 15, 2014
    Posts:
    146
    Why use the destroy tag at all?
    If you run your cleanup system before the destruction ecb you could just call ecb.Destroy on all of the entities that need to be cleaned up.
     
    lclemens likes this.
  19. Arnold_2013

    Arnold_2013

    Joined:
    Nov 24, 2013
    Posts:
    287
    This might be true. I am still very much in a learning phase and this is an interesting topic. But my thoughts are:

    There are multiple systems that can Add a DestroyTag (RemoveHitpointsSystem, LifeTimeSystem, CollisionSystem, ...).
    Followed by several CleanupSystems look for this DestroyTag + specific other components in order to correctly cleanup other entities (TowerDestroyCleanupSystem, VisibleEnemyCleanupSystem, ...).
    So the DestroyTag helps to decouple everything, so for example the LifetimeSystem does not need to determine what it is Destroying since it does not need to cleanup 'Dependent Entities'. The cleanup for a Projectile is nothing, for a Tower it is the removal of the turret entity and for a VisibleEnemy [data entity without visual representation] it is the destruction of the Dependant Entity with the RenderMesh (this is toggled on/off when enemy is (not) visible to the player). All 3 examples can be Destroyed by the LifeTimeSystem, when their lifetime runs out.

    It is possible the need for this decoupling comes from something that is wrong in my setup and the way I link entities together.
     
  20. lclemens

    lclemens

    Joined:
    Feb 15, 2020
    Posts:
    761
    I think system state components could be a good fit for what you're describing: https://docs.unity3d.com/Packages/com.unity.entities@0.51/manual/system_state_components.html
     
  21. mikaelK

    mikaelK

    Joined:
    Oct 2, 2013
    Posts:
    284
    hmm, What kind of issues where you having?
    I delay the destruction of entities at the end of the frame. So far I have had no problems.

    the EndSimulationEntityCommandBufferSystem is the last one to execute. Shouldn't it be 100% safe to destroy entities there?
     
  22. peaj_metric

    peaj_metric

    Joined:
    Sep 15, 2014
    Posts:
    146
    It is absolutely not safe if you:
    1) Queue component changes and entity destruction in the same buffer
    2) Use another buffer for component changes that might run after that (e.g. next frame)

    It seems for me most of the problems came from using ecbs at the beginning of the next frame instead of the end of the current frame.
    But the most important part is that the destruction ecb runs after the change ecb and no commands are queued in between.
    I still have to use a destruction buffer at the beginning of the frame because I destroy entities only in fixed update. Otherwise physics queries might return destroyed entities.

    I guess I will even create my own ECBSystems with specific naming (e.g. DestructionECBSystem and ChangeECBSystem) and cripple them so they cannot do anything else.

    I still think this should somehow be handled by Unity. It should not be that performance intensive to skip all ecb commands that effect destroyed entities.

    Performance by default is nice, but this will also be a pitfall by default.
     
    lclemens and mikaelK like this.
  23. mikaelK

    mikaelK

    Joined:
    Oct 2, 2013
    Posts:
    284
    I was going to argue against you, but after re reading docs I was about to say the same thing :)
    Its little cryptic
    upload_2022-6-21_12-47-17.png

    But for some reason I still find it odd to do the destroying at the beginning of the frame. Maybe making your own ecb system and reordering the systems to support your use case is the most cleanest solution as you said.

    At least it should be possible according to the docs.
    https://docs.unity3d.com/Packages/com.unity.entities@0.51/manual/entity_command_buffer.html
     
  24. peaj_metric

    peaj_metric

    Joined:
    Sep 15, 2014
    Posts:
    146