fix: ViewportAwareBounds component and lifecycle issues #3276
Conversation
…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.
TheMaverickProgrammer
changed the title
fix: The ViewportAwareBounds component and its lifecycle issuesfix: The ViewportAwareBounds component and lifecycle issues
spydon
changed the title
fix: The ViewportAwareBounds component and lifecycle issues
spydon
changed the title
There was a problem hiding this comment.
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
| 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(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I would do this within the setBounds by listening to the mounted completer of the BoundedPositionBehavior instead.
There was a problem hiding this comment.
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. 👍
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
What do you think about merging ViewportAwareBoundsBehavior into BoundedPositionBehavior and tossing the first?
There was a problem hiding this comment.
The BoundedPositionBehavior can be used without the ViewportAwareBoundsBehavior, it can target anything that is a PositionProvider.
Description
The behavior for
CameraComponent.setBoundswas 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
nullduringonMount. 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.ViewportAwareBoundsBehavioris now added as a side effect of mountingBoundedPositionBehaviorby listening forViewfinder.onChildrenChangedevents. This guarantees thatFlames 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
docsand added dartdoc comments with///.examplesordocs.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
Circlebounds. 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 thatCirclebounds should keep my viewport inside the circle as it is documented to stay inside the desired bounds shape. Therefore, I thinkCirclecase should have 4 incident points to fit the viewport. That is to say, the viewport is not the maxima of the newly calculatedCircle, but will be fully contained by this newCircle. 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?