From 026bf41f020de66ae9adfcdda9209bfbb75cf60c Mon Sep 17 00:00:00 2001 From: TheMaverickProgrammer <91709+TheMaverickProgrammer@users.noreply.github.com> Date: Mon, 16 Sep 2024 15:08:51 -0400 Subject: [PATCH] fix: ViewportAwareBounds component and lifecycle issues (#3276) # 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 https://github.com/flame-engine/flame/pull/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? --- .../viewport_aware_bounds_behavior.dart | 12 +- .../lib/src/camera/camera_component.dart | 57 +++++- packages/flame/lib/src/camera/viewfinder.dart | 13 ++ .../viewport_aware_bounds_behavior_test.dart | 166 +++++++++++++++++- 4 files changed, 227 insertions(+), 21 deletions(-) diff --git a/packages/flame/lib/src/camera/behaviors/viewport_aware_bounds_behavior.dart b/packages/flame/lib/src/camera/behaviors/viewport_aware_bounds_behavior.dart index 55664161341..7cdd693e828 100644 --- a/packages/flame/lib/src/camera/behaviors/viewport_aware_bounds_behavior.dart +++ b/packages/flame/lib/src/camera/behaviors/viewport_aware_bounds_behavior.dart @@ -94,23 +94,27 @@ class ViewportAwareBoundsBehavior extends Component with ParentIsA { ) .y, ); - final halfViewportSize = viewport.size / 2; + + final viewportSize = viewport.virtualSize; if (_boundsShape is Rectangle) { return Rectangle.fromCenter( center: _boundsShape.center, - size: worldSize - halfViewportSize, + size: worldSize - viewportSize, ); } else if (_boundsShape is RoundedRectangle) { - final halfSize = (worldSize - halfViewportSize) / 2; + final halfSize = (worldSize - viewportSize) / 2; return RoundedRectangle.fromPoints( _boundsShape.center - halfSize, _boundsShape.center + halfSize, (_boundsShape as RoundedRectangle).radius, ); } else if (_boundsShape is Circle) { + final diameter = + max(worldSize.x, worldSize.y) - max(viewportSize.x, viewportSize.y); + final radius = diameter / 2; return Circle( _boundsShape.center, - worldSize.x - max(halfViewportSize.x, halfViewportSize.y), + radius, ); } return _boundsShape; diff --git a/packages/flame/lib/src/camera/camera_component.dart b/packages/flame/lib/src/camera/camera_component.dart index 53619a90a06..91e139ab30c 100644 --- a/packages/flame/lib/src/camera/camera_component.dart +++ b/packages/flame/lib/src/camera/camera_component.dart @@ -1,3 +1,5 @@ +import 'dart:async'; + import 'package:flame/components.dart'; import 'package:flame/extensions.dart'; import 'package:flame/src/camera/behaviors/bounded_position_behavior.dart'; @@ -360,31 +362,68 @@ class CameraComponent extends Component { final boundedBehavior = viewfinder.firstChild(); final viewPortAwareBoundsBehavior = viewfinder.firstChild(); + if (bounds == null) { + // When bounds is null, all bounds-related components need to be dropped. boundedBehavior?.removeFromParent(); viewPortAwareBoundsBehavior?.removeFromParent(); return; } + + Future? boundedBehaviorFuture; if (boundedBehavior == null) { + final BoundedPositionBehavior ref; viewfinder.add( - BoundedPositionBehavior(bounds: bounds, priority: 1000), + ref = BoundedPositionBehavior( + bounds: bounds, + priority: 1000, + ), ); + + boundedBehaviorFuture = ref.mounted; } else { boundedBehavior.bounds = bounds; } - if (considerViewport) { - if (viewPortAwareBoundsBehavior == null) { - viewfinder.add( - ViewportAwareBoundsBehavior(boundsShape: bounds), - ); - } else { - viewPortAwareBoundsBehavior.boundsShape = bounds; + + if (!considerViewport) { + // Edge case: remove pre-existing viewport aware components. + viewPortAwareBoundsBehavior?.removeFromParent(); + return; + } + + // Param `considerViewPort` was true and we have a bounds. + // Add a ViewportAwareBoundsBehavior component with + // our desired bounds shape or update the boundsShape if the + // component already exists. + if (viewPortAwareBoundsBehavior == null) { + switch (boundedBehaviorFuture) { + case null: + // This represents the case when BoundedPositionBehavior was mounted + // earlier in another cycle. This allows us to immediately add the + // ViewportAwareBoundsBehavior component which will subsequently adapt + // the camera to the virtual resolution this frame. + _addViewPortAwareBoundsBehavior(bounds); + case _: + // This represents the case when BoundedPositionBehavior was added + // in this exact cycle but did not mount into the tree. + // We must wait for that component to mount first in order for + // ViewportAwareBoundsBehavior to correctly affect the camera. + boundedBehaviorFuture + .whenComplete(() => _addViewPortAwareBoundsBehavior(bounds)); } } else { - viewPortAwareBoundsBehavior?.removeFromParent(); + viewPortAwareBoundsBehavior.boundsShape = bounds; } } + void _addViewPortAwareBoundsBehavior(Shape bounds) { + viewfinder.add( + ViewportAwareBoundsBehavior( + boundsShape: bounds, + ), + ); + } + /// Returns true if this camera is able to see the [component]. /// Will always return false if /// - [world] is null or diff --git a/packages/flame/lib/src/camera/viewfinder.dart b/packages/flame/lib/src/camera/viewfinder.dart index dd2686ab990..8c638e7dc24 100644 --- a/packages/flame/lib/src/camera/viewfinder.dart +++ b/packages/flame/lib/src/camera/viewfinder.dart @@ -2,6 +2,7 @@ import 'dart:math'; import 'package:flame/components.dart'; import 'package:flame/extensions.dart'; +import 'package:flame/src/camera/behaviors/viewport_aware_bounds_behavior.dart'; import 'package:flame/src/effects/provider_interfaces.dart'; import 'package:flame/src/game/transform2d.dart'; import 'package:meta/meta.dart'; @@ -84,6 +85,18 @@ class Viewfinder extends Component onViewportResize(); } + /// The [considerViewport] flag is read-only and cannot be set except through + /// [CameraComponent.setBounds] as an optional parameter. + /// + /// If this value is true, a child component [ViewportAwareBoundsBehavior] + /// exists whose purpose is to keep the viewfinder's visible area in bounds + /// of the viewport w.r.t. the bounds shape. + /// + /// If this value is false, then no child [ViewportAwareBoundsBehavior] + /// will be present. False is the initial value. + bool get considerViewport => + firstChild() != null; + /// Reference to the parent camera. CameraComponent get camera => parent! as CameraComponent; diff --git a/packages/flame/test/camera/behaviors/viewport_aware_bounds_behavior_test.dart b/packages/flame/test/camera/behaviors/viewport_aware_bounds_behavior_test.dart index 0f08a0479ab..fe9fe7bce97 100644 --- a/packages/flame/test/camera/behaviors/viewport_aware_bounds_behavior_test.dart +++ b/packages/flame/test/camera/behaviors/viewport_aware_bounds_behavior_test.dart @@ -1,29 +1,179 @@ +import 'dart:ui'; + import 'package:flame/camera.dart'; import 'package:flame/experimental.dart'; import 'package:flame_test/flame_test.dart'; import 'package:flutter_test/flutter_test.dart'; +import 'package:vector_math/vector_math_64.dart'; void main() { group('ViewportAwareBoundsBehavior', () { - testWithFlameGame('setBounds considering viewport', (game) async { + testWithFlameGame('setBounds wrt Rectangle', (game) async { + final world = World()..addToParent(game); + final camera = CameraComponent.withFixedResolution( + width: 320, + height: 240, + world: world, + )..addToParent(game); + await game.ready(); + final bounds = Rectangle.fromLTRB(0, 0, 640, 480); + + // With considerViewport = false + camera.setBounds(bounds); + game.update(0); + expect( + (_getBounds(camera) as Rectangle?)?.toRect(), + Rectangle.fromLTRB(0, 0, 640, 480).toRect(), + reason: 'Camera bounds at unexpected location', + ); + + expect(camera.viewfinder.considerViewport, false); + + // With considerViewport = true + camera.setBounds(bounds, considerViewport: true); + game.update(0); + expect( + (_getBounds(camera) as Rectangle?)?.toRect(), + Rectangle.fromLTRB(160, 120, 480, 360).toRect(), + reason: 'Camera bounds did not consider viewport', + ); + + expect(camera.viewfinder.considerViewport, true); + }); + + testWithFlameGame('setBounds wrt RoundedRectangle', (game) async { + final world = World()..addToParent(game); + final camera = CameraComponent.withFixedResolution( + width: 320, + height: 240, + world: world, + )..addToParent(game); + await game.ready(); + final bounds = RoundedRectangle.fromLTRBR(0, 0, 640, 480, 32); + + // With considerViewport = false + camera.setBounds(bounds); + game.update(0); + expect( + (_getBounds(camera) as RoundedRectangle?)?.asRRect(), + RoundedRectangle.fromLTRBR(0, 0, 640, 480, 32).asRRect(), + reason: 'Camera bounds at unexpected location', + ); + + expect(camera.viewfinder.considerViewport, false); + + // With considerViewport = true + camera.setBounds(bounds, considerViewport: true); + game.update(0); + // Note that floating point drift occurs, so we account for + // this error threshold epsilon `E`. + const E = 0.03; // +/-3% + final camRRect = (_getBounds(camera) as RoundedRectangle?)?.asRRect(); + final expectedRRect = RoundedRectangle.fromLTRBR( + 163.2, + 126.4, + 476.8, + 353.6, + 32, + ).asRRect(); + expect( + _epsilonRRectEqualityCheck(camRRect!, expectedRRect, E), + true, + reason: 'Camera bounds did not consider viewport', + ); + + expect(camera.viewfinder.considerViewport, true); + }); + + testWithFlameGame('setBounds wrt Circle', (game) async { final world = World()..addToParent(game); - final camera = CameraComponent(world: world)..addToParent(game); + final camera = CameraComponent.withFixedResolution( + width: 320, + height: 240, + world: world, + )..addToParent(game); await game.ready(); - final bounds = Rectangle.fromLTRB(0, 0, 400, 50); + final bounds = Circle(Vector2(320, 240), 320); + // With considerViewport = false camera.setBounds(bounds); game.update(0); - expect((_getBounds(camera) as Rectangle).toRect(), bounds.toRect()); + expect( + (_getBounds(camera) as Circle?)?.center, + Vector2(320, 240), + reason: 'Camera bounds at unexpected location (considerViewport=false)', + ); + expect(camera.viewfinder.considerViewport, false); + + // With considerViewport = true camera.setBounds(bounds, considerViewport: true); game.update(0); expect( - (_getBounds(camera) as Rectangle).toRect(), - Rectangle.fromLTRB(200.0, -100.0, 200.0, 150.0).toRect(), + (_getBounds(camera) as Circle?)?.center, + Vector2(320, 240), + reason: 'Camera bounds at unexpected location (considerViewport=true)', + ); + + // Check bounds after moving away from the center + // while considerViewport = true + camera + ..setBounds(bounds, considerViewport: true) + ..moveBy(Vector2(-320, 0)); + game.update(0); + expect( + (_getBounds(camera) as Circle?)?.center, + Vector2(320, 240), + reason: 'Camera bounds did not consider viewport after move', + ); + + expect(camera.viewfinder.considerViewport, true); + }); + + testWithFlameGame('setBounds explicit null Shape request', (game) async { + final world = World()..addToParent(game); + final camera = CameraComponent.withFixedResolution( + width: 320, + height: 240, + world: world, + )..addToParent(game); + await game.ready(); + final bounds = Circle(Vector2(320, 240), 320); + + camera.setBounds(bounds); + game.update(0); + expect( + _getBounds(camera) as Circle?, + isNotNull, + reason: 'Camera bounds was null but expected a non-null Circle', + ); + + camera.setBounds(null); + game.update(0); + expect( + _getBounds(camera) as Circle?, + isNull, + reason: + 'Camera bounds expected to be null from side-effect of removing it', ); }); }); } -Shape _getBounds(CameraComponent camera) => - camera.viewfinder.firstChild()!.bounds; +Shape? _getBounds(CameraComponent camera) => + camera.viewfinder.firstChild()?.bounds; + +bool _epsilonRRectEqualityCheck(RRect a, RRect b, double epsilon) { + return (a.left - b.left).abs() <= epsilon && + (a.top - b.top).abs() <= epsilon && + (a.right - b.right).abs() <= epsilon && + (a.bottom - b.bottom).abs() <= epsilon && + (a.tlRadiusX - b.tlRadiusX) == 0 && + (a.tlRadiusY - b.tlRadiusY) == 0 && + (a.trRadiusX - b.trRadiusX) == 0 && + (a.trRadiusY - b.trRadiusY) == 0 && + (a.blRadiusX - b.blRadiusX) == 0 && + (a.blRadiusY - b.blRadiusY) == 0 && + (a.brRadiusX - b.brRadiusX) == 0 && + (a.brRadiusY - b.brRadiusY) == 0; +}