Fix Map freeze when `Camera.easeTo` is called at any movement event (move, zoom, rotate, pitch) from another `Camera.easeTo` call #3853

ToHold · 2024-03-18T10:05:42Z

Launch Checklist

Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
Briefly describe the changes in this PR.
Link to related issues.
Include before/after visuals or gifs if this PR includes visual changes.
Write tests for all new functionality.
Document any changes to public APIs.
Post benchmark scores.
Add an entry to CHANGELOG.md under the ## main section.

Hi, I'm still working with MapLibre Camera animations and I found a new case that cause a map freeze. Let me explain : when an easeTo animation is call during another easeTo animation's event, it will offset a wrong taskqueue id. It happens because Camera._stop is called from Camera._renderFrameCallback > Camera._onEaseFrame > Camera._fireMoveEvents that will delete Camera._easeFrameId, but also recreate a new one. It causes an unexpected _requestRenderFrame call that offset all taskqueue.
To avoid this case, I decided to create an EaseAnimation interface, that contains it's own callback and canceled property. Canceled property is now checked before _requestRenderFrame, and avoid this offset. Moreover, the fact that each EaseAnimation got their own callbacks ensures that _onEaseFrame, or _onEaseEnd will never be undefined such as it could happen before this PR : #3332

Reproduce : https://codepen.io/ToHold/pen/ZEZBgrq

ToHold · 2024-03-18T10:07:09Z

src/ui/camera.test.ts

@@ -1010,9 +1010,29 @@ describe('#easeTo', () => {
            }
        });

-        camera.easeTo({zoom: 20, bearing: 90, pitch: 60, duration: 500}, {done: true});
+        camera.easeTo({zoom: 20, bearing: 90, pitch: 60, duration: 500});


I also edited old tests that I've wrote to improve their functioning

"done" property should have been added in the second animation's event to ensure that it ended well

codecov-commenter · 2024-03-18T10:26:01Z

Codecov Report

Attention: Patch coverage is 93.13725% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 86.64%. Comparing base (7308da5) to head (ba1d6fc).
Report is 5 commits behind head on main.

Files	Patch %	Lines
src/ui/camera.ts	93.13%	3 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3853      +/-   ##
==========================================
- Coverage   86.91%   86.64%   -0.28%     
==========================================
  Files         241      241              
  Lines       32341    32390      +49     
  Branches     1976     1971       -5     
==========================================
- Hits        28110    28063      -47     
- Misses       3299     3386      +87     
- Partials      932      941       +9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sbachinin · 2024-03-18T11:02:50Z

I wonder what's the "user story" here.
In what situation would you need to call easeTo from another easeTo?

HarelM · 2024-03-18T12:10:35Z

Why not simply use setTimeout? or have the API return a promise so that you can call a new ease to and manage this outside this library?
Generally speaking, I would like to reduce the logic around this instead of complicating it...

ToHold · 2024-03-18T13:11:35Z

I'm working on 3D Map with models, buildings etc, I use Maplibre as Map core. I want to easeTo a 40 pitch when I zoom in uppon level 15, and I want to easeTo a 0 pitch, 0 bearing when I zoom out under 15. I listen for 'zoom' event and when it passes from 14 to 15 or 15 to 16, I call my animation.

The map freeze occurs in my case, when I zoom in / zoom out from NavigationControl because it calls an easeTo, that fires this 'zoom' event.
By the way there is no problem with scroll_zoom handler because this is not using an easeTo.

ToHold · 2024-03-18T13:29:32Z

You can try out here : https://codepen.io/ToHold/pen/oNOBgyg
You just have to try moving by drag drop after clicking on zoom in

ToHold · 2024-03-18T17:05:15Z

@HarelM I don't like to use setTimeout, because I don't like time based logics in algorithms. That's why it would be awesome if we could talk about this fix, if you want me to improve or change what I proposed in this PR.

sbachinin · 2024-03-19T02:26:53Z

Do you expect that you pitch animation will happen concurrently with zoom animation?
If so, then I think you expect too much from maplibre )
It can run only 1 animation at a time.

In your PR you are fixing the freeze, but does it allow to run 2 animations at once? I bet it doesn't (though I didn't try).
To run 2 animations at once and somehow "merge" their results will surely require a lot of new logic in Maplibre.
And we need some very strong reason to add a lot of new logic.

HarelM · 2024-03-19T06:18:58Z

I thought about it more and I think you can use await map.once("moveend") to wait for the movement to end, and then do the next movement, can't you?

ToHold · 2024-03-19T09:31:57Z

@sbachinin it shouldn't allow multiple animation at the same time, I know that it's too complicated to handle this. _easeAnimations is just a container that should store only one animation expect when an animation is created during move events. It's just a way to have its own "_canceled" property that avoid an unexpected RenderFrame (taskid) request. I tagged _easeAnimations as @internal and explained in comments that it's impossible to have 2 animations in a same time. Camera.stop() should everytime cancel all easeAnimations in _easeAnimations. There is just in Camera._stop that we can specified an easeId, but it should be use for internal use and with caution.
To conclude there is no logical changes for animations, just a unhandled freeze fix.

I tried to explain the problem with this diagram, I hope, this will clarify where the problem is.

ToHold · 2024-03-19T09:44:14Z

@HarelM the problem with await map.once("moveend") is that it will only fire after the end of my scrolls : https://codepen.io/ToHold/pen/vYbmdmJ

ToHold · 2024-03-19T09:51:09Z

I know that _easeAnimations as an array can be confusing, we can imagine a _previousEaseAnimation, _currentEaseAnimation. What do you think about it ?

HarelM · 2024-03-19T20:52:25Z

If the problem is with a misalignment, I would prefer to fix that and not try to create something more generic.
I would also consider changing the logic of the zoom in and out buttons to do the zoom and pitch change in the same "go", although that might not fix the pinch zoom on touch devices.

JannikGM · 2024-03-22T13:24:23Z

src/ui/camera.ts

@@ -1159,33 +1194,38 @@ export abstract class Camera extends Evented {
        }
    }

-    _afterEase(eventData?: any, easeId?: string) {
+    _afterEase(eventData?: any, easeId?: string | number) {


easeId can also be a number here, but only a string in interruptingEaseId below.

Personally I think a string makes more sense as it's more descriptive.

JannikGM · 2024-03-22T13:25:20Z

src/ui/camera.ts

+ * Interface that handle all ease animation property, it avoid any stop/add issue during move events
+ */
+export interface EaseAnimation {
+    easeId: string | number;


See https://github.com/maplibre/maplibre-gl-js/pull/3853/files#r1535566400

It's typically just a string (number not allowed), except here and in the other location I commented + onEaseEnd.

JannikGM · 2024-03-22T13:34:25Z

src/ui/camera.ts

+     * @internal
+     * Contains ease animations but it's important to understand that it can't have two or more animations at the same time.
+     * It's just used to handle ease animations stop.
+     * Every active animations should be stopped before runnin a new one.


What does this mean? Does this mean it still can't independently ease rotation, zoom and padding for example?

I independently ran into the easeTo problem this week, but we have a different problem:

We have a folding sidebar and we want to shift the vanishing point (example: https://maplibre.org/maplibre-gl-js/docs/examples/offset-vanishing-point-with-padding/ )

So in our handler where the sidebar gets unfolded, we call easeTo to shift the vanishing point.
However, the user can interrupt this (accidentally) by triggering a different animation (probably also by map interaction, but definitely if our app tries to set the zoom / move the camera center to focus on something which requires the sidebar (flyTo/easeTo), but which might happen sometime during the transition as data is fetched first).
However, that means the vanishing points will be incorrect forever afterwards because the padding transition gets aborted by flyTo/easeTo.

We definitely need the option to control the padding (and have it non-interruptable, by user interaction or another easeTo without padding) and independently control the zoom, rotation, center (which can be interruptable by the user).

It's not a good idea to just chain these animations (as discussed on the main thread), because the padding changes should align with the animation of the sidebar sliding out, but the camera should already start moving (and moving the camera manually during that time should freeze the camera center, but not the padding transition).

Given the complexity, I also wonder if it might be acceptable to remove the animation interface from maplibre entirely.
It could probably live in an external library / plugin which just calls map.setCamera({center, zoom, pitch, bearing}).

I'd recommend to create a discussion / multiple discussions about the subjects that you suggest here and in Slack. They seem to make sense but there's too much stuff here ).

As for the padding, I've recently researched this stuff and found the whole concept of padding very confusing. My conclusion so far is that we can remove padding altogether and use offset option for easeTo/flyTo instead.
Reason why:

First of all, padding creates this permanent offset of vanishing point. This is a maintenance problem for maplibre and also a possible cause of many small bugs. And I can't figure out why this offset was made permanent. Maybe you have an idea why?

Padding in easeTo/flyTo is actually an offset and nothing else. And we already have an offset option so they are basically duplicates, i.e. they solve one problem. (Though offset in itself is a little weird and needs to be fixed).

What does this mean? Does this mean it still can't independently ease rotation, zoom and padding for example?

I independently ran into the easeTo problem this week, but we have a different problem:

We have a folding sidebar and we want to shift the vanishing point (example: https://maplibre.org/maplibre-gl-js/docs/examples/offset-vanishing-point-with-padding/ )

So in our handler where the sidebar gets unfolded, we call easeTo to shift the vanishing point. However, the user can interrupt this (accidentally) by triggering a different animation (probably also by map interaction, but definitely if our app tries to set the zoom / move the camera center to focus on something which requires the sidebar (flyTo/easeTo), but which might happen sometime during the transition as data is fetched first). However, that means the vanishing points will be incorrect forever afterwards because the padding transition gets aborted by flyTo/easeTo.

We definitely need the option to control the padding (and have it non-interruptable, by user interaction or another easeTo without padding) and independently control the zoom, rotation, center (which can be interruptable by the user).

It's not a good idea to just chain these animations (as discussed on the main thread), because the padding changes should align with the animation of the sidebar sliding out, but the camera should already start moving (and moving the camera manually during that time should freeze the camera center, but not the padding transition).

Given the complexity, I also wonder if it might be acceptable to remove the animation interface from maplibre entirely. It could probably live in an external library / plugin which just calls map.setCamera({center, zoom, pitch, bearing}).

It was not intended to independently have ease, rotation, zoom and padding. The goal was to set easeId inside an independent object. Because in movement fired events it can appears that easeId can be overwrite by the end of a stopped ease animation. That's why I decided to store easeId an callback, inside seperated objects. But it's still calling a stop at the begining of an ease.

It can be a first step to achieve this, but it was not intended for.

ToHold · 2024-03-28T14:54:50Z

What's the final word ? Which changes are you expecting ?

HarelM · 2024-03-31T06:06:20Z

Here's my 2 cents:

Create a user story to describe your problem, maybe a better solution exists to solve it, instead of complicating the already too complicated code.
If there's a bug related to task ID mismatch, I would consider fixing just that at this point in time without introducing new functionality. The solution can be throwing an error to prevent the user from doing "illegal" stuff.

Once a user story is defined and we have found no better solution to the problem a short design of the required changes can be made so that we will be able to understand the fix that is going to be introduced.
The current code changes are a bit cryptic to me to review without the above, sorry...

ToHold · 2024-04-16T09:23:34Z

Here is the user story : #4010

HarelM · 2024-05-09T12:04:42Z

Do we want to continue working on this or should this PR be closed?

Théo Murcia and others added 15 commits November 9, 2023 12:36

Fix TypeError: this._onEaseFrame is not a function

1113d46

Update CHANGELOG.md

aeb9d9d

Update CHANGELOG.md

6f5ba92

adding test

29efce8

Better test and improving fix

b57cbf5

restore isEasing

063f447

remove old comment

df11174

Update CHANGELOG.md

6887c7f

improving tests and fix

9081989

Update src/ui/camera.test.ts

cf88360

Merge branch 'main' into main

50c6a3f

Merge branch 'main' of https://github.com/ToHold/maplibre-gl-js

2ef2bca

Adding tests that cause crash freeze

6274e94

Adding EaseAnimation interface

1876b5f

Update CHANGELOG.md

9772661

ToHold commented Mar 18, 2024

View reviewed changes

Théo Murcia added 2 commits March 18, 2024 11:08

Update CHANGELOG.md

76c1b45

Adding comments

ba1d6fc

JannikGM reviewed Mar 22, 2024

View reviewed changes

HarelM added the waiting for code review fixes label Mar 26, 2024

ToHold mentioned this pull request Apr 16, 2024

Map freeze if easeTo during move event #4010

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Map freeze when `Camera.easeTo` is called at any movement event (move, zoom, rotate, pitch) from another `Camera.easeTo` call #3853

Fix Map freeze when `Camera.easeTo` is called at any movement event (move, zoom, rotate, pitch) from another `Camera.easeTo` call #3853

ToHold commented Mar 18, 2024 •

edited

ToHold Mar 18, 2024

ToHold Mar 18, 2024 •

edited

codecov-commenter commented Mar 18, 2024 •

edited

sbachinin commented Mar 18, 2024

HarelM commented Mar 18, 2024

ToHold commented Mar 18, 2024

ToHold commented Mar 18, 2024

ToHold commented Mar 18, 2024

sbachinin commented Mar 19, 2024

HarelM commented Mar 19, 2024

ToHold commented Mar 19, 2024

ToHold commented Mar 19, 2024 •

edited

ToHold commented Mar 19, 2024

HarelM commented Mar 19, 2024

JannikGM Mar 22, 2024

JannikGM Mar 22, 2024 •

edited

JannikGM Mar 22, 2024

sbachinin Mar 24, 2024

ToHold Mar 25, 2024

ToHold Mar 25, 2024

ToHold commented Mar 28, 2024

HarelM commented Mar 31, 2024

ToHold commented Apr 16, 2024

HarelM commented May 9, 2024

Fix Map freeze when Camera.easeTo is called at any movement event (move, zoom, rotate, pitch) from another Camera.easeTo call #3853

Are you sure you want to change the base?

Fix Map freeze when Camera.easeTo is called at any movement event (move, zoom, rotate, pitch) from another Camera.easeTo call #3853

Conversation

ToHold commented Mar 18, 2024 • edited

Launch Checklist

ToHold Mar 18, 2024

Choose a reason for hiding this comment

ToHold Mar 18, 2024 • edited

Choose a reason for hiding this comment

codecov-commenter commented Mar 18, 2024 • edited

Codecov Report

sbachinin commented Mar 18, 2024

HarelM commented Mar 18, 2024

ToHold commented Mar 18, 2024

ToHold commented Mar 18, 2024

ToHold commented Mar 18, 2024

sbachinin commented Mar 19, 2024

HarelM commented Mar 19, 2024

ToHold commented Mar 19, 2024

ToHold commented Mar 19, 2024 • edited

ToHold commented Mar 19, 2024

HarelM commented Mar 19, 2024

JannikGM Mar 22, 2024

Choose a reason for hiding this comment

JannikGM Mar 22, 2024 • edited

Choose a reason for hiding this comment

JannikGM Mar 22, 2024

Choose a reason for hiding this comment

sbachinin Mar 24, 2024

Choose a reason for hiding this comment

ToHold Mar 25, 2024

Choose a reason for hiding this comment

ToHold Mar 25, 2024

Choose a reason for hiding this comment

ToHold commented Mar 28, 2024

HarelM commented Mar 31, 2024

ToHold commented Apr 16, 2024

HarelM commented May 9, 2024

Fix Map freeze when `Camera.easeTo` is called at any movement event (move, zoom, rotate, pitch) from another `Camera.easeTo` call #3853

Fix Map freeze when `Camera.easeTo` is called at any movement event (move, zoom, rotate, pitch) from another `Camera.easeTo` call #3853

ToHold commented Mar 18, 2024 •

edited

ToHold Mar 18, 2024 •

edited

codecov-commenter commented Mar 18, 2024 •

edited

ToHold commented Mar 19, 2024 •

edited

JannikGM Mar 22, 2024 •

edited