#123: aggregated family hooks that trigger IntervalSystems

[metaphore]

This PR picks up on #123 and implements family hooks for IteratingSystem

1. The PR is raw and requires docs for the new functionality.
2. At the moment of PR it's still unclear what's the best way to opt-in for the family onAdd/onRemove hooks. So, just for the proof of concept, I went ahead and used the simplest approach (IteratingSystem.familyHooks boolean flag). We can easily switch to any other approach, I have no strong preference for it.
3. The current implementation calls onAdd hooks in forward order and onRemove hooks in reverse.
4. I also added a simple control flow test (SystemFamilyHooksTest.kt). It might be not the best example of a test, but it illustrates and covers the expected hook call order.

@Quillraven please let me know what you think.

[Quillraven]

Thank you for the contribution! It looks already pretty good. I have some feedback though:

• Why do we need to call remove hooks in reverse order? Imo the order ideally shouldn't matter at all but if it does then why not have the same order for add and remove?
• I am still not a big fan of the extra parameter on the class to register hooks :( I prefer FamilyHook properties (or a new alternative approach, see below)
• The resulting family hook can be optimized because currently it will always check if it has its own hook or not every time it gets executed. This can be checked before and the result hook is then either "own hook + systems" or just system hooks.

I fiddled around yesterday a little bit with Kotlin features to combine lambdas. Your version is fine but in case you are interested, it might shorten the hook setup code a little bit. Output is "global class1 class2":

typealias FamilyHook = () -> Unit

val globalHook: FamilyHook = {
    println("global")
}

abstract class System {
    open val familyHook: FamilyHook? = null
}

class System1 : System() {
    override val familyHook = {
        println("class1")
    }
}

class System2 : System() {
    override val familyHook = {
        println("class2")
    }
}

class System3 : System()

fun main() {
    val systems = listOf(System1(), System2(), System3())
    val systemHooks: List<FamilyHook> = systems.mapNotNull { it.familyHook }

    // if there is no globalHook it should just be { systemHooks.forEach { it() } }
    val resultHook: () -> Unit = { globalHook().also { systemHooks.forEach { it() } } }
    resultHook()
}

---

I also read several articles yesterday about how to find out if a method is overridden by a subclass in Kotlin. From what I know there is no multi-platform way to do it and the only one that I found was using kotlin-reflect library which I don't want to introduce and I think it also only worked for JVM.

Therefore, unfortunately I don't see a way to achieve it with methods and that's why I'd vote for FamilyHook properties like we are already doing it in the WorldConfiguration or with EntityComparators.

What I don't like here (besidese that methods are more intuitive in the context of a class) is that we need two separate properties for each system class for add and remove hook. This brings me to a new idea.

---

It is similar to what we briefly discussed in the issue. Something like:

configureWorld {
  systems {
    add(MySystem(), MySystem::onAddEntity, MySystem::onRemoveEntity)
  }
}

The idea is that users can optionally create methods in their systems that take an entity and return nothing (=FamilyHook). They can then register it in the world configuration and we can then create the final family hook like you are doing it right now.

The advantage of this approach is:

• You can use normal methods in your systems
• We don't need extra properties for each system

The downside is some extra configuration in the systems block and that you don't override predefined methods in the system BUT I am not sure if those are really bad things.

Because to be honest what we are doing here can already be done with the already existing FamilyHook functionality anyway. It is just some nice "syntax sugar" to set it up in a more intuitive way.

Anyway, I am still not sure what is the best way. What do you think about the new idea?

[metaphore]

Why do we need to call remove hooks in reverse order?

It actually crossed my mind at the very last moment and I decided to include it here. But it sounds like it is its own big thing, it touches some areas outside the onEntityRemove order (like the system dispose order) and should be discussed separately. I will remove this aspect from the current PR and spawn a new issue for discussion. I will collect my thoughts and reasoning there.

The idea is that users can optionally create methods in their systems that take an entity and return nothing (=FamilyHook). They can then register it in the world configuration and we can then create the final family hook like you are doing it right now.

Boy, it goes deep, haha. I like your solution of specifying the hooks through system configuration. It's the most explicit and flexible, but also leads to some potentially long declarations like

add(TheVeryLongSystemNameThatWeCannotShoten0(), TheVeryLongSystemNameThatWeCannotShoten0::onAddEntity, TheVeryLongSystemNameThatWeCannotShoten0::onRemoveEntity)
add(TheVeryLongSystemNameThatWeCannotShoten1(), TheVeryLongSystemNameThatWeCannotShoten1::onAddEntity, TheVeryLongSystemNameThatWeCannotShoten1::onRemoveEntity)

I have another idea from the old OOP world, we can introduce two interfaces OnEntityAddFamilyHook/OnEntityRemoveFamilyHook that would be the markers for any IteratingSystem to opt-in for the hooks. Something like

interface OnEntityAddFamilyHook {
    fun onEntityAdd(entity: Entity)
}

interface OnEntityAddFamilyHook {
    fun onEntityRemove(entity: Entity)
}

class MySystem : IteratingSystem(family { }),
    OnEntityAddFamilyHook,
    OnEntityRemoveFamilyHook {

    override fun OnEntityAdd(entity: Entity) { }

    override fun onEntityRemove(entity: Entity) { }
}

That should keep it simple enough while still being explicit. The only confusion could be if someone tries to add those interfaces to IntervalSystem and expects them to work somehow. But you know, read the docs and don't do stupid things... 🙂

I fiddled around yesterday a little bit with Kotlin features to combine lambdas. Your version is fine but in case you are interested, it might shorten the hook setup code a little bit.

This looks good, thanks! I will adjust the hook aggregation code to your recommendations. The only thing that always bothers me is the way Kotlin compiler treats systemHooks.forEach { it() }. It's really hard to tell if that would be rendered to an iterator or a classic indexed for loop (which is preferred in performance-critical sections). Only for that reason, I decided to write the for loop explicitly.

[Quillraven]

I like the idea of the interfaces although it is a new concept in Fleks that was never used but I think in this scenario it is totally fine.
However, I'd shorten the names because I hate long names. Something like OnAddEntity or FamilyOnAdd or something.

For the problem with the IntervalSystem: I guess this can also be verified in the world configuration when adding a system. If it is an IntervalSystem and also implements one of those two interfaces then throw an exception.

[metaphore]

@Quillraven the PR is ready, please review it.

• Use IteratingSystem.FamilyOnAdd and IteratingSystem.FamilyOnRemove to subscribe systems for family hooks.
• Check and throw meaningful exceptions when the new interfaces are added to non-IteratingSystem systems.
• Aggregated family hooks now created using more idiomatic Kotlin syntax.
• onRemove hooks get called in forward order (and not in reverse).
• The system family hook test updated and merged into the main test file for the system tests (SystemTest.kt)

[Quillraven]

Looks good to me besides some minor details.

About the test: please extract it to a separate file like FamilySystemHookTest or something similar. The reason is that - to be honest - the test files are pretty messy in my opinion and sometimes quite big with a lot of class definitions at the top. This happened in the first versions of Fleks where I just wanted to quickly write down the tests for all the cases I had in mind and I didn't really care a lot about the beauty of the test cases ;)

For new features like this, in order to not make the already messy test files even messier, please create a new file.

Also, can you please add a test to what happens, when an entity gets created in the constructor of a system and that would end up in the family? I am pretty sure that we already have a similar test for this but I am not 100% sure. Such entities should still call the onAdd family hook.

And besides that I want to thank you for your time and contribution! It is a cool convenience feature and good addition to Fleks! When I release a new stable version, I will also document this new feature in the wiki.

edit: and also please merge/rebase to master once more because there is a conflict in SystemTest.kt.

[metaphore]

@Quillraven thanks for the feedback. I made the necessary changes.

Please note, that currently global family hooks and system family hooks work inconsistently unless the world is fully configured (aggregated hooks get created at the very end of WorldConfiguration.configure()). Thus it should be discouraged to modify the world in the system constructors.

This links back to my onInitialize proposal. And I still think that the ECS in general would benefit from the strict ordered onInitialize and onDispose lifecycle calls. It would help to eliminate any future world configuration side effects.

[Quillraven]

Tests look a lot cleaner now, thank you!

What do you mean with the inconsistency? I know that I took extra care for families to behave correctly when entities get created in a system's constructor, if I remember correctly. Should be this code piece:

 internal fun family(definition: FamilyDefinition): Family {
        if (definition.isEmpty()) {
            throw FleksFamilyException(definition)
        }

        val (defAll, defNone, defAny) = definition

        var family = allFamilies.find { it.allOf == defAll && it.noneOf == defNone && it.anyOf == defAny }
        if (family == null) {
            family = Family(defAll, defNone, defAny, this)
            allFamilies += family
            // initialize a newly created family by notifying it for any already existing entity
+            entityService.forEach { family.onEntityCfgChanged(it, entityService.compMasks[it.id]) }
        }
        return family
    }

What happens in following scenario?

• SystemA with FamilyA creates an entity for FamilyA in its constructor (this should already work and also trigger the add hook correctly)
• SystemB with FamilyA: I guess this system does not get called for the add hook right now?

I assume this works with the exisitng global hook configuration because that is configured before systems are created and the hook logic itself is final and therefore it just works.

With these new changes now I assume SystemB is not notified because it was not initialized/created when the entity in SystemA is created.

If that is true we need to come up with a solution for it. A quick idea is that in your setUpAggregatedFamilyHooks function, we know if a system is interested in an onAdd notification. We could then simply call its onAdd method programmatically for any existing entity in the family of the system.
This should be rather simple but of course SystemB could also create entities in its constructor and for those entities it should not get called again because otherwise it will be called twice for those entities. So somehow we'd need to get the entities of the system's family BEFORE the system is created.

I am of course not sure if it behaves like that because I didn't have time to try it out but I think it does. Anyway, I am sure there is a solution for it and it is hopefully not super complicated to solve. I guess it can be solved with a "one-time special care" logic during configuration phase.

Also, adding an onInit method to a system that gets called once after the world configuration is done is fine for me as a change. I think it would also solve the issue I described above BUT it does not prevent users from still doing the thing from above. So it is better to solve it because such things can be really difficult to identify bugs.

[Quillraven]

I think the entity creation in a system's constructor is a rather exceptional use-case. I think I never did that but I still wanted to support it because someone might need it.

However, I am not sure if the problem above is easily solveable, so an alternative might be:

• Add an onInit method to systems that gets called AFTER all world configuration is done for each system
• If someone needs entity creation at system creation then he needs to put it in the onInit method
• We can check if numEntities of the world is not zero after the configuration. If that's the case we throw an exception that entity creation in system constructors is not allowed and should be placed in onInit method

[metaphore]

I agree with your reasoning that supporting entity creation in system constructors is important, but just for the sake of backward compatibility. However, moving forward with the configuration features, it would be crucial to impose this one rule that the world cannot be modified (entities added) during configuration time. It would help to keep things simple in the configuration logic, without a need to handle such edge cases. I'm all for the introduction of onInit and throwing an exception if entities were added before the configuration finishes.

I'll add this functionality with tests to the PR

[metaphore]

• onInit added to systems.
• Check if entities were added to the world at the end of world configuration and throw a meaningful exception.
• Tests are updated to follow the new rule.

@Quillraven please review

[metaphore]

A little update. I cleaned up things a bit in the hook creation code and explicitly turned the hook-cached system list to an array. That way both family hooks and the world keep the systems in arrays. This will help later introduce a unified forEachReverse() extension function that will also be required in world.dispose() for #129 .

[Quillraven]

Cool stuff, thank you for all your effort and sorry that I can only review and give some comments at the moment to such bigger changes :(

I have two minor remarks (see above) but besides that the PR is ready to be merged - good job!

edit: hope you are happy with your onInit method now 😛

[metaphore]

No worries and thanks for your guidance! I like how polished and clean the project is and it is best to keep it that way. Totally worth it!

hope you are happy with your onInit method now 😛

Absolutely, haha! I already tried the changes to the library on my project to achieve the cases I described and it works flawlessly! This probably will be that "breaking change" thing in the changelog, but I hope it will pay off and serve as a foundation for any future configuration improvements.