feat: Add a method to adapt the camera bounds to the world #2769

Skyost · 2023-09-24T13:33:58Z

Description

This PR adds a method to the CameraComponent class that allows it to set its bounds according to the current world. In fact, this was the behavior of the old camera object.

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.

Related Issues

Closes #2601.

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

Skyost · 2023-09-25T10:14:02Z

@spydon I see, but calculating the worldSize may be a bit more difficult if the shape is not a Rectangle, isn't it ? Also, does strictBounds sounds good to you ?

spydon · 2023-10-02T11:26:14Z

@spydon I see, but calculating the worldSize may be a bit more difficult if the shape is not a Rectangle, isn't it ? Also, does strictBounds sounds good to you ?

Maybe:

/// When [considerViewport] is true none of the viewport can go outside
/// of the bounds, when it is false only the viewfinder anchor is considered.
void setBounds(Shape? bounds. {bool considerViewport = false}}) {

Skyost · 2023-10-03T14:41:40Z

Okay, I've made the changes you've suggested :

if (considerViewport && bounds != null) {
  final halfViewportSize = viewport.size / 2;
  bounds = Rectangle.fromCenter(
    center: bounds.center,
    size: Vector2(bounds.support(Vector2(1, 0)), bounds.support(Vector2(0, 1))) - halfViewportSize,
  );
}

One drawback is that the bounds are converted to a Rectangle. I don't know if it's possible to just "adapt" the given shape to the viewport as Shape is an abstract class.

spydon · 2023-10-03T15:42:29Z

One drawback is that the bounds are converted to a Rectangle. I don't know if it's possible to just "adapt" the given shape to the viewport as Shape is an abstract class.

Much better! Except that it doesn't compile. 😅
I think we should check at least support circular viewports and circular bounds in the future too, but maybe that could be a follow-up.

Skyost · 2023-10-03T17:53:48Z

Yep, it still wip. More commits to follow 🙂

This method allows to support various other viewport shapes.

Skyost · 2023-10-03T21:15:05Z

Alright, so I've added the following method :

Shape? _calculateViewportAwareBounds(Shape originalBounds) {
  final worldSize = Vector2(
    originalBounds.support(Vector2(1, 0)).x,
    originalBounds.support(Vector2(0, 1)).y,
  );
  final halfViewportSize = viewport.size / 2;
  if (originalBounds is Rectangle) {
    return Rectangle.fromCenter(
      center: originalBounds.center,
      size: worldSize - halfViewportSize,
    );
  } else if (originalBounds is RoundedRectangle) {
    final halfSize = (worldSize - halfViewportSize) / 2;
    return RoundedRectangle.fromPoints(
      originalBounds.center - halfSize,
      originalBounds.center + halfSize,
      originalBounds.radius,
    );
  } else if (originalBounds is Circle) {
    return Circle(
      originalBounds.center,
      worldSize.x - max(halfViewportSize.x, halfViewportSize.y),
    );
  }
  return null;
}

Which is being used in setBounds :

if (considerViewport && bounds != null) {
  bounds = _calculateViewportAwareBounds(bounds) ?? bounds;
}

Tell me what you think about it 🙂

spydon · 2023-10-04T11:21:26Z

Tell me what you think about it 🙂

Looks good! I think we need to take the zoom into consideration too though, because this doesn't work when the zoom level is changed right?

Skyost · 2023-10-11T11:31:59Z

Let's do that !

Skyost · 2023-10-11T11:44:34Z

@spydon Just checked the source code and it seems that BoundedPositionBehavior doesn't take any viewfinder.transformation into account. Maybe this belongs to a separate PR ?

spydon · 2023-10-11T12:02:40Z

@spydon Just checked the source code and it seems that BoundedPositionBehavior doesn't take any viewfinder.transformation into account. Maybe this belongs to a separate PR ?

The BoundedPositionBehavior doesn't need to take any viewfinder transformations into account, since it only checks on a position and a bounds. This PR on the other hand that adds bounds aware of the viewport needs to take into consideration when the CameraComponent.visibleWorldRect changes (i.e. when the viewport size changes after a resize or the zoom level of the viewfinder makes a smaller/bigger part of the world visible), otherwise if you for example first play the game in landscape mode and then flip it to portrait the bounds won't be correctly calculated.

…ng to the current viewport.

Skyost · 2023-10-12T08:59:47Z

Alright. Maybe the best approach is to add a new behavior to the viewfinder so that it can update the bounds based on the visible rectangle.

That's what I did in the latest commit. I haven't implemented the "update" part, but can you tell me what you think about it please ?

… visible world rectangle has changed.

spydon · 2023-10-18T10:07:37Z

Alright. Maybe the best approach is to add a new behavior to the viewfinder so that it can update the bounds based on the visible rectangle.

That's what I did in the latest commit. I haven't implemented the "update" part, but can you tell me what you think about it please ?

The idea is good, but the bounds should not have to be recalculated on every tick, but only when the transforms that are affecting it are changing. You can add listeners to those transforms and run the update method when they trigger.

Skyost · 2023-10-18T10:24:30Z

The idea is good, but the bounds should not have to be recalculated on every tick, but only when the transforms that are affecting it are changing. You can add listeners to those transforms and run the update method when they trigger.

Yes, I was thinking about that. The thing is, from what I know, these transforms don't have any method that allow to register a listener. I'll see what's the best approach.

spydon · 2023-10-18T11:39:51Z

Yes, I was thinking about that. The thing is, from what I know, these transforms don't have any method that allow to register a listener. I'll see what's the best approach.

The FixedResolutionViewport has it (called transform), we could just put that field directly on the Viewport and override it in the subclasses. And the viewfinder already has it.

Skyost · 2023-10-18T11:55:49Z

Yes, what I mean is I didn't know that the Transform2D class had a addListener method or something like that. But I just seen that it overrides ChangeNotifier.

… camera bounds.

…to be listened by `ViewportAwareBoundsBehavior`.

Skyost · 2023-10-19T08:39:12Z

Okay, the problem with listening for Transformation2D updates, it that when there is a FollowBehavior, the following line,

owner.position = delta..add(owner.position);

triggers an update at every tick.

spydon · 2023-10-21T17:39:55Z

Okay, the problem with listening for Transformation2D updates, it that when there is a FollowBehavior, the following line,
owner.position = delta..add(owner.position);
triggers an update at every tick.

scale is exposed as a NotifyingVector2 inside of Transform2D so that you can listen to only scale changes, are there more things from the transform that you need?

Skyost · 2023-10-23T08:56:30Z

Oh yes, sure.

spydon

Starting to look good!
Just some tests missing, the easiest would be to do golden tests for this I think, you can get some inspiration from here for example:
https://github.com/flame-engine/flame/blob/main/packages/flame/test/camera/viewports/fixed_aspect_ratio_viewport_test.dart

packages/flame/lib/src/camera/behaviors/viewport_aware_bounds_behavior.dart

spydon · 2023-11-03T21:58:22Z

Any updates on this @Skyost? :)

Skyost · 2023-11-06T09:33:57Z

Hey @spydon,

Sorry, I've never written any golden test in my life and have been unactive for the past two weeks. I'll try to make some progress today 🙂

Skyost · 2023-11-06T10:28:43Z

I've added a test that checks whether the viewport has been correctly updated. It's not a golden test, but I think it does the trick. Do you think it needs more tests ? If so, what parts of the ViewportAwareBoundsBehavior class need to be tested ?

spydon

Lgtm, the test doesn't really ensure things are rendered correctly, but since the other camera behaviour doesn't have a goldens test either I think we can let this pass for now.

packages/flame/test/camera/behaviors/viewport_aware_bounds_behavior_test.dart

Co-authored-by: Lukas Klingsbo <[email protected]>

# 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?

feat: Add a method to adapt the camera bounds to the world.

40a115d

Skyost changed the title ~~feat: Add a method to adapt the camera bounds to the world.~~ feat: Add a method to adapt the camera bounds to the world Sep 24, 2023

spydon reviewed Sep 24, 2023

View reviewed changes

packages/flame/lib/src/camera/camera_component.dart Outdated Show resolved Hide resolved

Removed adaptBoundsToWorld in favor of a boolean in setBounds.

19e8610

Added _calculateViewportAwareBounds.

0f04f02

This method allows to support various other viewport shapes.

Skyost added 2 commits October 3, 2023 23:17

Fixed a warning triggered by a parameter reassignment.

5140209

Merge branch 'main' into main

2693e2a

Merge branch 'main' into main

eb45480

Fixed imports.

85cecaa

Skyost added 3 commits October 12, 2023 10:22

Merge branch 'main' into main

e357e48

Simplified the setBounds method.

367cb74

Initial attempt of a component that updates the camera bounds accordi…

54513b2

…ng to the current viewport.

Skyost added 2 commits October 16, 2023 09:55

Merge branch 'main' into main

e5f3e0e

Now updating the bounds of the ViewportAwareBoundsBehavior when the…

b4af2be

… visible world rectangle has changed.

Merge branch 'main' into main

ba31084

Skyost added 4 commits October 18, 2023 13:58

Now listening to viewport transformations in order to recalculate the…

3f5329a

… camera bounds.

The Viewport class now holds the transform field which allows it …

d24b7ec

…to be listened by `ViewportAwareBoundsBehavior`.

Removed an unused import.

276a0ea

Merge branch 'main' into main

e144bae

Skyost added 2 commits October 19, 2023 11:22

Added various optimizations.

dcf554f

Fixed a lint warning.

d2a943d

Skyost requested a review from spydon October 19, 2023 09:40

Only listening to transform.scale instead of transform.

5f3621f

spydon reviewed Oct 23, 2023

View reviewed changes

packages/flame/lib/src/camera/behaviors/viewport_aware_bounds_behavior.dart Outdated Show resolved Hide resolved

packages/flame/lib/src/camera/behaviors/viewport_aware_bounds_behavior.dart Outdated Show resolved Hide resolved

Skyost added 2 commits October 24, 2023 10:49

Made various changes (see /pull/2769).

a41965c

Preparing to add some tests.

56035cc

Added a test for ViewportAwareBoundsBehavior.

a0e63df

Skyost added 2 commits November 6, 2023 11:28

Merge branch 'main' into main

9d01ba2

Ran dart format.

842c4e2

spydon approved these changes Nov 7, 2023

View reviewed changes

packages/flame/test/camera/behaviors/viewport_aware_bounds_behavior_test.dart Outdated Show resolved Hide resolved

Skyost and others added 4 commits November 8, 2023 09:44

Turned getBounds into _getBounds.

3c98983

Co-authored-by: Lukas Klingsbo <[email protected]>

Merge branch 'main' into main

2754533

Renamed the getBounds method everywhere else.

b506260

Renamed the bounds into boundsShape and added more docs.

4e6c9b2

spydon merged commit 87b69df into flame-engine:main Nov 8, 2023
8 checks passed

TheMaverickProgrammer mentioned this pull request Aug 21, 2024

fix: ViewportAwareBounds component and lifecycle issues #3276

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add a method to adapt the camera bounds to the world #2769

feat: Add a method to adapt the camera bounds to the world #2769

Skyost commented Sep 24, 2023

Skyost commented Sep 25, 2023

spydon commented Oct 2, 2023

Skyost commented Oct 3, 2023 •

edited

Loading

spydon commented Oct 3, 2023

Skyost commented Oct 3, 2023

Skyost commented Oct 3, 2023

spydon commented Oct 4, 2023

Skyost commented Oct 11, 2023

Skyost commented Oct 11, 2023

spydon commented Oct 11, 2023

Skyost commented Oct 12, 2023 •

edited

Loading

spydon commented Oct 18, 2023

Skyost commented Oct 18, 2023

spydon commented Oct 18, 2023

Skyost commented Oct 18, 2023 •

edited

Loading

Skyost commented Oct 19, 2023

spydon commented Oct 21, 2023

Skyost commented Oct 23, 2023

spydon left a comment

spydon commented Nov 3, 2023

Skyost commented Nov 6, 2023

Skyost commented Nov 6, 2023

spydon left a comment

feat: Add a method to adapt the camera bounds to the world #2769

feat: Add a method to adapt the camera bounds to the world #2769

Conversation

Skyost commented Sep 24, 2023

Description

Checklist

Breaking Change?

Related Issues

Skyost commented Sep 25, 2023

spydon commented Oct 2, 2023

Skyost commented Oct 3, 2023 • edited Loading

spydon commented Oct 3, 2023

Skyost commented Oct 3, 2023

Skyost commented Oct 3, 2023

spydon commented Oct 4, 2023

Skyost commented Oct 11, 2023

Skyost commented Oct 11, 2023

spydon commented Oct 11, 2023

Skyost commented Oct 12, 2023 • edited Loading

spydon commented Oct 18, 2023

Skyost commented Oct 18, 2023

spydon commented Oct 18, 2023

Skyost commented Oct 18, 2023 • edited Loading

Skyost commented Oct 19, 2023

spydon commented Oct 21, 2023

Skyost commented Oct 23, 2023

spydon left a comment

Choose a reason for hiding this comment

spydon commented Nov 3, 2023

Skyost commented Nov 6, 2023

Skyost commented Nov 6, 2023

spydon left a comment

Choose a reason for hiding this comment

Skyost commented Oct 3, 2023 •

edited

Loading

Skyost commented Oct 12, 2023 •

edited

Loading

Skyost commented Oct 18, 2023 •

edited

Loading