[core] - avoid animating with infinite zoom out factor for Transform::FlyTo #8302

tobrun · 2017-03-07T19:19:58Z

This PR fixes the issue if you try to flyTo a camera position that is zoomed out but has the (allmost) the same target LatLng.

closes #8300

mention-bot · 2017-03-07T19:19:59Z

@tobrun, thanks for your PR! By analyzing this pull request, we identified @1ec5, @jfirebaugh and @incanus to be potential reviewers.

tobrun · 2017-03-07T19:23:38Z

src/mbgl/map/transform.cpp

-    // When u₀ = u₁, the optimal path doesn’t require both ascent and descent.
-    bool isClose = std::abs(u1) < 0.000001;
+    // When u₀ = u₁ or zoom out factor = inf, the optimal path doesn’t require both ascent and descent.
+    bool isClose = std::abs(u1) < 0.000001 || isinf(r(0));


Actually now thinking that this shouldn't be added as part of bool isClose but should be added to the if check below..

1ec5 · 2017-03-07T22:32:24Z

src/mbgl/map/transform.cpp

-    // Perform a more or less instantaneous transition if the path is too short.
-    if (isClose && std::abs(w0 - w1) < 0.000001) {
+    // Perform a more or less instantaneous transition if the path is too short or zoom out factor is inf.
+    if ((isClose && std::abs(w0 - w1) < 0.000001) || isinf(r(0))) {


What causes the ascent factor to be infinite? Do we need to check the descent factor too?

Pardon me if miss something obvious, but for me (on ubuntu linux), this code doesn't compile (it does if I use explicitely std::isinf, as is done for abs)

Good catch @mrlem – it does compile on most of the platforms/compilers we support (as seen in the CI builds), but we should use std:: explicitly to be safe. 👍

@1ec5 good call on descent factor, let me try reproducing the reverse interaction.

What causes the ascent factor to be infinite: a division by 0 in the following code

auto r = [=](double i) { /// bᵢ double b = (w1 * w1 - w0 * w0 + (i ? -1 : 1) * rho2 * rho2 * u1 * u1) / (2 * (i ? w1 : w0) * rho2 * u1); return std::log(std::sqrt(b * b + 1) - b); };

Wasn't able to reproducing a infinite descent factor but I don't see an issue adding a check for this either. I added this in eeaefae.

a division by 0 in the following code

So that means either w1 or rho2 or u1 is 0, right? Sorry to keep going here; just want to make sure we aren’t running into something more fundamental.

kkaefer · 2017-03-09T18:53:36Z

src/mbgl/map/transform.cpp

@@ -243,8 +243,8 @@ void Transform::flyTo(const CameraOptions &camera, const AnimationOptions &anima

    // When u₀ = u₁, the optimal path doesn’t require both ascent and descent.
    bool isClose = std::abs(u1) < 0.000001;
-    // Perform a more or less instantaneous transition if the path is too short.
-    if (isClose && std::abs(w0 - w1) < 0.000001) {
+    // Perform a more or less instantaneous transition if the path is too short or zoom in/out factor is infinte


kkaefer · 2017-03-09T19:13:38Z

src/mbgl/map/transform.cpp

-    // Perform a more or less instantaneous transition if the path is too short.
-    if (isClose && std::abs(w0 - w1) < 0.000001) {
+    // Perform a more or less instantaneous transition if the path is too short or zoom in/out factor is infinte
+    if ((isClose && std::abs(w0 - w1) < 0.000001) || std::isinf(r(0)) || std::isinf(r(1))) {


Can you please explain why this is a correct solution? It feels like putting band-aid on the problem without fixing the underlying problem.

Been looking back and forth between the original paper and our implementation. One difference that I noticed is that we added a check for small changes, as these aren't ideal for the algorithm. Instead we are currently performing an easeTo instead. I don't feel that I'm adding a band-aid to the problem, I'm hardening the existing one.

Above is a normal situation of a flyTo implementation but the use-case described in
#8300 is having a origin and target camera position that is almost the same. This isn't fully captured by current bandaid solution. The algorithm seem to generate -infinite zoom out values as a result of log(o) in following code with i=1:

/** rᵢ: Returns the zoom-out factor at one end of the animation. @param i 0 for the ascent or 1 for the descent. */ auto r = [=](double i) { /// bᵢ double b = (w1 * w1 - w0 * w0 + -1 * rho2 * rho2 * u1 * u1) / (2 * (i ? w1 : w0) * rho2 * u1); return std::log(std::sqrt(b * b + 1) - b); };

My proposal is to optimise the bandaid solution as this interaction is not ideal for a flyTo interaction.

kkaefer · 2017-03-16T09:55:56Z

src/mbgl/map/transform.cpp

-    // Perform a more or less instantaneous transition if the path is too short.
-    if (isClose && std::abs(w0 - w1) < 0.000001) {
+    // Perform a more or less instantaneous transition if the path is too short or zoom in/out factor is infinte
+    if ((isClose && std::abs(w0 - w1) < 0.000001) || std::isinf(r(0)) || std::isinf(r(1))) {


Micro-optimization: Instead of computing r(0) and r(1) here, can we split up this conditional return and store the results of r(0) and r(1) into a constant; they are recomputed further down.

There’s already an r0 variable just below.

1ec5 · 2017-03-16T15:36:48Z

I’m able to reproduce the issue in macosapp using the parameters in #8300 (comment), but I’m noticing that r(0) is -12.395270530594363 and r(1) is 120828.22925742983, not infinity (and the transition isn’t “close” either). It may be relevant that a map view frame 683 points wide by 722 points high results in an mbgl::TransformState::minZoom of 0.49585502688717098 rather than 0, due to constraining the map at the world bounds.

tobrun · 2017-03-17T13:34:25Z

@1ec5 the device I was using had a mgbl::TransfrormState::minZooom = 0.2360141919000848.

… a transformation is close by for flyTo

1ec5 · 2017-03-17T16:19:35Z

My point is that the minZoom depends on the aspect ratio of the map view. For a non-full-screen map view, the aspect ratio could be just about anything.

tobrun · 2017-03-24T14:44:42Z

Closing as suggested band aid solution didn't capture all use-cases

tobrun added the Core The cross-platform C++ core, aka mbgl label Mar 7, 2017

tobrun added this to the android-v5.0.0 milestone Mar 7, 2017

tobrun self-assigned this Mar 7, 2017

tobrun requested a review from 1ec5 March 7, 2017 19:19

tobrun commented Mar 7, 2017

View reviewed changes

tobrun mentioned this pull request Mar 7, 2017

fix the animation with two positions too close #8097

Closed

1ec5 reviewed Mar 7, 2017

View reviewed changes

tobrun force-pushed the tvn-blank-map-with-zoom-out branch from eeaefae to 2e739e7 Compare March 8, 2017 17:38

zugaldia requested a review from kkaefer March 9, 2017 18:12

kkaefer reviewed Mar 9, 2017

View reviewed changes

kkaefer suggested changes Mar 16, 2017

View reviewed changes

[core] - take in account infinite zoom out factor when determining if…

7dec2ac

… a transformation is close by for flyTo

tobrun force-pushed the tvn-blank-map-with-zoom-out branch from 2e739e7 to 7dec2ac Compare March 17, 2017 14:12

tobrun mentioned this pull request Mar 17, 2017

Release Android v5.0.0 #8452

Closed

10 tasks

tobrun removed this from the android-v5.0.0 milestone Mar 17, 2017

tobrun closed this Mar 24, 2017

tobrun deleted the tvn-blank-map-with-zoom-out branch April 21, 2017 09:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] - avoid animating with infinite zoom out factor for Transform::FlyTo #8302

[core] - avoid animating with infinite zoom out factor for Transform::FlyTo #8302

tobrun commented Mar 7, 2017

mention-bot commented Mar 7, 2017

tobrun Mar 7, 2017 •

edited

Loading

1ec5 Mar 7, 2017

mrlem Mar 8, 2017 •

edited

Loading

1ec5 Mar 8, 2017 •

edited

Loading

tobrun Mar 8, 2017

tobrun Mar 8, 2017

1ec5 Mar 9, 2017

kkaefer Mar 9, 2017

kkaefer Mar 9, 2017

tobrun Mar 13, 2017

kkaefer Mar 16, 2017

kkaefer Mar 16, 2017

tobrun Mar 16, 2017

1ec5 Mar 16, 2017

1ec5 commented Mar 16, 2017 •

edited

Loading

tobrun commented Mar 17, 2017

1ec5 commented Mar 17, 2017

tobrun commented Mar 24, 2017

[core] - avoid animating with infinite zoom out factor for Transform::FlyTo #8302

[core] - avoid animating with infinite zoom out factor for Transform::FlyTo #8302

Conversation

tobrun commented Mar 7, 2017

mention-bot commented Mar 7, 2017

tobrun Mar 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrlem Mar 8, 2017 • edited Loading

Choose a reason for hiding this comment

1ec5 Mar 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 commented Mar 16, 2017 • edited Loading

tobrun commented Mar 17, 2017

1ec5 commented Mar 17, 2017

tobrun commented Mar 24, 2017

tobrun Mar 7, 2017 •

edited

Loading

mrlem Mar 8, 2017 •

edited

Loading

1ec5 Mar 8, 2017 •

edited

Loading

1ec5 commented Mar 16, 2017 •

edited

Loading