refactor: Removed parameter Component.updateTree({callOwnUpdate})

[st-pasha]

Description

• Component.updateTree() method will no longer have the optional parameter callOwnUpdate.
• The Game class now has method .updateTree(), which by default redirects to Game.update(), but alternative implementations may be provided by the derived class.
• Method FlameGame.update() now works according to how Component.update() is expected to work, not how Game.update() is expected to work. If you need Game.update() functionality for FlameGame, call FlameGame.updateTree().

Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• My PR includes unit or integration tests for all changed/updated/fixed behaviors and are passing (See Contributor Guide).
• My PR does not decrease the code coverage, or I have a very special case and explained on the PR description why this PR decreases the coverage.
• I updated/added relevant documentation (doc comments with ///) and updated/added examples in doc/examples.
• I have formatted my code with flutter format and the flutter analyze does not report any problems.
• I read and followed the Flame Style Guide.
• I removed the Draft status, by clicking on the Ready for review button in this PR.
• I am willing to follow-up on review comments in a timely manner.

Breaking Change

• No, this is not a breaking change.

Related Issues

Closes #1219

[spydon]

I can't say I'm a big fan of this solution, having overlapping functionality on different methods becomes confusing I think. The current solution isn't nice either, but I hope that we can come up with something else than this.

I would actually rather expose updateTree than doing this.

[st-pasha]

I would actually rather expose updateTree than doing this.

Alright, I renamed tick() into updateTree().

[st-pasha]

I would actually rather expose updateTree than doing this.

Alright, I renamed tick() into updateTree().

Turns out this won't work:

• If Game class implements updateTree(dt) => update(dt), then FlameGame will have inherited this implementation and has no means of reverting it. Indeed, in order to get back to normal Component's updateTree, the FlameGame would have to write updateTree(dt) => super.super.updateTree(dt), but that's not valid in Dart;
• If Game class leaves updateTree unimplemented, then it becomes a breaking change, since all other classes that derive from Game would no longer work without implementing this method.

[spydon]

Turns out this won't work:

• If Game class implements updateTree(dt) => update(dt), then FlameGame will have inherited this implementation and has no means of reverting it. Indeed, in order to get back to normal Component's updateTree, the FlameGame would have to write updateTree(dt) => super.super.updateTree(dt), but that's not valid in Dart;

I don't think I follow, why can't FlameGame just override the Game implementation of updateTree?

[st-pasha]

I don't think I follow, why can't FlameGame just override the Game implementation of updateTree?

I mean, we could just copy-paste the code from Component.updateTree, but as far as I can see, there is no way for FlameGame to call that implementation directly. Or is there?

[st-pasha]

there is no way

Ok, scratch that. There is a way, and it's even a very nice way, but it necessitates a breaking change.

So, the trick is to replace

FlameGame extends Component with Game

with

FlameGame extends Game with Component

So, this is a big conceptual change. But it's a nice change: it's much easier to think of FlameGame or OxygenGame as extending the abstract class Game rather than mixing it in.

It would also be a breaking change, because in order to be able to use Component as a mixin, it must not have a constructor. Currently, it has a constructor that optionally takes the priority argument. The constructor can be removed in lieu of setting the priority value after construction, but this would probably be breaking for a small number of users.

I wouldn't recommend such change just for the sake of being able to call the method updateTree instead of tick, but it seems like it could be a great change on its own -- WDYT @spydon?

[spydon]

I wouldn't recommend such change just for the sake of being able to call the method updateTree instead of tick, but it seems like it could be a great change on its own -- WDYT @spydon?

If we are having a breaking change here, I think it would be nicer to just add updateTree unimplemented to Game. Then FlameGame will also keep consistency with how the other components are implemented.

It would also be a breaking change, because in order to be able to use Component as a mixin, it must not have a constructor. Currently, it has a constructor that optionally takes the priority argument. The constructor can be removed in lieu of setting the priority value after construction, but this would probably be breaking for a small number of users.

We want to keep priority on the constructor here, since that is consistent from Component to all PositionComponents (which are most of our components). This change would be a breaking change affecting more users than the actual change with mixing in Component instead of extending it on the Game (since not many write their own game classes that aren't extending FlameGame).

[erickzanardo]

I think it would be nicer to just add updateTree unimplemented to Game.

updateTree is a FCS concept though, so it don't make much sense to have it on game.

Honestly, I prefer the way we have right now, I do agreed that it has problems, but I think I prefer those problems that any other alternative so far. We could mitigate those problems by having some section on our docs deeply explaining what callOwnUpdate is and why it exists.

[st-pasha]

We want to keep priority on the constructor here, since that is consistent from Component to all PositionComponents (which are most of our components).

PositionComponent won't be affected here:

class PositionComponent extends Component {
  PositionComponent({..., int? priority}) {
    changePriorityWithoutResorting(priority); // or we could make a simple setter in Component
  }
}

So the only users that would be affected are those that inherit from Component directly and call its constructor explicitly -- which, in my opinion, is a small minority.

That being said, I'm wary in general about creating methods in Game that have exactly the same name as methods in Component, because each such method spells trouble for FlameGame class. Sure, those methods might have same purpose now; but nobody guarantees that that purpose won't change in the future (as already happened with update). It is safer to have non-overlapping method names in two classes.

[spydon]

So the only users that would be affected are those that inherit from Component directly and call its constructor explicitly -- which, in my opinion, is a small minority.

Quite a lot of people use Component directly I would say, for building levels and screens, most of them probably don't use priority on that level though like you say. But it is about consistency, just like we want to be consistent of how the components are created, so that it is not done like a mixin in FlameGame, but inheritance in other components.

That being said, I'm wary in general about creating methods in Game that have exactly the same name as methods in Component, because each such method spells trouble for FlameGame class. Sure, those methods might have same purpose now; but nobody guarantees that that purpose won't change in the future (as already happened with update). It is safer to have non-overlapping method names in two classes.

We don't guarantee that, but we are in control of it. If we want to make breaking changes on that level we should of course be wary. But I think I'm leaning more to Erick's solution than the last one I suggested too, that we just keep it the way it currently is and document callOwnUpdate better (we haven't had any questions about it so far as far as I'm aware).

[st-pasha]

@spydon I hope we can at least agree on the fact that our current class hierarchy is ungood. We have Game class, which is great, and Component which is also fine -- but then FlameGame tries to be both a Game and a Component at the same time. Now, Dart doesn't allow multi-inheritance (and for a good reason), so we tried to circumvent that by declaring Game as a mixin. However, that's cheating: Game is conceptually the abstract base class, not a mixin. And FlameGame is conceptually closer to Game than to Component. So, we sacrificed the hierarchical integrity of our class system in order to make FlameGame work -- and it does work, with some warts.

And one of those warts is the fact that both Game and Component have the update() method, but they do different things. So we sort of have to make another hack in order to fix that: the callOwnUpdate parameter.

The problem then is that this hack propagates into the rest of the Component hierarchy. And that's bad, because it makes the original code smell endemic, and much harder to fix in the future.

we haven't had any questions about it so far as far as I'm aware

Well, except for #1224, obviously. I mean, just like renderTree, it's a very advanced feature needed for not so many users. But I did ran in this problem working on #1218, so it's not an abstract consideration.

we just keep it the way it currently is and document callOwnUpdate better

I do not see how this solves the original concern that the parameter cannot be respected by the implementations at all. And if the documentation says "ok you have this parameter, but feel free to ignore it" -- then what kind of parameter is it?

---

That being said, there are other methods for dealing with the FlameGame.update() method that do not involve adding extra function in Game. So maybe we should explore those?

[spydon]

Well, except for #1224, obviously. I mean, just like renderTree, it's a very advanced feature needed for not so many users. But I did ran in this problem working on #1218, so it's not an abstract consideration.

#1224 is this PR, I guess you meant another issue/PR?

That being said, there are other methods for dealing with the FlameGame.update() method that do not involve adding extra function in Game. So maybe we should explore those?

Absolutely, if you have more ideas, put them on the table. :)

[spydon]

Maybe the best would be to just duplicate the lines from Component to FlameGame, like I think you suggested in the beginning @st-pasha? So that it would be something like this in FlameGame:

  @override
  @mustCallSuper
  void update(double dt) {
    super.update(dt);
    _cameraWrapper.update(dt);
    if (parent == null) {
      updateTree(dt);
    }
  }
  
  @override
  void updateTree(double dt) {
    children.updateComponentList();
    if (parent != null) {
      update(dt);
    }
    children.forEach((c) => c.updateTree(dt));
  }

[st-pasha]

^ Yes, this should work. And, I mean, it's not such a big code chunk anyways...

Another possibility would be to add private variable bool _preventUpdate in FlameGame, so that it would look something like this:

class FlameGame {
  bool _preventUpdate = false;
  void update(double dt) {
    if (_preventUpdate) return;
    _cameraWrapper.update(dt);
    if (parent == null) {
      _preventUpdate = true;
      super.updateTree(dt);
      _preventUpdate = false;
    }
  }
}

[spydon]

^ Yes, this should work. And, I mean, it's not such a big code chunk anyways...

Another possibility would be to add private variable bool _preventUpdate in FlameGame, so that it would look something like this:

class FlameGame {
  bool _preventUpdate = false;
  void update(double dt) {
    if (_preventUpdate) return;
    _cameraWrapper.update(dt);
    if (parent == null) {
      _preventUpdate = true;
      super.updateTree(dt);
      _preventUpdate = false;
    }
  }
}

The variable way would call an overridden version of update twice, even if super was called.

[st-pasha]

The variable way would call an overridden version of update twice, even if super was called.

The idea here is that first the user calls update(), then update() sets flag preventUpdate to true and calls updateTree(). The method updateTree() would then call update() again, as it normally does, but the first statement in update() checks the flag preventUpdate, sees that it is true, and quickly exits as if the call didn't happen. The updateTree then finishes normally, the execution returns back to the original update, at which point the flag preventUpdate is set to false again. Thus, next time the system calls update(), it works normally again.

It might sound complicated, but think of it as passing the parameter callOwnUpdate=false not as an actual parameter, but as an environment variable.

[spydon]

The variable way would call an overridden version of update twice, even if super was called.

The idea here is that first the user calls update(), then update() sets flag preventUpdate to true and calls updateTree(). The method updateTree() would then call update() again, as it normally does, but the first statement in update() checks the flag preventUpdate, sees that it is true, and quickly exits as if the call didn't happen. The updateTree then finishes normally, the execution returns back to the original update, at which point the flag preventUpdate is set to false again. Thus, next time the system calls update(), it works normally again.

It might sound complicated, but think of it as passing the parameter callOwnUpdate=false not as an actual parameter, but as an environment variable.

Sure, I understand how it works, but imagine this scenario:

class MyGame extends FlameGame {
  @override
  void update(double dt) {
    super.update(dt);
    print(1);
  }
}

When that is added directly to the game widget update will be called, and then for each update call, since updateTree also calls update, the print(1) statement will be called again, so only the original super.update code will be skipped due to the variable.

[st-pasha]

I see, makes sense.
In that case we could declare update as @nonVirtual and make the user override updateTree instead, but this does feel kinda heavy-handed. Let's try going with your suggestion above and see how it shapes out.

[spydon]

Just a minor comment, otherwise lgtm!