Closed Quillraven closed 11 months ago
Looking good!
we should call onEnabled/onDisabled also when a system gets created
That sounds good, but in order to make them fully mirrored lifecycle events, that would require an extra call to onDisabled
right before the system is destroyed. That gets into the area of the "missing" initialize()
method that I proposed earlier. But we probably shouldn't go down that road now and can get away with a simple state change listener:
var enabled: Boolean = enabled
set(value) {
if (field == value) {
return
}
field = value
onEnabledChange(value)
}
/**
* This function gets called whenever the system [enabled] state changes.
*/
open fun onEnabledChange(enabled: Boolean) = Unit
I fixed the issue that the methods got called when enabled
didn't change. Also, I kept them as two separate methods because I prefer it that way. In Fleks I always tried to have separate methods for add/remove stuff because there are always cases where you don't need both and I find it annoying to force people to implement a method that isn't needed.
In that specific case I am not a big fan of passing a boolean parameter to such methods because you then end up with:
fun myFun(value: Boolean) {
if(value) {
// do that
} else {
// do other stuff
}
}
So you are forced to add a nesting level to either execute the "true" path or the "false" path. I guess it is personal preference but I prefer to have separate methods for those two paths and be able to skip one, if not needed at all.
I prefer to have separate methods for those two paths and be able to skip one, if not needed at all.
I mean there's nothing wrong with that, I agree it's cleaner and more convenient. My only reasoning for onEnebledChange
was that if onEnable
and onDisable
are not called on the system start and onDispose
, it might be confusing for someone who is not aware of that. But anyway, if you need those to be called just as I explained for a particular system you can do it manually. So lets just keep the PR as is and see if anyone ever complains about it.
PR for #124
@metaphore: Are you fine with that change? I am wondering, if we should call onEnabled/onDisabled also when a system gets created. I mean that we explicitly set it on the constructor (=
init
) block of an IteratingSystem to trigger those functions initially. But I am not 100% sure if this is a good idea. Maybe it is better to really just trigger them, when setEnabled is explicitly called by the user. What do you think?