fix: ViewportAwareBounds component and lifecycle issues #3276

TheMaverickProgrammer · 2024-08-21T18:47:11Z

Description

The behavior for CameraComponent.setBounds was not behaving correctly. The viewport-aware behavior was not triggering until a window resize event. All supported bound variants were not respecting the fixed resolution viewport, or in some cases, behaving unpredictably.

The issue was with the fact the viewport-aware behavior component depends on the bounded position component to be in its parent, but was returning null during onMount. The viewport math was using the logical size instead of the virtual size. I made some math changes to be accurate.

This PR adds new a getter CameraComponent.considerViewport. ViewportAwareBoundsBehavior is now added as a side effect of mounting BoundedPositionBehavior by waiting for the mounted future. This guarantees that
Flames life cycle is respected and the components behave as expected.

Tests pass on my own local project.

Videos are linked on the discord thread here.

Melos dry run completed successfully.

Checklist

I have followed the [Contributor Guide] when preparing my PR.
I have updated/added tests for ALL new/updated/fixed functionality.
I have updated/added relevant documentation in docs and added dartdoc comments with ///.
I have updated/added relevant examples in examples or docs.

Breaking Change?

Yes, this PR is a breaking change.
No, this PR is not a breaking change.

Migration instructions

No work to be done.

Related Issues

#2769

QUESTIONS FOR THE DEVS

So I'm not satisfied with the fixes for Circle bounds. The bounds uses the maxima of the viewport, and yes, creates a circle that stays within the size of the bounds circle, but I'd expect that Circle bounds should keep my viewport inside the circle as it is documented to stay inside the desired bounds shape. Therefore, I think Circle case should have 4 incident points to fit the viewport. That is to say, the viewport is not the maxima of the newly calculated Circle, but will be fully contained by this new Circle. This way, you'll never see outside of the bounds as it, seems to me anyway, implies. However I do not know if this is intuitive for others and expected behavior. Thoughts?

spydon

I think the current behavior of the circle is fine, the behavior's main function is to restrict the movement of the viewfinder (the clipping can be done elsewhere).

Some tests would be good, so that we don't end up with regressions here later.
Here is the previous (very lacking) test:
https://github.com/flame-engine/flame/blob/main/packages/flame/test/camera/behaviors/viewport_aware_bounds_behavior_test.dart

spydon · 2024-08-21T18:53:55Z

packages/flame/lib/src/camera/viewfinder.dart

+  void onChildrenChanged(Component child, ChildrenChangeType type) {
+    super.onChildrenChanged(child, type);
+
+    if (child is BoundedPositionBehavior) {
+      final viewPortAwareBoundsBehavior =
+          firstChild<ViewportAwareBoundsBehavior>();
+      if (type == ChildrenChangeType.added && camera.considerViewport) {
+        final bounds = viewPortAwareBoundsBehavior?.boundsShape = child.bounds;
+        // Failed to update because component was null.
+        // We must add instead.
+        if (bounds == null) {
+          add(
+            ViewportAwareBoundsBehavior(
+              boundsShape: child.bounds,
+            ),
+          );
+        }
+      } else {
+        viewPortAwareBoundsBehavior?.removeFromParent();
+      }
+    }
+  }


I would do this within the setBounds by listening to the mounted completer of the BoundedPositionBehavior instead.

I did at first but the code looked messy since the bound behavior can also be mounted already and that needed its own code-path to handle. The code was much simpler to read and understand here. Also, ViewportAwareBoundsBehavior strictly depends on the existence of BoundedPositionBehavior so trying to treat them as two separate classes seems incorrect to me. Putting the setup code here illustrates their relationship better.

If this is a final decision, I can revisit it tomorrow and move it back to the camera component setBounds. Lemme know. 👍

I still think that it is better to keep the code clean by not having dependencies in both directions here.
This way is also more expensive, even though it doesn't matter that much in the viewfinder, since it's quite rare to add many components it.

What do you think about merging ViewportAwareBoundsBehavior into BoundedPositionBehavior and tossing the first?

The BoundedPositionBehavior can be used without the ViewportAwareBoundsBehavior, it can target anything that is a PositionProvider.

Understood.

spydon · 2024-09-12T10:43:22Z

@TheMaverickProgrammer any update on this? :)

…sBehavior is now added as a side effect of mounting BoundedPositionBehavior by listening for Viewfinder.onChildrenChanged events. This gaurantees that Flames life cycle is respected and the components behave as expected. ViewportAwareBoundsBehavior._calculateViewportAwareBounds correctly calculates the new bounds by using viewport.virtualSize instead of the logical size. Also, some math was changed to be accurate. Tests pass on local project. Dry run completed successfully.

…nt's setBounds(). Expanded upon and verified unit tests.

TheMaverickProgrammer · 2024-09-14T19:52:00Z

Need to test something I forgot before pushing. I will re-open after verifying the mount operation happens correctly.

spydon

Just one small comment, other than that it looks good

spydon · 2024-09-15T10:06:49Z

packages/flame/lib/src/camera/camera_component.dart

+  /// component is mounted correctly when the tree updates.
+  ///
+  /// The default value is false.
+  bool get considerViewport => _considerViewport;


Maybe this should be clarified in the name that it is related to the viewfinder, since this is in the CameraComponent. Or maybe the variable should live in the viewfinder instead?

Since it relates to a pre-existing convenience for the end-user, b/c of the effect of setBounds(considerViewport: true), adding a comment that this getter is related to the camera's view finder seems reasonable.

This getter didn't exist before though, in the setBounds method it is clear that considerViewport is related to the bounds, meanwhile it isn't in the general CameraComponent.

What do you want to name it? The only thing that comes to mind is some variant of hasBoundsAwareViewport.

Yeah, I can only come up with long names too, so I think it would be more natural to move it to the viewfinder, it's not something that the user should have to access often anyways (if ever).

Super, thanks!

…. Changed description of the circle test cases to be more clear of their intention: the identical values are for two different outcomes.

spydon

Lgtm! Thanks for your contribution

# Description The behavior for `CameraComponent.setBounds` was not behaving correctly. The viewport-aware behavior was not triggering until a window resize event. All supported bound variants were not respecting the fixed resolution viewport, or in some cases, behaving unpredictably. The issue was with the fact the viewport-aware behavior component depends on the bounded position component to be in its parent, but was returning `null` during `onMount`. The viewport math was using the logical size instead of the virtual size. I made some math changes to be accurate. This PR adds new a getter `CameraComponent.considerViewport`. `ViewportAwareBoundsBehavior` is now added as a side effect of mounting `BoundedPositionBehavior` by waiting for the `mounted` future. This guarantees that Flames life cycle is respected and the components behave as expected. Tests pass on my own local project. Videos are linked on the discord thread [here](https://discord.com/channels/509714518008528896/1275814019235709003/1275888732481785925). Melos dry run completed successfully. ## Checklist - [x] I have followed the [Contributor Guide] when preparing my PR. - [x] I have updated/added tests for ALL new/updated/fixed functionality. - [x] I have updated/added relevant documentation in `docs` and added dartdoc comments with `///`. - [ ] I have updated/added relevant examples in `examples` or `docs`. ## Breaking Change? - [ ] Yes, this PR is a breaking change. - [x] No, this PR is not a breaking change. ### Migration instructions No work to be done. ## Related Issues #2769 ## QUESTIONS FOR THE DEVS So I'm not satisfied with the fixes for `Circle` bounds. The bounds uses the maxima of the viewport, and yes, creates a circle that stays within the size of the bounds circle, but I'd expect that `Circle` bounds should keep my viewport _inside_ the circle as it is documented to stay _inside_ the desired bounds shape. Therefore, I think `Circle` case should have 4 incident points to fit the viewport. That is to say, the viewport is not the maxima of the newly calculated `Circle`, but will be fully contained by this new `Circle`. This way, you'll never see outside of the bounds as it, seems to me anyway, implies. However I do not know if this is intuitive for others and expected behavior. Thoughts?

TheMaverickProgrammer changed the title ~~fix: The ViewportAwareBounds component and its lifecycle issues~~ fix: The ViewportAwareBounds component and lifecycle issues Aug 21, 2024

spydon changed the title ~~fix: The ViewportAwareBounds component and lifecycle issues~~ fix: The ViewportAwareBounds component and lifecycle issues Aug 21, 2024

spydon changed the title ~~fix: The ViewportAwareBounds component and lifecycle issues~~ fix: ViewportAwareBounds component and lifecycle issues Aug 21, 2024

spydon reviewed Aug 21, 2024

View reviewed changes

TheMaverickProgrammer added 2 commits September 14, 2024 14:37

reverted changes to viewfinder and moved logic back to camera compone…

41fb37f

…nt's setBounds(). Expanded upon and verified unit tests.

TheMaverickProgrammer force-pushed the bugfix/viewport_wtr_bounds@mav branch from cfa3230 to 41fb37f Compare September 14, 2024 19:47

TheMaverickProgrammer closed this Sep 14, 2024

component life cycle fix reimplemented and tested

21e2b7d

TheMaverickProgrammer reopened this Sep 14, 2024

TheMaverickProgrammer requested a review from spydon September 14, 2024 21:37

spydon reviewed Sep 15, 2024

View reviewed changes

moved CameraComponent.considerViewport -> Viewfinder.considerViewport…

95badad

…. Changed description of the circle test cases to be more clear of their intention: the identical values are for two different outcomes.

spydon approved these changes Sep 16, 2024

View reviewed changes

spydon enabled auto-merge (squash) September 16, 2024 19:05

spydon merged commit 026bf41 into flame-engine:main Sep 16, 2024
8 checks passed

TheMaverickProgrammer deleted the bugfix/viewport_wtr_bounds@mav branch September 16, 2024 19:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: ViewportAwareBounds component and lifecycle issues #3276

fix: ViewportAwareBounds component and lifecycle issues #3276

TheMaverickProgrammer commented Aug 21, 2024 •

edited

Loading

spydon left a comment

spydon Aug 21, 2024 •

edited

Loading

TheMaverickProgrammer Aug 21, 2024

spydon Aug 21, 2024

TheMaverickProgrammer Aug 21, 2024

spydon Aug 21, 2024

TheMaverickProgrammer Aug 21, 2024

spydon commented Sep 12, 2024

TheMaverickProgrammer commented Sep 14, 2024

spydon left a comment

spydon Sep 15, 2024

TheMaverickProgrammer Sep 15, 2024 •

edited

Loading

spydon Sep 15, 2024

TheMaverickProgrammer Sep 15, 2024

spydon Sep 16, 2024

TheMaverickProgrammer Sep 16, 2024

spydon Sep 16, 2024

spydon left a comment

fix: ViewportAwareBounds component and lifecycle issues #3276

fix: ViewportAwareBounds component and lifecycle issues #3276

Conversation

TheMaverickProgrammer commented Aug 21, 2024 • edited Loading

Description

Checklist

Breaking Change?

Migration instructions

Related Issues

QUESTIONS FOR THE DEVS

spydon left a comment

Choose a reason for hiding this comment

spydon Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spydon commented Sep 12, 2024

TheMaverickProgrammer commented Sep 14, 2024

spydon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheMaverickProgrammer Sep 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spydon left a comment

Choose a reason for hiding this comment

TheMaverickProgrammer commented Aug 21, 2024 •

edited

Loading

spydon Aug 21, 2024 •

edited

Loading

TheMaverickProgrammer Sep 15, 2024 •

edited

Loading