Improve camera transitions and related map callbacks

[naftalibeder]

Description

Continues #1833.

• Includes padding when positioning the camera.
• Adds padding options to the example scene.
• Creates a persistent reference to a CameraAnimator that allows for the padding animation to be stopped and started (to prevent overlapping animations that cause a jump). This is because camera.padding does not appear to be directly animatable.
• Reorganizes the value parsing in toUpdateItem(stop:) to make it clearer what object/values are being returned.

Checklist

• I have tested this on a device/simulator for each compatible OS (iOS only)
• I formatted JS and TS files with running yarn lint:fix in the root folder
• I updated the documentation with running yarn generate in the root folder
• I mentioned this change in CHANGELOG.md
• I updated the typings files (index.d.ts)
• I added/ updated a sample (/example)

Video

example.mov

Discussion

1. How is Move different from Flight? There are four transition options but I can only think of three ways for the camera to move (flight, ease, and linear). It seems like perhaps Move should be deprecated?
2. Is there any appetite for using CameraAnimator more heavily, to optionally allow users to animate camera properties individually? Imagine, for instance, having continuous, frequent updates to the camera's lat/lon, while simultaneously performing a slow change to the zoom level. This would require a change in the API, and it would add complexity, but I think it would better represent what Mapbox v10 has to offer, and would also match one of the nice subtleties Apple Maps has during navigation.

[mfazekas]

How is Move different from Flight? There are four transition options but I can only think of three ways for the camera to move (flight, ease, and linear). It seems like perhaps Move should be deprecated?

One possibility is that fly zooms out goes to destination then zooms in, while move probably would not do that. But I might be wrong about that.

Is there any appetite for using CameraAnimator more heavily, to optionally allow users to animate camera properties individually? Imagine, for instance, having continuous, frequent updates to the camera's lat/lon, while simultaneously performing a slow change to the zoom level. This would require a change in the API, and it would add complexity, but I think it would better represent what Mapbox v10 has to offer, and would also match one of the nice subtleties Apple Maps has during navigation.

Yes we can experiment with that, it's ok to have v10 specific features.

[naftalibeder]

If move doesn't zoom out at all, then I'm not sure how it's different from ease?

[naftalibeder]

And just to note, I can't work on this over the weekend, but I did discover a bug that causes changing the padding to make the camera jump around a bit. I need to look into it more, so maybe hold off on merging?

[mfazekas]

If move doesn't zoom out at all, then I'm not sure how it's different from ease?

Looks like from implementation move is mapped to none so no animation there. It's not a great name...

For 'Ease' and Linear we used:
easeCamera, which doesn't changes the zoom (on enter/exit). And for linear we don't use easingInterpolator
https://docs.mapbox.com/android/maps/api/9.7.1/com/mapbox/mapboxsdk/maps/MapboxMap.html#easeCamera-com.mapbox.mapboxsdk.camera.CameraUpdate-

For 'Flight' we're using animateCamera which does complex animation.
https://docs.mapbox.com/android/maps/api/9.7.1/com/mapbox/mapboxsdk/maps/MapboxMap.html#animateCamera-com.mapbox.mapboxsdk.camera.CameraUpdate-int-com.mapbox.mapboxsdk.maps.MapboxMap.CancelableCallback-

And we'd mapped animation modes here:

maps/javascript/components/Camera.js

Lines 605 to 616 in f42cacc

	_getNativeCameraMode(config) {
	switch (config.animationMode) {
	case Camera.Mode.Flight:
	return MapboxGL.CameraModes.Flight;
	case Camera.Mode.Move:
	return MapboxGL.CameraModes.None;
	case Camera.Mode.Linear:
	return MapboxGL.CameraModes.Linear;
	default:
	return MapboxGL.CameraModes.Ease;
	}
	}

So:

1. flyTo => Flight, is complex animation with zoom in/out
2. moveTo => 'None`, so it's an immediate translation without animation (???)
3. linearTo => Linear
   4.* including easeTo => Ease

It would be great to came up with v10 specific API, that gives use flexible way to handle camera animations, and also Viewport api

[dorthwein]

Hi - so I noticed some pretty big differences in RCTMGLCamera.swift between this PR and the main branch.

Heres a simpler change off of the main branch that enables iOS padding. @mfazekas i defer to you on which or both you want to merge in.

It also doesn't have the jerking issue that this PR has.

#1844

[naftalibeder]

@dorthwein Thanks! I made a comment in your PR, but for posterity: the reason for the extra complexity is to allow padding to animate. Just setting it on the camera does change the padding, but instantaneously, not smoothly.

[naftalibeder]

@mfazekas I would love to have a go (in a different PR!) at creating a different API for the camera. That makes me wonder, however, if we should turn main into a dedicated pre-v10 branch, and have a separate v10 branch that will eventually be a breaking change.

Otherwise, I think we're going to water down any changes, or add unnecessary complexity to the API, to keep backward compatibility.

What do you think about v10 being a breaking / non-breaking update?

Moved to #1850.

[naftalibeder]

@mfazekas I think this is pretty good to go for iOS - see the example below.

The bug I mentioned where the camera "jumps around a bit" was actually because the padding and the camera were able to transition with different easing curves, so I made some changes to pick an appropriate curve for each camera transition type.

This also clarifies the animation modes on the native side to flight | ease | linear | none, although I held off on changing the Javascript API until we decide if breaking changes are acceptable (it would mean changing moveTo to none).

example.mov

[naftalibeder]

@mfazekas The important part of the last bunch of commits is updating the payload shape and fixing a bug where the visible bounds were not being correctly returned (this line returned the max bounds available to the camera), but I also took the chance to do some reorganization of RCTMGLMapView.swift.

No regressions or breaking API changes, everything is additive only.

map.mov

[naftalibeder]

I discovered a bug where if you changed padding without changing bounds or centerCoordinate, the JS wouldn't send any positional information, so the resulting camera config would be erroneous.

I believe this was handled on the native side pre-v10 by caching the previous camera state, but I always found that prone to bugs because it created multiple sources of truth (JS and native). In my opinion, it's strongly preferable to require that the Camera.js state be the sole source of truth.

(You'll notice that at the end of the video, because the horizontal padding sums to greater than the screen width, the map zooms out in a way that looks like a bug. It's actually correct. This can be visually fixed by setting minZoomLevel, which is not yet implemented natively for v10.) EDIT: This is now implemented here, and works well.

@mfazekas What is your thought about merging this? It's getting to be a pretty big PR and I wouldn't mind synchronizing with main.

example.mov

[mfazekas]

@naftalibeder this looks good, but unit tests are failing

[mfazekas]

👍

[vcebotari]

setCamera animation is broken, it doesn't animate, just jump to coordinates and zoom

[naftalibeder]

@vcebotari what platform/SDK/version are you on?

[rastapasta]

Aloha @vcebotari and @naftalibeder , ran into same issue with current main - but was easy to solve: manually set setCamera's animationMode to either flyTo, easeTo or linearTo - as learned from the nice type definitions, but not yet from the documentation.

Maybe we should default the animationMode to something else than moveTo - at least in case the user sets an animationDuration.

And let's add it to the docs as well :)

Cheers and thanks for this PR!

[vcebotari]

@vcebotari what platform/SDK/version are you on?

hi, it's iOS, thank's to @rastapasta , that was the problem, adding animationMode solved the issue

[mfazekas]

@naftalibeder this changes default to Move from Ease. I'd like to revert, as this breaks code like:

cameraRef.setCamera({heading: 60, zoomLevel: 13.5, animationDuration: 20000})

That was doing a an animation, now it doesn't do any animation.

[naftalibeder]

Yeah, it’s a little confusing to have an animation mode that means “transition instantly“, but still allow specifying a duration.

What about deprecating (but still keeping for backward compatibility) move, and relying on the animation duration entirely - so there would be fly, ease, and linear, and if you want an instantaneous transition, you just specify animationDuration=0?

That would solve this particular “bug“, and I think make the API less prone to conflicts like this in the future.

[mfazekas]

My question was more about default. I don't think we should change default mode, unless we have strong arguments for that.

You're right that Move/None is redundant with animationDuration=0. I don't feel very strongly about this. I think it's fine to add a warning when someone tries to use non zero animationDuration with Move/None and it's also fine to do what you've suggested, that is to treat any animationDuration=0 as move, etc.

But we should support Move in this version give warning that it will be removed in v11. Then it's fine to remove in v11.