modify the transform implementation to allow for concurrent animations #3487

yousifd · 2025-05-19T18:12:45Z

The previous implementation of the transform class overrides the current animation with the new one. This new implmentation changes that to allow for concurrent transformations to occur for different types of transfomrations at the same time. For example, you can have two concurrent animations where one modifies the zoom while the other modifies the position of the camera without overriding the other.

This change is necessary to allow for concurrent camera animations during navigation.

louwers

Allowing concurrent animations is a nice feature, but without any changes to the platform APIs I don't think this implementation is backward compatible? It should be.

Some general notes:

Implementations should be added to .cpp files (unless it's a class/function template implementation), not in headers.
Do not use single-letter variables.

louwers · 2025-05-19T20:40:40Z

src/mbgl/map/transform.hpp

@@ -16,6 +16,30 @@

 namespace mbgl {

+template <class T>


Why are these structs with everything public?

Why are you using a shared pointer? Does that data need to be heap allocated?

Why are these structs with everything public?

They don't have to be public, will move them to be private.

Why are you using a shared pointer? Does that data need to be heap allocated?

It needs to be shared across all PropertyAnimations member variables. We need it so that a latlng and zoom that share the same PropertyAnimation (such as when you update the camera to translate and zoom at the same time) you don't have to keep track of two copies of the same PropertyAnimation and only have to update one when necessary and the other would be updated as well. For example, if an animation has a finish frame callback and that animation updates both the latlng and the zoom values of the camera we only call the finish frame callback once for either of the latlng or zoom PropertyAnimation objects and we wouldn't call it again for the other PropertyAnimation. This is necessary to keep backwards compatibility for animations that span multiple properties.

@louwers I've exposed a flag to enable or disable this feature during navigation for both android and ios. I've also cleaned up large parts of the code and fixed some of the failing tests. Please let me know if you need help understanding the changes or if you think some of the changes should be done differently.

louwers · 2025-05-19T20:41:17Z

src/mbgl/map/transform.hpp

+    bool animationTransitionFrame(std::shared_ptr<PropertyAnimation>&, double);
+    void animationFinishFrame(std::shared_ptr<PropertyAnimation>&);
+
+    void visit_pas(const std::function<void(std::shared_ptr<PropertyAnimation>&)>& f) {


Nit: we tend to use camelCase.

github-actions · 2025-05-19T21:21:18Z

Bloaty Results (iOS) 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +0.1% +13.6Ki  +0.1% +16.0Ki    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results-ios/pr-3487-compared-to-main.txt

yousifd · 2025-06-22T12:37:31Z

@louwers

but without any changes to the platform APIs I don't think this implementation is backward compatible? It should be.

This implementation is backwards compatible. What changes to the platform APIs are you expecting?

The previous implementation of the transform class overrides the current animation with the new one. This new implmentation changes that to allow for concurrent transformations to occur for different types of transfomrations at the same time. For example, you can have two concurrent animations where one modifies the zoom while the other modifies the position of the camera without overriding the other. This change is necessary to allow for concurrent camera animations during navigation.

… transforms

This exposes a new flag to the maplibre ios interface to allow users to choose to have concurrent camera animations during tracking. This makes it so that all calls to `cancelTransforms` from ios are ignored and independent concurrent camera animations are ran concurrently rather than overriding each other.

this makes it so that users of the android integration can choose to have indepedent camera animations run concurrently instead of them waiting on each other or canceling each other.

louwers · 2025-07-01T11:31:34Z

src/mbgl/map/transform.hpp

-                         const AnimationOptions&,
-                         const std::function<void(double)>&,
-                         const Duration&);
+    void startTransition(const CameraOptions&, const Duration&, std::shared_ptr<Animation>&);


Passing a shared pointer by reference does not make sense I think.

Just take a normal reference and dereference the pointer when calling it.

done 0f387f2

louwers · 2025-07-01T11:37:11Z

src/mbgl/map/transform.hpp

+              scaling(scaling_),
+              rotating(rotating_) {}
+
+        double interpolant(TimePoint);


I wasn't sure about the name myself but that's the name of a function that returns the value used for interpolation https://en.wiktionary.org/wiki/interpolant

I think it can be const right?

louwers · 2025-07-01T11:39:33Z

...id/MapLibreAndroid/src/main/java/org/maplibre/android/location/LocationComponentOptions.java

+     *
+     * @return whether concurrent camera animations are enabled or disabled
+     */
+    public LocationComponentOptions.Builder concurrentCameraAnimationsEnabled(Boolean concurrentCameraAnimationsEnabled) {


Enabling concurrent camera animations is not only relevant for the LocationComponent, is it? So it should not be a setting that exists only here.

Currently that's the only use case I can think of. If you want to have multiple animations at the same time from the application side then you could just bundle the animations into a single setCamera call.

If you still think that concurrent camera animations should be universal then I'd also have to change the logic around cancelTransitions to not check for the current camera mode.

If you want to have multiple animations at the same time from the application side then you could just bundle the animations into a single setCamera call.

Why is this not possible in the location component?

What if someone wants to implement their own location component?

I think I understand. Would it make more sense to add a boolean to the java Transform class and expose the concurrent animations flag from the MapView class instead?

louwers · 2025-07-16T16:17:52Z

src/mbgl/map/transform.hpp

 private:
+    template <class T>
+    struct Property {
+        std::shared_ptr<Animation> animation;


Why is this a shared pointer?

The Animation object is used to keep track of the properties current animation. It keeps track whether we've ran the animation's frame callback, animation's finish callback, and the panning, scaling, and rotating state.

So, to avoid updating the above for every property we use a shared pointer to ensure that if any one of the properties finished running the animation's frame or finish callbacks then the other one doesn't do it again.

louwers · 2025-07-16T16:18:04Z

src/mbgl/map/transform.hpp

+    bool animationTransitionFrame(Animation&, double);
+    void animationFinishFrame(Animation&);
+
+    void visitProperties(const std::function<void(Animation&)>& f) {


Please don't use single-letter variables.

I'm not sure if I can come up with a more descrptive name for the function, maybe I can call it func. I followed the same style you already have for visit functions similar to RenderOrchestrator::visitLayerGroups.

louwers · 2025-07-16T16:18:18Z

src/mbgl/map/transform.cpp

@@ -692,4 +806,15 @@ void Transform::setFreeCameraOptions(const FreeCameraOptions& options) {
    state.setFreeCameraOptions(options);
 }

+double Transform::Animation::interpolant(TimePoint now) {
+    bool isAnimated = duration != Duration::zero();


Can use the method you added.

louwers · 2025-07-16T16:24:13Z

src/mbgl/map/transform.cpp

+            .target = endPoint,
+            .set = true,
+            .frameLatLngFunc =
+                [=, this](TimePoint now) {


Please see https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-this-capture

louwers · 2025-07-16T16:27:53Z

src/mbgl/map/transform.cpp

+    if (!activeAnimation) {
+        activeAnimation = true;
+
+        bool panning = false, scaling = false, rotating = false;


Nit: we tend not to use multiple variable declarations.

https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-name-one

louwers · 2025-07-16T16:28:32Z

src/mbgl/map/transform.hpp

+        bool ran = false;      // Did this property animation run this frame
+        bool finished = false; // Did we execute the finish frame for this property animation this frame
+        bool done = false;     // Did this property animation reach the end of the frame
+        bool panning = false, scaling = false, rotating = false;


Please define separately https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-name-one

louwers · 2025-07-16T16:29:11Z

src/mbgl/map/transform.hpp

@@ -132,22 +132,87 @@ class Transform : private util::noncopyable {
    FreeCameraOptions getFreeCameraOptions() const;
    void setFreeCameraOptions(const FreeCameraOptions& options);

+    struct Animation {


Does this need to be a struct? Can it be a class?

It's just a container class of related information. I don't think there is any need to make the properties private by default and add accessors. I can make the whole definition of Animation be private to avoid others using it if that makes the usage clearer?

louwers · 2025-07-16T16:34:39Z

src/mbgl/map/transform.cpp

@@ -103,13 +103,13 @@ void Transform::jumpTo(const CameraOptions& camera) {
 * smooth animation between old and new values. The map will retain the current
 * values for any options not included in `options`.
 */
-void Transform::easeTo(const CameraOptions& inputCamera, const AnimationOptions& animation) {
+void Transform::easeTo(const CameraOptions& inputCamera, const AnimationOptions& options) {


Nit: instead of renaming this to options, perhaps use animationOptions? Goes for several places.

louwers · 2025-07-16T16:36:11Z

src/mbgl/map/transform.hpp

+    struct Animation {
+        TimePoint start;
+        Duration duration;
+        AnimationOptions options;


An Animation has a duration, but an Animation also has an AnimationsOptions object with an optional duration. I think this needs some more refactoring...

So in the flyTo and easeTo functions where we initialize the Animation object there is logic around setting the duration value that doesn't necessarily equal to the duration value in AnimationOptions. Maybe I could change those to override the value in AnimationOptions but that might have unintended side effects.

louwers

Left some more comments. As discussed on Slack, needs test coverage before we can consider merging this.

General comment: please use const for definitions wherever possible.

github-actions · 2025-07-16T18:45:50Z

Bloaty Results 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +0.1%  +114Ki  +0.1% +28.9Ki    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-3487-compared-to-main.txt

Compared to d387090 (legacy)

    FILE SIZE        VM SIZE    
 --------------  -------------- 
   +33% +38.3Mi  +449% +26.8Mi    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-3487-compared-to-legacy.txt

github-actions · 2025-07-16T18:49:10Z

Benchmark Results ⚡

Benchmark                                                     Time             CPU      Time Old      Time New       CPU Old       CPU New
------------------------------------------------------------------------------------------------------------------------------------------
OVERALL_GEOMEAN                                            +0.0194         +0.0192             0             0             0             0

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/benchmark-results/pr-3487-compared-to-main.txt

louwers

I just tried the GLFW app, you can build and run it with:

cmake --preset macos
cmake --build build-macos --target mbgl-glfw
build-macos/platform/glfw/mbgl-glfw --style https://tiles.openfreemap.org/styles/liberty

You can jump between various locations with the 'A' key.

It seems to be broken with this PR.

Before:

before.mov

After:

Screen.Recording.2025-07-21.at.18.54.56.mov

I am skeptical of the approach taken by this PR. Whether concurrent camera animations are enabled or not should be set on the C++ level, not defined at the platform level, definitely not tucked away in an internal component only used by the platform itself. Relying on cancelTransitions being called is brittle and not backward-compatible on all platforms. Also the Transform class having knowledge of the location component is not a good design. In its current form I do not feel comfortable approving this PR.

What I suggest now and for next time: create an issue describing the problem first. Perhaps with a video or two. I still do not 100% understand the problem being addressed here. Especially when modifying fundamental code in the library that has the potential to disrupt our users, it is better to discuss the issue first or even write a design proposal. That way we can together explore the solution space and choose a good approach before code is written.

It took a while for my opinion to crystalize. I apologize that you went though several rounds of reviews and addressing comments. I aim to do better next time and I hope this does not discourage you from contributing.

yousifd · 2025-07-22T10:02:45Z

@louwers I fully understand and I should have started with an issue or some sort of longer form discussion about the problem. I just got a bit too excited and shared this PR hoping we can have this discussion here. I'll post the issue with all the necessary details and the approach I took here and we can go from there.

I fully understand your reluctance to merge this and I expected it. I'll make sure to follow the same process going forward for other significant changes.

louwers requested changes May 19, 2025

View reviewed changes

louwers reviewed May 19, 2025

View reviewed changes

yousifd added 7 commits June 26, 2025 16:05

cleanup code and make things more private

15f5534

handle panning, rotating, and scale state correctly during concurrent…

b6ad42b

… transforms

add toggle for ignored failed tests to results viewer

c220b3d

resolve all test and naming issues with the new transform implementation

003eeae

expose the concurrent camera animations during navigation to android

5caff41

this makes it so that users of the android integration can choose to have indepedent camera animations run concurrently instead of them waiting on each other or canceling each other.

yousifd force-pushed the concurrent_transforms branch from 06e7a0e to 5caff41 Compare July 1, 2025 09:38

yousifd requested a review from louwers July 1, 2025 11:25

louwers reviewed Jul 1, 2025

View reviewed changes

pass animation by reference rather than pointer reference

0f387f2

yousifd requested a review from louwers July 2, 2025 08:38

louwers reviewed Jul 16, 2025

View reviewed changes

louwers requested changes Jul 16, 2025

View reviewed changes

yousifd added 2 commits July 17, 2025 16:55

cleanup and refactor code based on comments and style expectations

9ce818d

add tests and fix code based on these test fixes

70ecf8d

yousifd requested a review from louwers July 17, 2025 14:31

louwers requested changes Jul 21, 2025

View reviewed changes

correctly call the finish callback when a value is set but not used

5730ce7

resolve changing parameters through flyto

f6a66c1

Uh oh!

modify the transform implementation to allow for concurrent animations #3487

Are you sure you want to change the base?

modify the transform implementation to allow for concurrent animations #3487

Uh oh!

Conversation

yousifd commented May 19, 2025

Uh oh!

louwers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

louwers May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented May 19, 2025

Bloaty Results (iOS) 🐋

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results-ios/pr-3487-compared-to-main.txt

Uh oh!

yousifd commented Jun 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yousifd Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yousifd Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

louwers left a comment

Choose a reason for hiding this comment

Uh oh!

louwers May 19, 2025 •

edited

Loading

yousifd Jul 17, 2025 •

edited

Loading

yousifd Jul 17, 2025 •

edited

Loading