Quillraven / Fleks

Fast, lightweight, multi-platform entity component system in Kotlin
MIT License
180 stars 20 forks source link

Proposal: Component add/remove hooks that don't rely on Component.onAdd/onRemove #120

Closed LobbyDivinus closed 1 year ago

LobbyDivinus commented 1 year ago

We can detect component addition / removal by overriding the onAdd / onRemove methods in our component class. However, by doing that our components become effectively more than simple data objects. Sure, we could include a reference to another handler for these events in the component and then call that handler's methods instead, but that kind of defeats the purpose of data only component classes. I expect this to also make serialization / deserialization harder when non serializable data is involved.

In my use case I am using a physics library that uses physics bodies which require manual cleanup logic when they get removed. My only option right now is to detect the removal via the onRemove method in the component to trigger the cleanup logic that depends on state stored outside of the ECS. In general I think it's a good design if systems (more broadly code that manages components) depend on components not the other way round.

I tried to use the family onRemove hook but noticed that these hooks get called after the component was removed already so I cannot use it for clean-up.

So what I propose is to add methods similar to the family onAdd / onRemove hooks that would internally register to ComponentsHolder and provide the component as a parameter when called.

LobbyDivinus commented 1 year ago

Here's a proof of concept implementation for onRemove not worthy of a PR, yet.

In FamilyConfiguration:

    inline fun <reified T: Component<*>> ComponentType<T>.onRemove(noinline callback: World.(T) -> Unit) {
        world.componentService.holder(this).onRemove = callback
    }

In ComponentsHolder:

    var onRemove: (World.(T) -> Unit)? = null  // Attribute
    // Call it in the two places where Component.onRemove() gets called if not null

Usage:

    families {
        SomeComponent.onRemove {
            // Do clean-up it
            println("Cleaned up")
        }
    }
Quillraven commented 1 year ago

Which library are you using and how does a cleanup look like? I use Box2D und to cleanup a body you need the physic world which is not stored in the component itself: https://github.com/Quillraven/MysticWoods/blob/fleks-2.0/core/src/main/kotlin/com/github/quillraven/mysticwoods/component/PhysicComponent.kt

I have some other examples for cleanups where you get the important objects for the cleanup with the inject method of the world inside onRemove of a component.

I currently don't see what is missing or what cannot be achieved with this approach.

edit: here is another example that removes an actor of a Scene2D stage when the component gets removed using the inject approach: https://github.com/Quillraven/MysticWoods/blob/fleks-2.0/core/src/main/kotlin/com/github/quillraven/mysticwoods/component/FloatingTextComponent.kt

LobbyDivinus commented 1 year ago

Hi, I'm using the Bullet physics wrapper included in libGDX. In that wrapper the btRigidBody class does not hold a reference to the world it is registered with. Using inject would work here, indeed.

Fair enough, my issue with the current approach is not of functional nature but an architectural one. In my mind components are not supposed to contain any logic.

Quillraven commented 1 year ago

You are correct that components should be "dumb" data classes but for me it is more intuitive to initialize and cleanup data of the component close to the component itself.

I don't really like adding an additional hook because imo we have all important hooks at the moment. Also, hooks always add some overhead meaning we should keep them to a minimum.

Quillraven commented 1 year ago

I think this issue can be closed. If not, please reopen it.

metaphore commented 11 months ago

Sorry to revive the old thread, but I bumped into something similar to what @LobbyDivinus mentioned

I need to clean up entities when their parent removed. It makes sense to set up a family's onRemove hook and do the necessary cleanup in there. But the major issue is that the hook gets called after the entity was removed and it's not possible to recover any data from it before it gets destroyed.

Here's a very simplified version of the case:

class Transform : Component<Transform> {
    val children = ArrayList<Transform>()
}

configureWorld {    
    val family = family { all(Transform) }

    onRemove(family) { entity ->
        val ownTransform = entity[Transform] // Exception here "Entity doesn't have Transform component"
        for (child: Transform in ownTransform.children)
            val childEntity = getEntity(child) // This is magic and another question, but let's keep it aside.
            childEntity.remove()
        }
    }
}

While this can be solved using Transform.onRemove() component hook, I'd still prefer to keep that "global" logic somewhere on the higher level and outside the data-oriented component code. This also doesn't work if the case involves more a complex family (e.g. multiple components).

@Quillraven Well, it all boils down to a simple question: is it possible to call the family's onRemove hook before the entity is reset?

Quillraven commented 11 months ago

@metaphore I think that should be possible. The method is part of entity.kt:

/**
     * Removes the given [entity]. If [delayRemoval] is set then the [entity]
     * is not removed immediately and instead will be cleaned up within the [cleanupDelays] function.
     *
     * Notifies all [families][World.allFamilies] when the [entity] gets removed.
     */
    operator fun minusAssign(entity: Entity) {
        if (entity !in entityProvider) {
            // entity is already removed
            return
        }

        if (delayRemoval) {
            delayedEntities += entity
        } else {
            entityProvider -= entity
            val compMask = compMasks[entity.id]

            // trigger optional remove hook
            removeHook?.invoke(world, entity)

            // remove components
            compMask.clearAndForEachSetBit { compId ->
                compService.holderByIndexOrNull(compId)?.minusAssign(entity)
            }

            // update families
            world.allFamilies.forEach { it.onEntityRemoved(entity) }
        }
    }

Just move the family loop before the remove components block. That should do the trick. Can you try that out in your example and let me know if that solves your issues? Then I can double check if any test in Fleks fails and if not, we can bring that to the master (=SNAPSHOT version for now).

metaphore commented 11 months ago

Thanks, Simon, it works like a charm (the family's onRemove hook)!

I also checked if onRemove for components has a similar problem. And yes it does. At the moment of Component#onRemove(Entity) the entity parameter is already stripped of all the components. I see that making it work in a similar fashion might be a bit tricky, but this is less of a problem since:

  1. A component instance de-factor has access to all the data of itself at the moment of onRemove.
  2. Accessing an entity's other components from within a component class is a little odd, to say the least.

But still, if it's possible to make the component's onRemove be called before the entity is removed would be a good thing.

Quillraven commented 11 months ago

@metaphore: I did the adjustment now and moved the family hook before components are removed. Tests are still passing. Thank you for also trying it out in your example! The SNAPSHOT version should soon be live including this change (and also pumping Kotlin to 1.9.21 and serialization to 1.6.1).

For the components: I don't see an easy and efficient way to achieve your request. The only thing I see would be two separate loops whenever an entity gets removed. The first loop to call each component's remove hook and a second loop to then really remove the component (which would currently call the removehook once more).

I am also wondering again why you would need the information of another component in the cleanup logic of a component. Sounds to me more like you want to work on a family basis instead of component level and there you can achieve it now with the new change.

Also:

Accessing an entity's other components from within a component class is a little odd, to say the least.

Again, I think this should be a family matter and not a component matter but the component hooks are running within the context of a world, so you can write the usual syntax like:

override fun World.onAdd(entity: Entity) {
    val otherCmpVar = entity[MyComponent].someVar
    // ...
}

Just be aware that this will not work properly because it depends on the id of MyComponent and the component itself. If MyComponent has a lower id than component, then it gets removed before and you cannot access it. If it has a higher id then it should work. But again (third time 😊) I don't recommed it. Family should be the better option in such cases.

metaphore commented 11 months ago

Thanks, Simon!

I think this should be a family matter and not a component matter

Yep, that makes total sense, I fully agree with that 👍

Just be aware that this will not work properly because it depends on the id of component

The only note I'd make is it would be good to highlight this somewhere in the documentation. Because for me it was expected that in the hook calls, the entity wasn't cleaned yet, and its components can be accessed there.