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.

Sign up for GitHub

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

Rework Camera#easeTo to address problems with pitch #3130

Closed
wants to merge 3 commits into from
Closed

Rework Camera#easeTo to address problems with pitch #3130

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9edd195
Rework Camera#easeTo to address problems with pitch
yeldarby Sep 1, 2016
e19dd98
Remove linear-ease debug page
yeldarby Sep 1, 2016
1ebf24f
Fix comment
yeldarby Sep 14, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 39 additions & 4 deletions js/ui/camera.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ var browser = require('../util/browser');
var LngLat = require('../geo/lng_lat');
var LngLatBounds = require('../geo/lng_lat_bounds');
var Point = require('point-geometry');
var Transform = require('../geo/transform');

/**
* Options common to {@link Map#jumpTo}, {@link Map#easeTo}, and {@link Map#flyTo},
Expand Down Expand Up @@ -408,13 +409,27 @@ util.extend(Camera.prototype, /** @lends Map.prototype */{
startZoom = this.getZoom(),
startBearing = this.getBearing(),
startPitch = this.getPitch(),
startCenter = tr.center,

zoom = 'zoom' in options ? +options.zoom : startZoom,
bearing = 'bearing' in options ? this._normalizeBearing(options.bearing, startBearing) : startBearing,
pitch = 'pitch' in options ? +options.pitch : startPitch,

toLngLat,
toPoint;
toPoint,

offsetLocation,
toLocation,
fromLocation,
endTransform;

// Setup a Transform representing the state of the map at the end of the transition
endTransform = new Transform();
endTransform.center = tr.center;
endTransform.resize(tr.width, tr.height);
endTransform.zoom = zoom;
endTransform.bearing = bearing;
endTransform.pitch = 0; // fixes #3119 by pretending the map is not pitched; use endTransform.pitch = pitch to revert to the old behavior
Copy link
Contributor

@lucaswoj lucaswoj Sep 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's be mindful of how using Transform in this way affects the architecture of GL JS.

This is a new way to use the Transform class. Transform has all sorts of functionality that isn't being used at this call site. Up until now, we have had exactly one Transform per instance of Camera and the two classes have been tightly coupled together. This change further complicates the role of Transform and its coupling to other modules.

Here is a proposal for a quick refactoring Transform into a few different pieces to enable this use case and improve our overall system architecture:

  • use an object that implements the CameraOptions interface in place of Transform
  • factor out all map size state into Camera or Map
  • factor out all other functionality in Transform into separate util functions which take CameraOptions as an argument

EDIT: See comment below 👇

Does this make sense?

ref #2801

Copy link
Contributor

@lucaswoj lucaswoj Sep 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! I have another idea that'll require much less work:

  • factor Trasform#pointLocation into a standalone utility function with the interface
function pointLocation(
    point: Point,
    viewport: CameraOptions & {width: number, height: number}
)


if ('center' in options) {
toLngLat = LngLat.convert(options.center);
Expand All @@ -427,8 +442,6 @@ util.extend(Camera.prototype, /** @lends Map.prototype */{
toLngLat = tr.pointLocation(toPoint);
}

var fromPoint = tr.locationPoint(toLngLat);

if (options.animate === false) options.duration = 0;

this.zooming = (zoom !== startZoom);
Expand All @@ -442,11 +455,31 @@ util.extend(Camera.prototype, /** @lends Map.prototype */{
this.fire('zoomstart', eventData);
}

offsetLocation = endTransform.pointLocation(toPoint);
fromLocation = endTransform.pointLocation(endTransform.centerPoint);

toLocation = LngLat.convert([
toLngLat.lng - (offsetLocation.lng - fromLocation.lng),
toLngLat.lat - (offsetLocation.lat - fromLocation.lat)
]);

clearTimeout(this._onEaseEnd);

this._ease(function (k) {
// When zooming we need to scale our lat/lon interpolation because the distances change over the course of the transition
var k2 = k,
deltaZoom = zoom - startZoom,
totalDistanceExpansion = Math.pow(0.5, deltaZoom) - 1,
currentDelta,
currentDistanceExpansion;

if (this.zooming) {
tr.zoom = interpolate(startZoom, zoom, k);

currentDelta = tr.zoom - startZoom;
currentDistanceExpansion = Math.pow(0.5, currentDelta) - 1;

k2 = currentDistanceExpansion / totalDistanceExpansion;
}

if (this.rotating) {
Expand All @@ -457,7 +490,9 @@ util.extend(Camera.prototype, /** @lends Map.prototype */{
tr.pitch = interpolate(startPitch, pitch, k);
}

tr.setLocationAtPoint(toLngLat, fromPoint.add(toPoint.sub(fromPoint)._mult(k)));
var lng = interpolate(startCenter.lng, toLocation.lng, k2);
var lat = interpolate(startCenter.lat, toLocation.lat, k2);
tr.center = LngLat.convert([lng, lat]);

this.fire('move', eventData);
if (this.zooming) {
Expand Down