-
-
Notifications
You must be signed in to change notification settings - Fork 895
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
fix: ViewportAwareBounds component and lifecycle issues #3276
base: main
Are you sure you want to change the base?
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.
fix:
The ViewportAwareBounds
component and its lifecycle issuesfix:
The ViewportAwareBounds component and lifecycle issues
fix:
The ViewportAwareBounds component and lifecycle issuesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would do this within the setBounds
by listening to the mounted
completer of the BoundedPositionBehavior
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about merging ViewportAwareBoundsBehavior
into BoundedPositionBehavior
and tossing the first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The BoundedPositionBehavior
can be used without the ViewportAwareBoundsBehavior
, it can target anything that is a PositionProvider
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood.
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
duringonMount
. 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 mountingBoundedPositionBehavior
by listening forViewfinder.onChildrenChanged
events. 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
docs
and added dartdoc comments with///
.examples
ordocs
.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 thatCircle
bounds should keep my viewport inside the circle as it is documented to stay inside the desired bounds shape. Therefore, I thinkCircle
case 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?