Mapbox-gl-js: 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 https://github.com/mapbox/mapbox-gl-native/issues/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 https://github.com/mapbox/mapbox-gl-js/issues/2801#issuecomment-257687030 and my proposal in https://github.com/mapbox/mapbox-gl-js/issues/2801#issuecomment-257710669 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]

I like the map.setCamera(cameraOptions, animationOptions) proposal. It's clean, easy to document and not verbose to use.

The challenge is determining the best default behavior. Should it animate by default? If so, should it fly or ease? If so, we would also have to change the current default of setBearing/setZoom etc. to animating.

We could also add bounds to cameraOptions to get rid of fitBounds.

[mapsam]

Excellent. I'll get a prototype of map.setCamera(cameraOptions, animationOptions) put together. Thanks for the input @1ec5 and @mourner!

[mapsam]

One more thing, if we are breaking apart cameraOptions and animationOptions, should these be broken apart all the way down into the private methods? Or should we combine the two objects within setCamera (using util.extend) so they can continue to trickle down into functions like _ease, which currently only take an options argument?

[1ec5]

We could also add bounds to cameraOptions to get rid of fitBounds.

Nice. An alternative approach would be a method that returns the best camera fitting a given bounds. That camera could be composed with setCamera() to implement the behavior currently provided by fitBounds(), avoiding the need to muddle up CameraOptions with options like padding and offset.

For what it’s worth, in the iOS and macOS SDKs, we followed MapKit’s lead by treating “visible coordinate bounds” as state that can be get/set just like the camera. That’s probably overkill here, but there’s also a handy -cameraThatFitsCoordinateBounds: method that can be composed with -setVisibleCoordinateBounds:.

[mourner]

@mapsam I think those should be separate for clarity. E.g. _ease only needs AnimationOptions.

[mapsam]

Prototype over at fresh-new-camera has a setCamera and getCamera method, which replaces easeTo, jumpTo, flyTo.

I haven't removed panTo ... but that can be removed as well, since it's just a wrapper around the easing animation.

Gotta write up tests, but things are looking good, and the API feels intuitive.

[lucaswoj]

fresh-new-camera is looking _fresh_.

Do you think we should keep methods that are simple wrappers of setCamera such as setZoom?

    setZoom(zoom, eventData) {
        this.setCamera({zoom: zoom}, {type: 'none'}, eventData);
         return this;
    }

A user can just as easily call the underlying setCamera method themselves:

setCamera({zoom: 5})
setZoom(5)

[mapsam]

With the addition of setCamera and getCamera, this removes the animation-specific methods. There are still camera-specific methods (listed below) that we should discuss removing. I have two conflicting feelings:

1. Remove it all! The idea of a single method setCamera that does it all is really enticing & clean. Working on updating all examples & tests would be a large-but-doable task.
2. This isn't what I'd expect for a mapping API: maybe I'm comparing to what I know (Leaflet) but turning everything into a camera removes the geographic part of this API. Instead of a geo library that changes the camera, we're a camera API that can take in geographic information.

Getters (these get information about the camera state)

• getCenter
• getZoom
• getBearing
• getPitch

Setters (these set camera state, no animation)

• setCenter
• setPitch
• setBearing
• setZoom
• jumpTo

Animators (set the camera state, with animation)

• panBy (ease)
• panTo (ease)
• zoomTo (ease)
• zoomIn (ease)
• zoomOut (ease)
• rotateTo (ease)
• resetNorth (ease)
• snapToNorth (ease)
• easeTo (ease)
• flyTo (fly)
• fitBounds (mixed)

Others

• setCamera [new]
• getCamera [new]
• isEasing
• stop

[mapsam]

@lucaswoj I think we go whole hog and remove all of the wrappers. The way I see it: the goal of this refactor is to clean things up... all of the wrappers make the camera API a bit murky so keeping them goes against that goal. This will enforce a tighter API and get people more acquainted with the camera itself.

[1ec5]

I agree with removing most of the convenience methods. However, some of the animators, like panBy() and snapToNorth(), do a lot more than it appears at first glance. How do you pan a map 100 pixels to the right using setCamera()? It’s possible but unintuitive – it would require a full-fledged example.

[mourner]

@mapsam the list is missing easeTo, flyTo, jumpTo and fitBounds. My opinion on the API:

• I think we should keep all basic setters/getters, because people rely on it a lot and expect these to be present when coming from Leaflet and other mapping APIs.
• Get rid of jumpTo, flyTo, easeTo, panTo, zoomTo, zoomIn, zoomOut, rotateTo in favor of setCamera (non-animated by default and accepting an animations object when necessary).
• Keep panBy because it has non-trivial logic. Also make it non-animated by default and accept animations options.
• Divided on fitBounds, which also has non-trivial logic. On one hand we could add special bounds property to the camera options, but setCamera would have to take a completely different code path for it. Maybe we should keep it along with panBy, also making non-animated by default and accepting animation options.
• Get rid of snapToNorth and resetNorth because this can be a detail of the rotate handler and is not necessary to expose as a camera API.

[jfirebaugh]

👍 @mourner.

I think we should replace fitBounds with cameraForBounds, which can then be composed with setCamera and animation options. cameraForBounds could also accept bearing and pitch options, which would act as overrides for the current map values. So:

• map.setCamera(map.cameraForBounds(bounds)): fit the bounds within the current rotation and pitch
• map.setCamera(map.cameraForBounds(bounds, {bearing: 0})): fit the bounds, forcing north-up

(cameraForBounds can't accept zoom or center options, because those are determined by the bounds.)

[jfirebaugh]

cameraForBounds

Following the consolidation trend, we may actually want to make a more generic cameraFor function which accepts LngLat, LngLatBounds, and GeoJSON objects. Related tickets: #3307, #2112, #2245.

[lucaswoj]

We may be able to create a method similar to cameraForBounds to handle the panBy, and resetNorth cases.

map.setCamera(map.panCameraBy('pixels', camera, {x: 100}))

[mapsam]

@mourner my bad, I was only including the methods remaining after my first pass in the fresh-new-camera branch. You're correct those should be on that list so we can discuss fully.

cameraForBounds

I created a bounds property in the CameraOptions object that setCamera can use and override zoom - just to see what it looks like. I'd be happy with this or with a new method cameraForBounds that returns a similar object.

Proposal

With @mourner & @jfirebaugh's suggestions, the API looks like this:

| method | description | questions |
| --- | --- | --- |
| setCamera [new] | method that takes a CameraOptions and AnimationOptions object, no animation by default | - |
| - setCenter | set the camera center, wrapper around setCamera, no animation by default | - |
| - setPitch | set the camera pitch, wrapper around setCamera, no animation by default | - |
| - setBearing | set the camera bearing, wrapper around setCamera, no animation by default | - |
| - setZoom | set the camera zoom, wrapper around setCamera, no animation by default | - |
| getCamera [new] | combines all get_ methods into one object | - |
| - getCenter | get the camera's focal point | |
| - getZoom | get camera's current zoom | |
| - getBearing | get camera's current bearing | |
| - getPitch | get the camera's current pitch | |
| - getBounds | gets the bounds | move from map.js into camera.js |
| cameraForBounds [new] | returns a CameraOptions object based on a LngLatBoundsLike input, overriding the zoom parameter | is there a more intuitive name here? |
| panBy | wrapper around setCamera, keep since calculating it is unintuitive | - |
| isEasing | not documented and unused internally | this feels out of place ... can we include #3068 in here? |
| stop | stop all transitions | - |

[mapsam]

Could we keep fitBounds but instead call it setBounds to match the setters above?

Per discussion with @mourner setBounds isn't a perfect alternative to getBounds. setBounds, unlike the rest of the set_ methods, can have varying effects due to the bounding box you provide. The word fit is much more suggestive to the actual action. Removing setBounds idea, but going to move getBounds into the camera API.

[jfirebaugh]

We should probably rename or even remove getBounds as well. getBounds suggests a simple accessor that returns an intrinsic property of the camera, when in reality it computes a result -- the smallest axis-aligned bounding box that completely encloses the visible region, I think? -- which is non-obvious when the map is rotated or tilted.

[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? https://github.com/mapbox/mapbox-gl-js/issues/2801#issuecomment-257972897 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?