Rework Camera#easeTo and Transform#pointLocation #4320

joswinter · 2017-02-22T20:47:11Z

The changes in this pull request fix #4178 and #3112.

Previously when using the Camera#easeTo function the following can happen where the camera only eases to the north. The path of the camera is illustrated in the left image where the camera moves across both poles when easing between two coordinates A and B, this is unwanted behavior. The easeTo should ease in the direction of the shortest path to the endpoint, The correct behaviour is illustrated in the right image. The incorrect behaviour can be observed in the following gist: https://bl.ocks.org/joswinter/f2e9bc8413ed00875ac06eca35d7b83d

These bugs were already fixed by @yeldarby in pull request #3130 which hasn't been merged. I continued working on the code in that pull request and reworked the structure of Camera#easeTo and Transform#pointLocation as commented by @lucaswoj. The Transform#pointLocation now has an optional viewport argument which is used to calculate the pointLocation for the given point. I also changed the Camera#easeTo implementation so that all tests in camera.test.js pass.

Continues on changes in mapbox#3130 pull request

…7/mapbox-gl-js into fix-easeTo-direction

lucaswoj

Thanks for the PR and sorry for the long hold time for a review. We've been very busy the last few weeks here @ Mapbox!

lucaswoj · 2017-03-14T18:57:24Z

src/geo/transform.js

+            if ('center' in v) this.center = v.center;
+            if ('zoom' in v) this.zoom = v.zoom;
+            if ('bearing' in v) this.bearing = v.bearing;
+            if ('pitch' in v) this.pitch = v.pitch;


This is a step in the right direction. I'm still uncomfortable with this stateful & implicit setting & resetting of parameters on Transform. Passing arguments this way is surprising and creates potential for bugs & unintended side effects. coordinateLocation and coordinatePoint worked with this viewport object directly rather than reading from Transform.

I get what you mean by that, I looked into it. I could add an extra argument to the coordinateLocation and pointCoordinate methods but the reason why I used this implicit setting of parameters is to make sure that the pixelMatrix is calculated for the viewport. What do you suggest that I should do concerning the _calcMatrices function, should I rewrite it to allow a viewport parameter? _calcMatrices uses the following fields:

this.x

this.y

this._fov

this._pitch

this.width

this.height

this.worldSize

If I'm going to do it with a viewport parameter I'm also a bit afraid to introduce a lot of duplicate code because every parameter has a seperate check and a lot of other Transform fields are dependent on the ones used in the viewport such as this.worldSize which is dependent on this.scale which is dependent on this.zoom.

Ideally, we would work together to find a way to add a viewport parameter to _calcMatrices, reduce the amount of input needed by _calcMatricies where possible, and refactor to eliminate any duplicate code. Does that sound doable based on your experience in this part of the codebase?

lucaswoj · 2017-03-14T18:59:09Z

src/ui/camera.js

-            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]);


Is it possible to write test for this new functionality? Regression tests for #3112 & #4178?

…seTo-direction

joswinter · 2017-03-17T09:52:22Z

I created a regression test for the Camera#easeTo function. It seems to be working but it is not 100% exact. In the test, I check whether the camera is easing in the right direction in the case of this example.

I have added a _calcMatricesViewport and a _constrainViewport method which helps in calculating the inversePixelMatrix for a viewport @lucaswoj. However, there are two tests that keep failing:

the zooms with specified offset test.
the zooms with specified offset relative to viewport on a rotated camera test.

The functionality should be the same as it was in commit aaede, do the fields in transform.js are influenced by something else which differ in passing the viewport or in setting all transform fields manually from the viewport? The following code was changed in Transform but the output of the pointLocation method shouldn't change.

joswinter · 2017-03-18T17:51:39Z

@lucaswoj I got the tests working now. I want to start refactoring now but I still have some questions. The first two things that should happen is that:

_constrain and _constrainViewport need to be merged. How should I merge these two because at the moment one of them returns something and the other sets the field of the class directly?
_calcMatrices and _calcMatricesViewport need to be merged. The same holds here, but I have some concerns with the projMatrix field that is being set in the middle of the _calcMatrices function.

Do you have any suggestion how I should merge these functions? For most of the functions in the Transform class I already added a viewport parameter and a line at the start with if (viewport === undefined) viewport = this.getViewport();, do you think that is the correct way to go?

edit: I refactored and merged the two functions.

lucaswoj

Thank you so much for following through on the requested architectural changes. 🎉 😄

I have a few points of in-the-weeds feedback.

@jfirebaugh @mourner would you care to take a look at the algorithm changes?

lucaswoj · 2017-03-20T20:14:38Z

src/geo/transform.js

-            this.xLng(zoomedCoord.column * this.tileSize),
-            this.yLat(zoomedCoord.row * this.tileSize));
+            x,
+            y);


Minor nit: can we make this all one line?

lucaswoj · 2017-03-20T20:16:47Z

src/geo/transform.js

@@ -283,8 +314,10 @@ class Transform {
        const coord0 = [p.x, p.y, 0, 1];
        const coord1 = [p.x, p.y, 1, 1];

-        vec4.transformMat4(coord0, coord0, this.pixelMatrixInverse);
-        vec4.transformMat4(coord1, coord1, this.pixelMatrixInverse);
+        const pixelMatrixInverse = this._calcMatrices(viewport);


This operation is expensive enough that we will need to cache pixelMatrixInverse for at least the global viewport.

lucaswoj · 2017-03-20T20:18:42Z

src/geo/transform.js

-            sx = maxX - minX < size.x ? size.x / (maxX - minX) : 0;
+            minX = (180 + this.lngRange[0]) * worldSize / 360;
+            maxX = (180 + this.lngRange[1]) * worldSize / 360;
+            sx = maxX - minX < this.size.x ? this.size.x / (maxX - minX) : 0;


Can you explain what changed & why in this algorithm?

mourner · 2017-03-20T21:37:54Z

Happy to see a PR that fixes the issues, but surprised that it requires so many complicated changes, many of them introducing new expensive calculations. I'll need to spend some time on the problem to see if there's possibly a more elegant approach we're not yet seeing.

lucaswoj · 2017-03-27T19:49:46Z

@mourner Should we delay merging this PR until you've had a chance to prototype a more elegant implementation?

mourner · 2017-03-29T10:46:09Z

@lucaswoj yes, let's delay for a few days! We'll have a hard time maintaining the code merged as is.

mourner · 2017-03-29T17:04:50Z

I'm moving with an alternative PR, submitting soon. Basically the root problem here is not pointLocation implementation, it's that we use it for easeTo interpolations in the first place. Moving the approach closer to how flyTo works fixes the problem with simpler code changes, so I'll close the PR for now, but it'll be there in case we want to revisit making pointLocation more flexible. Thanks for working on this!

joswinter added 11 commits February 17, 2017 14:19

Reworked easeTo to fix wrong direction bug

69b0824

Continues on changes in mapbox#3130 pull request

Removed additional Transform usage in easeTo

d1d010f

Added offset to toLocation in easeTo

6005fb4

Recreated Transform#pointLocation to use a CameraOptions parameter

49d0a42

Reworked easeTo to fix wrong direction bug

0235f77

Continues on changes in mapbox#3130 pull request

Removed additional Transform usage in easeTo

cbc8b72

Added offset to toLocation in easeTo

11654ad

Recreated Transform#pointLocation to use a CameraOptions parameter

e8b4019

Merge branch 'fix-easeTo-direction' of https://github.com/delftswa201…

8de90e3

…7/mapbox-gl-js into fix-easeTo-direction

Removed vertical.html debug page

274832a

Refactored Transform#pointLocation function

fa7c44b

lucaswoj reviewed Mar 14, 2017

View reviewed changes

joswinter added 4 commits March 16, 2017 10:42

Added separate functions for viewport setting and getting

27550b1

Merge remote-tracking branch 'refs/remotes/mapbox/master' into fix-ea…

54b482c

…seTo-direction

Fixed some incorrect code style formatting

b4be075

Added easeTo regression test

aaede12

lucaswoj added the under development label Mar 16, 2017

joswinter added 11 commits March 17, 2017 09:43

Renamed easeTo regression test

3a4d2b3

Added _calcMatricesViewport method

fbfb938

Added viewport argument to coordinateLocation

1205022

Added viewport argument to pointCoordinate

d5465d7

Added viewport bounding

e46cc70

Inverted this fields to start of method

e97390e

Use viewport in _calcMatricesViewport method

6b78db8

Added _constrainViewport method to transform

56d515c

Inverted this fields to start of constrainViewport method

7c067f6

Removed boundViewport method

33e1044

Use viewport in _constrainViewport method

8bead14

lucaswoj removed the under development label Mar 17, 2017

joswinter added 3 commits March 18, 2017 16:25

Fixed incorrect transform#pointLocation behaviour

24a437e

Fixed incorrect code style formatting

2d457f0

Fixed incorrect zoom usage in pointCoordinate

9aed0c9

joswinter added 6 commits March 19, 2017 10:39

Removed setViewport method

4488031

Merged _calcMatrices functions

9c75664

Added missing field set in merged calcMatrices

8a4305c

Merged _constrainViewport and _constrain

31358c3

Moved check to correct position to avoid infinite loops

f923a18

Fixed incorrect zoom and center assignment in _constrain

8fa4a4b

lucaswoj reviewed Mar 20, 2017

View reviewed changes

joswinter added 2 commits March 20, 2017 22:40

Added caching of pixelMatrixInverse

469b067

Fixed wrong variable references

9526cca

mourner closed this Mar 29, 2017

mourner mentioned this pull request Apr 4, 2017

Fix easeTo interpolation on pitched maps #4540

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework Camera#easeTo and Transform#pointLocation #4320

Rework Camera#easeTo and Transform#pointLocation #4320

joswinter commented Feb 22, 2017 •

edited

Loading

lucaswoj left a comment

lucaswoj Mar 14, 2017

joswinter Mar 16, 2017

lucaswoj Mar 16, 2017

lucaswoj Mar 14, 2017

joswinter commented Mar 17, 2017 •

edited

Loading

joswinter commented Mar 18, 2017 •

edited

Loading

lucaswoj left a comment

lucaswoj Mar 20, 2017

lucaswoj Mar 20, 2017

lucaswoj Mar 20, 2017

mourner commented Mar 20, 2017

lucaswoj commented Mar 27, 2017

mourner commented Mar 29, 2017

mourner commented Mar 29, 2017 •

edited

Loading

Rework Camera#easeTo and Transform#pointLocation #4320

Rework Camera#easeTo and Transform#pointLocation #4320

Conversation

joswinter commented Feb 22, 2017 • edited Loading

lucaswoj left a comment

Choose a reason for hiding this comment

lucaswoj Mar 14, 2017

Choose a reason for hiding this comment

joswinter Mar 16, 2017

Choose a reason for hiding this comment

lucaswoj Mar 16, 2017

Choose a reason for hiding this comment

lucaswoj Mar 14, 2017

Choose a reason for hiding this comment

joswinter commented Mar 17, 2017 • edited Loading

joswinter commented Mar 18, 2017 • edited Loading

lucaswoj left a comment

Choose a reason for hiding this comment

lucaswoj Mar 20, 2017

Choose a reason for hiding this comment

lucaswoj Mar 20, 2017

Choose a reason for hiding this comment

lucaswoj Mar 20, 2017

Choose a reason for hiding this comment

mourner commented Mar 20, 2017

lucaswoj commented Mar 27, 2017

mourner commented Mar 29, 2017

mourner commented Mar 29, 2017 • edited Loading

joswinter commented Feb 22, 2017 •

edited

Loading

joswinter commented Mar 17, 2017 •

edited

Loading

joswinter commented Mar 18, 2017 •

edited

Loading

mourner commented Mar 29, 2017 •

edited

Loading