Retained Entity if reactive system is before execute system

[akoolenbourke]

Hi all.

Was fiddling with the hello world tutorial and added an execute system to create a DebugMessage entity in its Execute() call.

The set of systems also has a CleanupSystem at the end that destroyed any entities in the DebugMessage group.

What happened was two fold

1. The message wasn't printed at all
2. The hierarchy showed an error icon (red C) and said the entity was retained by the collector in the reactive system.

(Note switching the order of the execute and reactive systems makes it work)

My thinking is that what happened was that when the execute system runs it ends up adding the entity to the collector of the reactive system that runs before it. The cleanup system then runs and called e.Destroy(). When the reactive system next runs it realises something is wrong.

My questions are, is this expected, and why is this the case? Shouldn't the fact the entity was in the collector/group be enough for the entity to not really be destroyed because a reference is still held in the collector.

Does this mean that we always should have execute systems before reactive systems if both reference the same entities?

Just after some clarification because I couldn't find anything in the docs anywhere on this phenomenon

[akoolenbourke]

Just adding that this issues makes using Entitas extremely difficult unless I'm missing something here.

Unless you can be 100% sure of what each System is doing at any point in the execution order you really have to make sure that any entity Destroy'ing happens after all Reactive systems and before any other systems that might signal destruction, even other Reactive systems.

Some people have deferred Destroy until the end of the next frame but that runs the risk of an Execute system running with an entity that is meant to be Destroyed. This seems like a bad idea.

Any thoughts on this? Is this as big of an issue as I am thinking? IS the solution something crazy like in a DestroySystem, if there are entities to destroy, then call systems.ClearReactiveSystems() before proceeding?

Trying to shutdown a level cleanly is proving to be a nightmare.

[ghost]

Hello and welcome in the community, can you please provide some code? I'm curious, because this shouldn't be the case if you are not doing it wrong. This behaviour is not expected. All kind of execute systems are processed in the execution order you provide in your Feature. Shutting down a level ist usually very easy. Of course depends on what to do. But destroying entities isn't a big deal.

[akoolenbourke]

Hi there. I can share a whole Unity project but this code should hopefully be enough to reproduce/identify the issue.

I started with the HelloWorld tutorial. I then added another ExecuteSystem+CleanupSystem that just created a new entity with a DebugMessageComponent on it every Execute() and deletes all entities in the cleanup (Note, there are no other entities other than ones with DebugMessageComponent on them.

using Entitas;

public class GenerateDebugMessageSystem : IExecuteSystem, ICleanupSystem
{
    GameContext game;

    public GenerateDebugMessageSystem(Contexts contexts)
    {
        game = contexts.game;
    }

    public void Cleanup()
    {
        foreach(var e in game.GetEntities())
        {
            e.Destroy();
        }
    }

    public void Execute()
    {
        game.CreateEntity().AddDebugMessage("Hello!");
    }
}

I added the system to TutorialSystem.cs as the last system

public class TutorialSystems : Feature
{
    public TutorialSystems(Contexts contexts) : base("Tutorial Systems")
    {
        Add(new HelloWorldSystem(contexts));
        Add(new DebugMessageSystem(contexts));
        Add(new GenerateDebugMessageSystem(contexts));
    }
}

If you check in the entity debugger you will see there is always a retained entity. If you swap the order of DebugMessageSystem and GenerateDebugMessageSystem in TutorialSystem.cs the error does not happen. This is why I was wondering if it was a collection issue. The error-generating way, would see a DebugMessage collected in GenerateDebugMessage, but then the cleanup tries to destroy it whilst it's retained by the collector. The working-way the item is released by by the DebugMessageSystem so all's good.

[akoolenbourke]

Hi there. Any thoughts on this one?

[gregharding]

@StormRene I've been chatting to @akoolenbourke and working through the tutorials (eg. Hello World) using v1.13.0 from the Asset Store and have hit similar problems with the inspector telling me that some entities have been retained and did I forget to call "entity.Release(owner)?", even though I'm not explicitly using retain/release in my code.

I just made a test project with a TestSystems feature, a TestMessageSystem that's spawning entities with TestMessageComponents, a reactive system that listens for TestMessages, and a cleanup system that destroys TestMessages.

Firstly, if the reactive system is added to the TestSystems feature before the execute system that generates them then the reactive system doesn't process them and retain errors show up in the inspector (but interestingly not every time the test messages are generated).

If the reactive system is added after the execute system in the feature then things are fine, but if I then create a new entity with another TestMessage while executing in that reactive system I see the retain errors (and obviously, due to the cleanup, the entity is destroyed before it can be collected and processed). If I remove the cleanup destroying the entities, everything works fine.

I first hit this problem when I was testing the debug message system in the hello world. When I was adding other reactive systems that created entities with debug messages things started misbehaving and the retain warnings appeared. It might very well be something that I've missed in learning the ways of ecs, but it doesn't quite make sense.

I haven't fully worked through it all but it feels like the collectors or groups in the reactive system might be out of sync somehow with the reusable entity pool. I'm happy to zip up the few classes I am using as a test if you want to take a look.

Update: @c0ffeeartc is suggesting that this is UI problem, not a real error.

[c0ffeeartc]

@akoolenbourke, I ran hello world and your systems example

If you check in the entity debugger you will see there is always a retained entity. If you swap the order of DebugMessageSystem and GenerateDebugMessageSystem in TutorialSystem.cs the error does not happen.

Visual debugging shows that entity is retained and has red icon with e. Reactive system collector then releases entity, entity gets reused and it's expected behaviour. While it may look misleading there is no error in this. The problem would be if entity wouldn't get reused. Maybe visual debugging could be improved somehow to avoid this misleading appearance

When the reactive system next runs it realises something is wrong.

How does it show that something wrong? Visual debugging appearance or debug log exception/error/warning. If it's visual debugging than it's not always an error

My questions are, is this expected, and why is this the case? Shouldn't the fact the entity was in the collector/group be enough for the entity to not really be destroyed because a reference is still held in the collector.

Entities are never destroyed but get disabled, components removed and put into pool by AERC(Automatic Entity Reference Counting). Reactive system collector reduces reference count and moves disabled entity from retained to reuseable entities.

Does this mean that we always should have execute systems before reactive systems if both reference the same entities?

No, entity will get released by AERC.

[akoolenbourke]

If anyone wants, here is a link to a Unity project that shows this all happening. Unity Project

@c0ffeeartc What part of Entitas is causing this bogus retain error then? Is it the fact that when entity.Destroy() is called that retention is detected and an error is reported? Is it the reactive system that recognises that the entity is in fact destroyed and complains then?

It seems that Execute(List<>) isn't called for the reactive system so I can assume Destroy()ed entities are never processed again (Of course reuse of an entity is another story but that's OK)

I really need to know what is the correct way to handle this. Either

1. fix my code by making sure I order perfectly the systems (This seems error prone and a huge cognitive burden).
2. Ignore the errors which will undoubtedly lead to developers just ignoring all retain errors which is not good. or
3. Fix Entitas in some way.

[c0ffeeartc]

I ran the attached project and result is - no error, red icon in visual debugging, entity gets retained by collector and then released and reused on the next frame.

It seems that Execute(List<>) isn't called for the reactive system so I can assume Destroy()ed entities are never processed again

Destroyed entities don't have components and are filtered by Reactive system then released.

1. fix my code by making sure I order perfectly the systems (This seems error prone and a huge cognitive burden).

Yes it's correct way. It's possible to create additional command that is processed at next frame so that all systems can handle it #610

2. Ignore the errors which will undoubtedly lead to developers just ignoring all retain errors which is not good. or

Please write error log from console.

3. Fix Entitas in some way.

Once the error is clear and there is solution it could happen

[akoolenbourke]

@c0ffeeartc OK so I think there is a misunderstanding about what an "error" is. What I am calling an error is the fact that I see a red icon in the visual debugger. Those red icons are meant to show problems AFAIC, and they also display big large warning texts in the inspector too (Context observer behaviour when clicking on a context in the hierarchy).

Are you saying that this is normal behaviour? That I should expect this red icon error and just ignore it? Seems strange to me. A big red icon and warning text suggests that something is wrong but if it's not then I think the visual debugger needs fixing so as to not confuse things. If there's lots of entities being dealt with this way I can imagine the tendency will be for a developer to just end up ignoring all warnings/errors completely which is not good.

I'm not sure how the

Yes it's correct way. It's possible to create additional command that is processed at next frame so that all systems can handle it #610

I'm not sure how that will help though. The entity has been destroyed but still retained. You'd have to make sure that any DestroySystem runs after any system that might flag for destruction but before any system that might cause collection. This seems like a massive limitation and cognitive burden.

Thanks for your help

[FNGgames]

Aaron, I've run your test project. I see what you're reporting but I believe this is perfectly normal, fine and reasonable behaviour. To make it clear what was happening, I added a frame timer to the SpewDebugMessageSystem so that it only spewed once every 10 frames.

What you see is that the entity is retained by the collector of DebugMessageSystem for the frame after it was created (as you have described).

At this point, it is passed into the system but rejected by the filter method (entity.hasDebugMessage evaluates to false because entity has no components after e.Destroy() is called in the cleanup system). The collector for the DebugMessageSystem is then cleared and the entity is no longer retained by anything so it is recycled into the pool and ready to be reused. This is exactly what the filter method is for.

There is absolutely nothing wrong with this behaviour: You call Destroy() and entitas just waits till there is nothing retaining the entity then pools it. What you see as a warning in the visual debugging is there so that you can identify entities that never get cleaned up. It shows for one frame here because the warning shows whenever there is a destroyed entity waiting to be pooled, but this is nothing to worry about. The warning becomes useful if it's sitting there 30 seconds later and the entity is still not being pooled, then you know you have a logical problem involving entity retention somewhere. In the example you gave though, seeing it flash for one frame is not a problem and shouldn't be worried about. It isn't an error message, it's an information message that can help you find errors if you need to.

So in terms of what that means for entitas design and system ordering:

(1) No you do not need to protect against systems invalidating an entity after it's been collected by another system.

(2) You do not have to meticulously design your system order to prevent this from happening, all you need to do is have sensible filters and these will stop the system hitting a null reference looking for a component that isn't there.

(3) If you really do want an entity to survive until it's processed by a specific system, even if that's on the next frame, what you would do is not destroy automatically it in a cleanup system, but instead flag if for destroy inside the system that processes it. In your example you would remove the system that auto-cleans up DebugMessage entities, and instead add a destroy system which reacts to a DestroyedComponent and put it at the end of the execution order. The idea is to say "hey I've processed it now so go ahead and destroy it", instead of assuming you're going to destroy every one that comes up within the frame on which it was created.

So with this setup you'd have:

DebugMessageSystem (ReacSys, Matcher.DebugMessage, Process message then call e.isDestroyed = true) SpewDebugMessageSystem (ExecSys, Creates entity with DebugMessageComponent) DestroySystem (CleanupSys for destroying entities with Destroyed component)

what you would get on frame 1 is DebugMessageSystem: nothing SpewDebugMessageSystem: new entity with Test component DestroySystem: nothing

then on frame 2 you have: DebugMessageSystem: processes entity with test component, flag it with DestroyedComponent SpewDebugMessageSystem: creates new entity with test component, ready for next frame DestroySystem: destroys entity that was created in frame 1.

Does that make sense?

[akoolenbourke]

Thanks a lot for taking the time to look at this for me - it's really appreciated.

From this I'd like to suggest maybe some changes to Entitas so that it detects these situations and doesn't report a wanting/error icon. I do understand that this might add more processing overhead and therefore is not desirable but if there was a way it'd be great. Even if it was an option for reporting such as (Don't warn on 1 frame retain age errors). I realise that this is hiding some details from the user about how entity lifetimes work.

Hey, even if this problem was documented somewhere it'd be great. I have wasted valuable time on this issue only to find out it's normal behaviour :)

Again, thanks a lot