-
-
Notifications
You must be signed in to change notification settings - Fork 922
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
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 #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?
- Loading branch information
1 parent
df6a228
commit 624b09c
Showing
4 changed files
with
227 additions
and
21 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
166 changes: 158 additions & 8 deletions
166
packages/flame/test/camera/behaviors/viewport_aware_bounds_behavior_test.dart
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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<BoundedPositionBehavior>()!.bounds; | ||
Shape? _getBounds(CameraComponent camera) => | ||
camera.viewfinder.firstChild<BoundedPositionBehavior>()?.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; | ||
} |