Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fresh new camera #3583

Closed
wants to merge 5 commits into from
Closed

Fresh new camera #3583

wants to merge 5 commits into from

Conversation

mapsam
Copy link
Contributor

@mapsam mapsam commented Nov 9, 2016

This is a complete refactor of the Camera API (js/ui/camera.js) per discussion in #2801 - some major changes:

  • brand new setCamera method that takes three arguments, 1) camera options, 2) animation options, 3) event data.
  • animations are turned OFF by default when using setCamera. Many internal usages of setCamera are updated to include their former animation type.
  • Animation options now include a type parameter that has three valid values: none, ease, and fly. Many of the animation options parameters are specific to certain types of animations.
  • Updated tests for the camera API
  • new getCameraForBounds method that returns a camera options object based on a bounds. We are keeping fitBounds, which is a wrapper around this, and subsequently a wrapper around setCamera.

TODO

  • update examples
  • update other tests that use former camera API methods
  • clean up the JSDOC in the camera API to 💯 reflect valid parameters
  • determine final name for getCameraForBounds method.
  • CHANGELOG updates
  • post benchmark scores
  • manually test the debug page

Questions

  • is a switch statement okay to use in the setCamera method?
  • we've removed the duration: 0 workaround, which means you can't fake your way through the ease animation any longer. This only appears to be a problem with the scroll zoom functionality, which requires ease for smooth transitions. I've worked around this by setting duration: 1. Is this okay?

I'd like to get some approval of the direction of this work before I update all of the examples and other tests, if that's okay.

cc @lucaswoj @mourner @1ec5 @mollymerp @lbud @jfirebaugh

break apart camera & animation options

add getCamera method, replace old methods throughout codebase

start updating tests, cameraForBounds method

panTo tests

writing some animation tests

toying with tests

continue writing tests

fly tests

add bounds tests
@jfirebaugh
Copy link
Contributor

Awesome progress here @mapsam. A few points on specific API details:

  • I suggest changing the type key of AnimationOptions to animation. setCamera(..., {animation: 'ease'}) reads more clearly than setCamera(..., {type: 'ease'}).

  • I liked @1ec5's definitions in Unify camera API into a single method #2801 (comment):

    CameraOptions describes state whereas AnimationOptions describes a transition between two states.

    Under this definition, the around option belongs in the latter category, and should belong to AnimationOptions rather than CameraOptions.

  • In turn, this implies that we should find a more general term for this typedef than AnimationOptions, since around is an option not necessarily associated with animation. MovementOptions? Or perhaps we should rename CameraOptions to Camera and call the second parameter simply "options".

@mourner
Copy link
Member

mourner commented Nov 11, 2016

Under this definition, the around option belongs in AnimationOptions

@jfirebaugh I think it belongs to CameraOptions. When doing an animation, you can either specify target center + offset, or around (they're mutually exclusive and determine the final center).

@mapsam mapsam mentioned this pull request Nov 11, 2016
1 task
@jfirebaugh
Copy link
Contributor

Let's think more about both offset and around then. They have the potential to introduce problematic special cases or failure conditions.

  • What will setCamera do if someone specifies both offset and around?
  • What will setCamera do if someone specifies around + properties other than zoom?
  • Would it be more appropriate to have a method like cameraForZoomAround, in the vein of cameraForBounds?
  • Looking at the current code, around is supported for easeTo, but not jumpTo or flyTo. Does that mean it's only going to be supported for setCamera(..., {type: 'ease'})?
  • Looking at the current code, offset is supported for easeTo and flyTo, but not jumpTo. It's documented as being a member of AnimationOptions, not CameraOptions. Is this still going to be true?

@1ec5
Copy link
Contributor

1ec5 commented Nov 11, 2016

offset and around roughly correspond to padding and anchor in gl-native’s mbgl::CameraOptions struct, respectively. (On the other hand, that struct is internal to the SDKs.) I think it’s reasonable to treat these properties as state (and therefore put them in CameraOptions), because offset affects the frame of reference for center and around affects the point of reference for zoom and angle.

Looking at the current code, around is supported for easeTo, but not jumpTo or flyTo. Does that mean it's only going to be supported for setCamera(..., {type: 'ease'})?
Looking at the current code, offset is supported for easeTo and flyTo, but not jumpTo.

padding and anchor are supported for all three operations in gl-native. If offset and around doesn’t work here, we should find a way to make them work.

@andrewharvey
Copy link
Collaborator

Currently https://www.mapbox.com/mapbox-gl-js/api/#Map#fitBounds supports maxZoom, looking at the latest API docs for this branch I can't see maxZoom mentioned in fitBounds. Does it make sense to add it as a cameraOptions?

@jfirebaugh
Copy link
Contributor

Hey @mapsam -- I'm in the process of cleaning out our PR queue and it looks like there hasn't been any activity on this one recently. If you're still interested in coming back to it, please let us know and we'll be happy to help you get it to completion. Until then, I'm going to close it out. Thanks again for your contributions!

@jfirebaugh jfirebaugh closed this Jun 28, 2017
@jfirebaugh jfirebaugh deleted the fresh-new-camera branch June 28, 2017 19:45
@mapsam
Copy link
Contributor Author

mapsam commented Jun 28, 2017

Thanks for gardening @jfirebaugh! You're right, unfortunately I don't have the capacity to finish it right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GL native → GL JS For feature parity with Mapbox Maps SDK on a native platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants