flame icon indicating copy to clipboard operation
flame copied to clipboard

Add onParentResize lifecycle method

Open st-pasha opened this issue 3 years ago • 15 comments

Currently we have the onGameResize() lifecycle method which fires first before the component's onLoad(), and then every time the game canvas changes.

This FR is about creating an alternative lifecycle event onParentResize() that would be conceptually similar to onGameResize, but the deficiencies of the latter. The onGameResize method will remain as-is, thus this FR will not introduce any breaking changes.

The logic of onParentResize() is as follows:

  • Whenever a component changes size, it will call onParentResize() for all its children.
  • If the child does NOT change its size, then onParentResize() is not propagated deeper into the tree.
  • This method is NOT invoked before onLoad().
  • This method is invoked for the FlameGame class when the widget size changes.

st-pasha avatar Mar 07 '22 09:03 st-pasha

Isn't it the same for the user to just add a callback to the parent's NotifyingVector2 size when they need this functionality? Then we don't have to do lots of onParentResize calls where they aren't asked for.

spydon avatar Mar 07 '22 09:03 spydon

NotifyingVector is not available on Canvas or Camera, plus it's definitely not as convenient.

At the same time, resize events happen too rarely to worry about performance implications.

st-pasha avatar Mar 07 '22 11:03 st-pasha

NotifyingVector is not available on Canvas or Camera

How can a Canvas be a parent of a component? Camera as in the CameraComponent?

plus it's definitely not as convenient.

I think it is more convenient in some sense, since you can add a listener dynamically at runtime, but you can't do the same with overriding onParentResize. I see the value of it now though, let's see what the others say @flame-engine/flame-colab.

At the same time, resize events happen too rarely to worry about performance implications.

True

spydon avatar Mar 07 '22 12:03 spydon

How can a Canvas be a parent of a component?

FlameGame is mounted into a GameWidget, thus for the FlameGame the "parent" is the Canvas.

I think it is more convenient in some sense

Listening to the .size vector is already possible, so this adds an alternative way of responding to resize events without preventing the other way.

st-pasha avatar Mar 07 '22 18:03 st-pasha

For example, one use case is this: suppose there is a game and you want to change the sizes/layout of the game elements when the canvas changes. Naturally you would assume that onGameResize would be a good fit here.

However, since FlameGame.onGameResize is called before the game was either loaded or mounted, we end up in a state where nothing can be properly sized because game elements hasn't been properly created yet. The only solution today is to manually create a flag to keep track when it is safe to resize. Ideally, though, this kind of logic should be handled by Flame itself.

st-pasha avatar Mar 07 '22 21:03 st-pasha

Maybe onGameResize should just be called after onLoad instead, but we´d have to make sure that the game's size is still set before onLoad. I think it will add some confusion having both onGameResize and onParentResize.

spydon avatar Mar 09 '22 21:03 spydon

Ideally, of course, we would deprecate and eventually remove onGameResize. But for now we can start with creating a viable alternative: onParentResize, without causing any breaking changes.

I think it will add some confusion having both onGameResize and onParentResize.

Generally, a confusion can be remedied via clear documentation. I do not anticipate that anyone would need to use both methods at the same time -- only one or another.

Note that onParentResize has a wider use-case: it triggers not only when the game canvas is resized (which doesn't happen very often), but also when any other component resizes (which could be more frequent).

st-pasha avatar Mar 09 '22 21:03 st-pasha

Ideally, of course, we would deprecate and eventually remove onGameResize. But for now we can start with creating a viable alternative: onParentResize, without causing any breaking changes.

Tthey seem to have different usecases though? The onGameResize would be for adjusting your component to the game size, responsively for example, but what would it mean if a child of a plain Component gets their onParentResize called, when its parent doesn't even have a size? Maybe the name isn't optimal, because I guess that you intend it to be recursive in those cases?

Generally, a confusion can be remedied via clear documentation. I do not anticipate that anyone would need to use both methods at the same time -- only one or another.

A good API is still prio one, saying that anything can be done as long as it has clear documentation is not the way to go imho.

Note that onParentResize has a wider use-case: it triggers not only when the game canvas is resized (which doesn't happen very often), but also when any other component resizes (which could be more frequent).

All those other usecases seems to me like they could be covered by a listener to the size that you care about though?

I agree that we should do something about onGameResize being called before onLoad, but I'm not convinced this is a good solution.

spydon avatar Mar 09 '22 22:03 spydon

but what would it mean if a child of a plain Component gets their onParentResize called, when its parent doesn't even have a size?

So, onParentResize is called by the parent whenever it changes its size. Since plain Component doesn't have a notion of size, it won't be calling onParentResize for its children. The PositionComponent, on the other hand, will. Generally, onParentResize is called on the immediate children only, and doesn't propagate deeper inside the tree (unless the children decide to do that themselves). Thus, it'll look something like this:

void changeSize(Vector2 newSize) {
  size = newSize;
  children.forEach((child) => child.onParentResize());
}

A good API is still prio one, saying that anything can be done as long as it has clear documentation is not the way to go imho.

I concur, a good API is important. And as I argue, onParentResize is a better API than onGameResize in general. We would have to have them coexist for a while, because of the backward-compatibility promise, but that shouldn't become a barrier to progress.

All those other usecases seems to me like they could be covered by a listener to the size that you care about though?

The problem with size-listener approach is that (1) it's more cumbersome -- you need to both subscribe and unsubscribe to this event; and (2) it's not universal -- not all components with size utilize notifying vectors.

st-pasha avatar Mar 10 '22 06:03 st-pasha

So, onParentResize is called by the parent whenever it changes its size. Since plain Component doesn't have a notion of size, it won't be calling onParentResize for its children. The PositionComponent, on the other hand, will. Generally, onParentResize is called on the immediate children only, and doesn't propagate deeper inside the tree (unless the children decide to do that themselves). Thus, it'll look something like this:

void changeSize(Vector2 newSize) {
  size = newSize;
  children.forEach((child) => child.onParentResize());
}

That won't work for replacing onGameResize then? Imagine for example a Compoment called level containing all the PositionCompoments for a level, how would those PositionComponents resize when the game is resized when onGameResize is removed?

spydon avatar Mar 10 '22 06:03 spydon

That won't work for replacing onGameResize then? Imagine for example a Compoment called level containing all the PositionCompoments for a level, how would those PositionComponents resize when the game is resized when onGameResize is removed?

Well, there is a number of options, depending on the game type:

  • The simplest alternative to achieve the same behavior as we have right now, is to write in the top level FlameGame class

    void onParentResize() {
      propagateToChildren((c) => c.onGameResize(canvasSize));
    }
    

    This way every single component in the game will be informed that the game's canvas has changed.

  • Within the main FlameGame class have a function that computes the overall layout in a centralized location and then informs every corresponding component of their new placement.

  • Each component does its own thing in response to onParentResize, as described. For example, level (aka "world") doesn't change in size when the canvas changes, so there is no reason for elements that are within the level to be aware of such change. At the same time, the camera (and the viewport) will change in size, so any component that is attached to the viewport (the "HUD components") will be informed that there was a change in their parent's size.

    Note how this approach is more flexible: a viewport may change in size even if the canvas has not, and HUD components definitely want to be informed of such changes.

st-pasha avatar Mar 10 '22 07:03 st-pasha

Well, there is a number of options, depending on the game type:

* The simplest alternative to achieve the same behavior as we have right now, is to write in the top level `FlameGame` class
  ```dart
  void onParentResize() {
    propagateToChildren((c) => c.onGameResize(canvasSize));
  }
  ```

So keeping the onGameResize in the long run then? Because this would be something in FlameGame by default, and not implemented by the user, right?

  Note how this approach is more flexible: a viewport may change in size even if the canvas has not, and HUD components definitely want to be informed of such changes.

I do see the value in it now when you have explained it, but I don't think it should replace onGameResize. Maybe it would also make more sense in the long run to have onGameResize be onViewportResize instead.

spydon avatar Mar 10 '22 08:03 spydon

So keeping the onGameResize in the long run then? Because this would be something in FlameGame by default, and not implemented by the user, right?

My thinking was that it won't be the default. In the vast majority of games you don't need every single component to be aware of canvas resizing. And by "vast majority" I mean that I can't think of any game where this approach would be desirable.

Maybe it would also make more sense in the long run to have onGameResize be onViewportResize instead.

Viewport is the component (at least in the new camera system), and it has a size. Thus, when the viewport changes in size, it would trigger onParentResize for its children.

The only case that onParentResize does not cover is that it's not called before the component's onLoad. However, I don't consider this feature to be particularly useful (since the canvas size can be obtained in onLoad anyways), and as discussed before, it may even sometimes be harmful.

st-pasha avatar Mar 10 '22 18:03 st-pasha

My thinking was that it won't be the default. In the vast majority of games you don't need every single component to be aware of canvas resizing. And by "vast majority" I mean that I can't think of any game where this approach would be desirable.

It is currently used for all games that don't use a viewport and use all of the screen size available and size their components accordingly.

The only case that onParentResize does not cover is that it's not called before the component's onLoad. However, I don't consider this feature to be particularly useful (since the canvas size can be obtained in onLoad anyways), and as discussed before, it may even sometimes be harmful.

It's not the canvas size that you want though, it is the size of the viewport. But maybe that can be accessed in onLoad as well already if you have a gameRef?

spydon avatar Mar 12 '22 11:03 spydon

It's not the canvas size that you want though, it is the size of the viewport.

With the new camera, HUD components are just regular components that are attached to the Viewport. So for them, the "viewport size" is just the size of the parent. And whenever viewport resizes, it will trigger onParentResize for their children -- exactly as we want.

Note also that there could be multiple cameras, and correspondingly multiple viewports. Each viewport will be responsible for its own onParentResize if and when that particular viewport changes in size.

st-pasha avatar Mar 17 '22 16:03 st-pasha