Closed RefuX closed 2 months ago
Thank you! I will try to have a look by tomorrow and give you feedback :)
Had a quick check on my phone. In general looks good to me but need to have a more detailed look. Two things that I can already tell:
Updated with review feedback 👍
Updated with review feedback 👍
haha unlucky timing, I just did the detailed review in parallel. Will have another look this afternoon :)
Resolved the own hook conundrum using your first approach. Let me know if you like how it looks.
Looks better now but the hook stuff is still not correct. It is a little bit confusing because it is a single hook from a family point of view but actually multiple things can be called. Here is a scenario that describes it better:
worldCfgHook
. It is simply a function that takes an entity and does somethingFamilyOnAdd
interface to system A that iterates over the same family of the previous step. Let's call that systemAHook
.systemBHook
The special setUpAggregatedFamilyHooks
function is then combining all three hooks into a single function. The new function executes during an add "trigger" in following order:
worldCfgHook
systemAHook
systemBHook
For removal it is the same but in reversed order.
If you now remove system A of the world then the hook of the family remains but the new execution will be:
worldCfgHook
systemBHook
systemAHook
logic needs to be removed. When adding system A again, it needs to become part of the single hook function again depending on where the system was added.
I understand if this is a little complex for someone who does not know about that special use-case and all the details ;) I think this is also not covered by any of the test cases because otherwise you would already have failing tests.
I will have some time on the weekend and can help you with that. But of course, if you feel motivated to do it on your own then also feel free to do so :)
Everything else looks fine from my side now and can be merged. We are just missing the proper logic for this special aggregate hook
stuff.
@RefuX:
had a little bit of time this morning. In theory the code should work but needs to be verified in a proper test case. Please have a look as well:
1) I removed the ownAddHook
and ownRemoveHook
from the family again
2) WorldConfiguration is then directly setting again the addHook
/ removeHook
3) I split the setUpAggregatedFamilyHooks
method into two methods. The first one is initAggregatedFamilyHooks
which gets called after world configuration (it is basically the same as it was before). The second one is a new updateAggregatedFamilyHooks
method that only updates the add/remove hook of a single family. It gets called when a system is added/removed and the system has those special interfaces.
4) I introduced two "lazy caches" to remember the original world configuration hooks. I think this should be the best option from a memory consumption point of view.
The two lazy caches. I hope mutableMapOf supports null values:
/**
* Map of add [FamilyHook] out of the [WorldConfiguration].
* Only used if there are also aggregated system hooks for the family to remember
* its original world configuration hook (see [initAggregatedFamilyHooks] and [updateAggregatedFamilyHooks]).
*/
private val worldCfgFamilyAddHooks = mutableMapOf<Family, FamilyHook?>()
/**
* Map of remove [FamilyHook] out of the [WorldConfiguration].
* Only used if there are also aggregated system hooks for the family to remember
* its original world configuration hook (see [initAggregatedFamilyHooks] and [updateAggregatedFamilyHooks]).
*/
private val worldCfgFamilyRemoveHooks = mutableMapOf<Family, FamilyHook?>()
Updated system add:
/**
* Adds a new system to the world.
*
* @param index The position at which the system should be inserted in the list of systems. If null, the system is added at the end of the list.
* This parameter is optional and defaults to null.
* @param system The system to be added to the world. This should be an instance of a class that extends IntervalSystem.
*
* @throws FleksSystemAlreadyAddedException if the system was already added before.
*/
fun add(index: Int, system: IntervalSystem) {
if (systems.any { it::class == system::class }) {
throw FleksSystemAlreadyAddedException(system::class)
}
if (system is IteratingSystem && (system is FamilyOnAdd || system is FamilyOnRemove)) {
updateAggregatedFamilyHooks(system.family)
}
mutableSystems.add(index, system)
}
Updated system remove:
/**
* Removes the specified system from the world.
*
* @param system The system to be removed from the world. This should be an instance of a class that extends IntervalSystem.
* @return True if the system was successfully removed, false otherwise.
*/
fun remove(system: IntervalSystem) {
mutableSystems.remove(system)
if (system is IteratingSystem && (system is FamilyOnAdd || system is FamilyOnRemove)) {
updateAggregatedFamilyHooks(system.family)
}
}
The updated init aggregated hooks and update aggregated hooks:
/**
* Extend [Family.addHook] and [Family.removeHook] for all
* [systems][IteratingSystem] that implement [FamilyOnAdd] and/or [FamilyOnRemove].
*/
internal fun initAggregatedFamilyHooks() {
// validate systems against illegal interfaces
systems.forEach { system ->
// FamilyOnAdd and FamilyOnRemove interfaces are only meant to be used by IteratingSystem
if (system !is IteratingSystem) {
if (system is FamilyOnAdd) {
throw FleksWrongSystemInterfaceException(system::class, FamilyOnAdd::class)
}
if (system is FamilyOnRemove) {
throw FleksWrongSystemInterfaceException(system::class, FamilyOnRemove::class)
}
}
}
// register family hooks for IteratingSystem.FamilyOnAdd containing systems
systems
.mapNotNull { if (it is IteratingSystem && it is FamilyOnAdd) it else null }
.groupBy { it.family }
.forEach { entry ->
val (family, systemList) = entry
val ownHook = worldCfgFamilyAddHooks.getOrPut(family) { family.addHook }
family.addHook = if (ownHook != null) { entity ->
ownHook(this, entity)
systemList.forEach { it.onAddEntity(entity) }
} else { entity ->
systemList.forEach { it.onAddEntity(entity) }
}
}
// register family hooks for IteratingSystem.FamilyOnRemove containing systems
systems
.mapNotNull { if (it is IteratingSystem && it is FamilyOnRemove) it else null }
.groupBy { it.family }
.forEach { entry ->
val (family, systemList) = entry
val ownHook = worldCfgFamilyRemoveHooks.getOrPut(family) { family.removeHook as FamilyHook }
family.removeHook = if (ownHook != null) { entity ->
systemList.forEachReverse { it.onRemoveEntity(entity) }
ownHook(this, entity)
} else { entity ->
systemList.forEachReverse { it.onRemoveEntity(entity) }
}
}
}
/**
* Update [Family.addHook] and [Family.removeHook] for all
* [systems][IteratingSystem] that implement [FamilyOnAdd] and/or [FamilyOnRemove]
* and iterate over the given [family].
*/
private fun updateAggregatedFamilyHooks(family: Family) {
// system validation like in initAggregatedFamilyHooks is not necessary
// because it is already validated before (in initAggregatedFamilyHooks and in add/remove system)
// update family add hook by adding systems' onAddEntity calls after its original world cfg hook
val ownAddHook = worldCfgFamilyAddHooks.getOrPut(family) { family.addHook }
val addSystems = systems.filter { it is IteratingSystem && it is FamilyOnAdd && it.family == family }
family.addHook = if (ownAddHook != null) { entity ->
ownAddHook(this, entity)
addSystems.forEach { (it as FamilyOnAdd).onAddEntity(entity) }
} else { entity ->
addSystems.forEach { (it as FamilyOnAdd).onAddEntity(entity) }
}
// update family remove hook by adding systems' onRemoveEntity calls before its original world cfg hook
val ownRemoveHook = worldCfgFamilyRemoveHooks.getOrPut(family) { family.removeHook }
val removeSystems = systems.filter { it is IteratingSystem && it is FamilyOnRemove && it.family == family }
family.removeHook = if (ownRemoveHook != null) { entity ->
removeSystems.forEach { (it as FamilyOnRemove).onRemoveEntity(entity) }
ownRemoveHook(this, entity)
} else { entity ->
removeSystems.forEach { (it as FamilyOnRemove).onRemoveEntity(entity) }
}
}
Great, I was just about to start looking at this and was writing some test cases 👍
looks good now - thanks a lot for adding the new test cases to also verify this special hook behavior <3
Btw, is it okay for you if this feature is currently only available in SNAPSHOT version? Or do you need a stable release for it?
Btw, is it okay for you if this feature is currently only available in SNAPSHOT version? Or do you need a stable release for it?
I'm currently running off a local jar, so I'm not fussed 😎
It's warning me about unmerged commits, so maybe I can't close this yet.
Yeah, I did not merge it yet, just approved it :) Will merge it tomorrow.
One more thing that I noticed now: it seems like systems.forEach is now using an iterator in the generated Bytecode which is not ideal because that floods the garbage collector with useless objects in our scenario.
Can you please update that to use a for loop with indices inside world.update
?
@RefuX: should be ready in SNAPSHOT version in a few minutes. Thanks again for your contribution, especially for adding the test cases to cover the special hook behavior!
Great, thanks so much, using the SNAPSHOT 👍
I changed world.system to return a List instead on an Array, so while it didn't impact any tests, I guess its possible some users are expecting an Array?
I ran the benchmarks before and after changes and results are basically identical.
I added a constructor to IntervalSystem (to match old signature), as it was breaking some js/wasm tests otherwise.
Please review and give feedback.