Unify camera API into a single method

[lucaswoj]

As part of the v1 effort, we should consider refactoring the camera API

• the names of methods like panBy, panTo, flyTo, jumpTo, easeTo don't make their behaviour obvious (I need to reference the docs regularly)
• there is an opportunity to express the camera API in terms of fewer methods (maybe even a single method 😄)

[1ec5]

👍 on unifying panTo, flyTo, jumpTo, and easeTo for sure: it’d be nice if AnimationOptions were the one-stop shop for specifying how you get to the destination camera, and the default were to have no animation at all.

On the other hand, panBy is quite handy. It isn’t always obvious how to pan by a relative pixel distance, especially when bearing and pitch are involved. Could it be generalized to accept a options object that could vary the zoom level or bearing instead?

[lucaswoj]

Combining this ticket with #2820, "Support overlapping and/or queued Camera animations." See also mapbox/mapbox-gl-native#3625

[1ec5]

The equivalent gl-native ticket is mapbox/mapbox-gl-native#3625.

[mapsam]

Brainstorming:

moveTo

Everything within the current available animation methods involve moving the map based on a set of options (i.e. bearing, speed, zoom, etc.). What if we were to integrate all of them into a single method, called moveTo that includes an additional option animationType, which specifies how to animate the motion.

Example usage:

map.moveTo({
  animationType: 'ease', // 'fly', 'none', 'something-new'
  center: [0, 0],
  // more options
});

This would allow us to set the moveTo animation default (possibly no animation whatsoever, replacing jumpTo). Internally we can update all motion to moveTo and extend their options to include animationType, which would reduce the number of methods used internally.

[mapsam]

An alternative to the above ^

moveTo w/ animation object

The example above lumps movement and animation all into the same options object. We could make animations, currently AnimationOptions a subset of options, so you'd write out your moveTo function like this:

map.moveTo({
  center: [0, 0],
  bearing: 0,
  pitch: 0,
  zoom: 10,
  animation: {
    type: 'ease', // 'fly', 'none', 'etc'
    duration: 1000,
    easing: function(f) { return f; } 
  }
});

I think we can remove the animate: false option, and just default to no animations. Internal usage of moveTo could set animation then.

[1ec5]

I think it would be more intuitive to keep AnimationOptions separate from CameraOptions. CameraOptions describes state whereas AnimationOptions describes a transition between two states. For consistency with the native SDKs, we could add complementary getCamera() and setCamera() methods to Map; getCamera() returns a CameraOptions that you could pass into another Map’s setCamera() along with an AnimationOptions in a separate argument.

I also wonder if we should unify AnimationOptions and the style specification’s Transition type, considering how similar they already are.

[1ec5]

Since #2820 has been folded into this issue, it bears noting that any options left undefined in CameraOptions could continue to animate concurrently. So a scenario like this would stagger two animations that would run to completion, because bearing is undefined in the second call to setCamera():

map.setCamera({ bearing: 180 }, { type: 'ease', duration: 5000 });
setTimeout(() => map.setCamera({ pitch: 60 }, { type: 'ease', duration: 5000 }), 3000);

[mapsam]

Good points @1ec5 - I'm happy to keep them separate. The reaction to combine them is due to the direct connection of options in some camera methods interacting with animations, like flyTo where speed is very much part of the animation but gets set in a single options object. flyTo is pretty muddled:

Options describing the destination and animation of the transition.
Accepts [CameraOptions](#CameraOptions), [AnimationOptions](#AnimationOptions),
and the following additional options...

With two separate objects, setCamera() would look like this:

map.setCamera({center: [0,0]}, {type: 'ease'});

I'm not sure if that's confusing or not, honestly.

[1ec5]

The extra options on flyTo are all related to the transition (AnimationOptions) rather than the final state (CameraOptions). Therefore the only difference between your proposal in #2801 (comment) and my proposal in #2801 (comment) is that the AnimationOptions would go in a separate argument to setCamera() rather than a property in the CameraOptions.

If the two objects really do need to be combined, it would make more sense to invert the options structure: CameraOptions would be assigned to the to property on the AnimationOptions object passed into an animate() method.

In any case, if need be, easeTo can probably be expressed as a special case of flyTo with the right value of curve or maxZoom.

[mourner]

@jfirebaugh it's not obvious, but I'd prefer making the docs clearer rather than removing it altogether. Makes transition from Leaflet/etc. easier.

[1ec5]

With the addition of cameraForBounds (or boundsToCamera), is a setBounds or fitBounds really needed? #2801 (comment) Could we complement it with a more flexible boundsForCamera (or cameraToBounds)?

[mourner]

@1ec5 I would keep map.fitBounds(bounds, options) as a shortcut for map.setCamera(map.cameraForBounds(bounds, 0), options) (small things like this matter for beginners), but not a strong opinion.

[mapsam]

@mourner running into some .bind errors with the navigation controls.

Example: currently when creating the "zoom in" button we pass map.zoomIn.bind(map) as the function to execute. This worked previously since zoomIn was a wrapper around the easeTo function, so we had space to set defaults for the zooming functionality. Now we can pass in our cameraOptions and animationOptions with the .bind function like this:

map.setCamera.bind(map, {zoom: map.getZoom() + 1}, {type: 'ease'})

The problem: the values in .bind (i.e. map.getZoom()) are only calculated once, so the function doesn't recalculate every time you click the button. This means you can only zoom in/out once.

Any suggestions?

[mapsam]

I'm able to get around the above ^ by building in a private functions for NavigationControl that are called like:

this._zoomIn.bind(map)

And are just implementations of setCamera

_zoomIn() { map.setCamera({zoom: map.getZoom() + 1}, {type: 'ease'}); }

[mapsam]

Can we get rid of the duration: 0 workaround now that we are specifying animation types? With no animation specified, setCamera defaults to no animation. We could also build in a check like:

if (duration === 0) animationOptions.type = 'none';

This would allow us to remove the duration !== 0 and animationOptions.animate checks within the ease and fly animation code.

[1ec5]

Can we get rid of the duration: 0 workaround now that we are specifying animation types?

Yes, I think so. Also, per #1473, we should make sure unanimated camera changes are synchronous, in addition to taking no time to complete.

[mapsam]

get rid of the duration: 0

Realizing there are quite a few tests that rely on duration: 0 to get themselves within the ease/fly transforms, which handle more camera options than the "no animation" transform. Example:

map.setCamera({offset: [100, 0]});

This will run through the non-animation code, which doesn't even check for the offset parameter.

[mapsam]

get rid of the duration: 0

More-so, there is a good amount of duration: 0 in scroll animations, where smooth ease is desired over a ton of non-animated transitions. Removing duration zero means a lot of these interactions aren't as smooth as before. See how duration is set to 0 in the scroll zoom if a wheel isn't used.

Two options:

1. Keep all instances of duration: 0 within the ease function
2. Set duration: 1 for quick ease animations, which leads to a similarly smooth experience

[jwangler]

+1

Definitely would like to see merging the concepts of bounds and camera options together into something like a BoundCameraOptions where bounding box takes the place of center and zoom, and center and zoom are then derived when flyTo/jumpTo/easeTo are called with those BoundCameraOptions..

[tuukka]

What's the status of this work? I would need getCamera and cameraForBounds for a map I'm working on, and they could be implemented without breaking backward-compatibility, but is that ok?

[asheemmamoowala]

With v2 we added a free camera with options to get and set the camera directly, and Map#cameraForBounds.

The existing jumpTo and easeTo will be kept for API compatibility.