Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#124: add onEnabled/onDisabled functions to IntervalSystem #127

Merged
merged 2 commits into from
Nov 28, 2023

Conversation

Quillraven
Copy link
Owner

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?

@Quillraven Quillraven added the enhancement New feature or request label Nov 27, 2023
@Quillraven Quillraven added this to the 2.6 milestone Nov 27, 2023
@metaphore
Copy link
Contributor

metaphore commented Nov 27, 2023

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

@Quillraven
Copy link
Owner Author

Quillraven commented Nov 28, 2023

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.

@metaphore
Copy link
Contributor

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.

@Quillraven Quillraven merged commit 98f9428 into master Nov 28, 2023
4 checks passed
@Quillraven Quillraven deleted the #124_enable_virtual_functions branch November 28, 2023 12:57
@Quillraven Quillraven mentioned this pull request Dec 12, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants