Fix viewer.flyTo ignoring and resetting minimumZoomDistance #5668

cguldner · 2017-07-23T22:08:03Z

This fixes #5573.
I might have messed something up by doing this, but here goes my best shot!

For some reason, the screenSpaceCameraController.minimumZoomDistance was being set to the radius of the bounding sphere, which happened to be always zero, when zooming to an entity or billboard. As the maximumZoomDistance is never set in this way, and this was the commit message when that block of code was added. So I figured that wasn't particularly necessary anymore.

If nothing else, you could probably keep the tests!

…es haven't been set

hpinkos · 2017-07-24T13:10:55Z

Thanks @burn123! @ggetz can you review?

ggetz · 2017-07-24T13:38:46Z

CHANGES.md

@@ -23,6 +23,7 @@ Change Log
 * Fixed crash when using the `Cesium3DTilesInspectorViewModel` and removing a tileset [#5607](https://github.com/AnalyticalGraphicsInc/cesium/issues/5607)
 * Fixed polygon outline in Polygon Sandcastle demo [#5642](https://github.com/AnalyticalGraphicsInc/cesium/issues/5642)
 * Fixed label positioning when using `HeightReference.CLAMP_TO_GROUND` and no position [#5648](https://github.com/AnalyticalGraphicsInc/cesium/pull/5648)
+* Fixed `viewer.flyTo` not respecting zoom limits, and resetting minimumZoomDistance if the camera zoomed past the minimumZoomDistance. [5573](https://github.com/AnalyticalGraphicsInc/cesium/issues/5573)


viewer -> Viewer

ggetz · 2017-07-24T13:42:15Z

Specs/Scene/CameraSpec.js

@@ -2605,6 +2605,36 @@ defineSuite([
        expect(distance).toBeLessThan(sphere.radius * 3.0);
    });

+    it('flyToBoundingSphere does not zoom closer than minimumZoomDistance', function() {
+        scene.mode = SceneMode.SCENE3D;
+        var MIN_VALUE = 1000;


Again, use camel case

ggetz · 2017-07-24T13:52:48Z

Source/Scene/Camera.js

    function adjustBoundingSphereOffset(camera, boundingSphere, offset) {
        if (!defined(offset)) {
            offset = HeadingPitchRange.clone(Camera.DEFAULT_OFFSET);
        }

+        var DEFAULT_ZOOM = 100.0;
+        var MINIMUM_ZOOM = camera._scene.screenSpaceCameraController.minimumZoomDistance;
+        var MAXIMUM_ZOOM = camera._scene.screenSpaceCameraController.maximumZoomDistance;


As per our Coding Guide, these should be camel case, see https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Documentation/Contributors/CodingGuide/README.md#naming

ggetz · 2017-07-24T14:06:26Z

Source/Scene/Camera.js

        var range = offset.range;
        if (!defined(range) || range === 0.0) {
            var radius = boundingSphere.radius;
-            if (radius === 0.0) {
+            // If the minimumZoomDistance hasn't been set yet, it is equal to 1,
+            if (radius < MINIMUM_ZOOM && MINIMUM_ZOOM > 1) {


I'm a little confused why we can't default to the minimum zoom always.

Also see CesiumMath.clamp, that might simplify this if/else block a little.

I was taking the approach that if the radius is 0, it would go to the default zoom unless the minimum zoom had been set already. If you zoom all the way to 1, it doesn't look very good, just a bunch of grey, because it is too close to the surface.

Ok I see. However checking if it's < 1 doesn't guarantee the minimumZoom hasn't been set. Would it make sense to set minimumZoom to default to a higher value? @hpinkos

cguldner · 2017-07-24T16:29:13Z

@ggetz See if this is what you had in mind. I actually think the minimumZoom problem was fixed by using clamp

ggetz · 2017-07-24T17:50:41Z

Thanks @burn123, the code looks good to me, but there is one failing unit test now.

cguldner · 2017-07-24T18:14:19Z

@ggetz I think the only failing test is something with the 3D Tile Spec, and it says "Async callback was not invoked within timeout", which I don't believe is related to this. I think I was getting that error on master as well.

ggetz · 2017-07-24T20:29:47Z

I opened #5671 for the test failure.

Other than that, this looks good to me. @hpinkos is this good to go?

cguldner · 2017-07-27T14:04:03Z

@hpinkos Did you have a chance to look at this yet?

hpinkos · 2017-07-27T18:43:06Z

I'll take a look at this first thing tomorrow

cguldner · 2017-07-28T18:09:52Z

@hpinkos Do you think it would be possible to get this in by the next release? Our application is built on Cesium, but there is a good chance we will stay on Cesium 1.36 for a while, so it would be very useful to have this fixed. But I know you guys are busy, so I understand if not!

hpinkos · 2017-07-28T18:20:35Z

Yes, thanks for the reminder. I'll look at this now

hpinkos · 2017-07-28T19:00:06Z

Source/Scene/Camera.js

    function adjustBoundingSphereOffset(camera, boundingSphere, offset) {
        if (!defined(offset)) {
            offset = HeadingPitchRange.clone(Camera.DEFAULT_OFFSET);
        }

+        var defaultZoom = 100.0;


Move this back outside of the function where it was before

hpinkos · 2017-07-28T19:00:42Z

Source/Scene/Camera.js

        var range = offset.range;
        if (!defined(range) || range === 0.0) {
            var radius = boundingSphere.radius;
-            if (radius === 0.0) {
-                offset.range = MINIMUM_ZOOM;
+            if(radius === 0) {


Add back the space between if and (

hpinkos · 2017-07-28T19:01:35Z

Source/DataSources/EntityView.js

@@ -317,9 +317,6 @@ define([
            var hasViewFrom = defined(viewFromProperty);

            if (!hasViewFrom && defined(boundingSphere)) {
-                var controller = scene.screenSpaceCameraController;
-                controller.minimumZoomDistance = Math.min(controller.minimumZoomDistance, boundingSphere.radius * 0.5);


@mramato do you remember why this was added?

Because our default minimumZoomDistance (at least at the time) was fairly large, so it was impossible to zoom to a model because you would end up 50 meters away. What is the current default minimumZoomDistance?

@mramato If no minimumZoom is specified, it goes to 1, and the flyTo will go to about 100 units away if the bounding sphere for that model has a radius of 0. But this problem is fixed by the clamping of the zoom to the screenSpaceCameraController minimum and maximum zoom, so now it will never go outside those bounds.

hpinkos · 2017-07-28T19:02:56Z

Other than that, everything looks fine to me. All the tests pass locally. I just want to hear from @mramato about this change in case that behavior was added for a reason

cguldner · 2017-07-28T20:25:28Z

@hpinkos I committed the changes!

hpinkos · 2017-07-31T21:56:41Z

Thanks @burn123! This will be included in the 1.36 release, available tomorrow

cguldner added 5 commits July 22, 2017 19:34

Fix minimum zoom getting reset on flyTo

04e31a8

Fix flyTo going past minimum zoom for billboards

d1c876a

Remove console statement

2676a5e

Handle case when outside maximumZoomDistance, or when the zoomDistanc…

bcb14c7

…es haven't been set

Write tests for Camera to not fly outside limits on flyToBoundingSphere

e04bf0f

cguldner closed this Jul 23, 2017

cguldner reopened this Jul 23, 2017

ggetz requested changes Jul 24, 2017

View reviewed changes

cguldner added 2 commits July 24, 2017 10:42

Change some variable names to camelCase

875b4c8

Simplify if statement

4ba2964

hpinkos reviewed Jul 28, 2017

View reviewed changes

cguldner added 2 commits July 28, 2017 16:18

Style changes

9d00cbd

Style changes

0ccf646

hpinkos requested a review from mramato July 31, 2017 14:47

mramato approved these changes Jul 31, 2017

View reviewed changes

hpinkos merged commit 3dd45ba into CesiumGS:master Jul 31, 2017

cguldner deleted the flyToZoomLimits branch August 1, 2017 13:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix viewer.flyTo ignoring and resetting minimumZoomDistance #5668

Fix viewer.flyTo ignoring and resetting minimumZoomDistance #5668

cguldner commented Jul 23, 2017

hpinkos commented Jul 24, 2017

ggetz Jul 24, 2017

ggetz Jul 24, 2017

ggetz Jul 24, 2017

ggetz Jul 24, 2017

cguldner Jul 24, 2017

ggetz Jul 24, 2017

cguldner commented Jul 24, 2017

ggetz commented Jul 24, 2017

cguldner commented Jul 24, 2017

ggetz commented Jul 24, 2017

cguldner commented Jul 27, 2017

hpinkos commented Jul 27, 2017

cguldner commented Jul 28, 2017

hpinkos commented Jul 28, 2017

hpinkos Jul 28, 2017

hpinkos Jul 28, 2017

hpinkos Jul 28, 2017

mramato Jul 31, 2017

cguldner Jul 31, 2017 •

edited

Loading

hpinkos commented Jul 28, 2017

cguldner commented Jul 28, 2017 •

edited

Loading

hpinkos commented Jul 31, 2017

Fix viewer.flyTo ignoring and resetting minimumZoomDistance #5668

Fix viewer.flyTo ignoring and resetting minimumZoomDistance #5668

Conversation

cguldner commented Jul 23, 2017

hpinkos commented Jul 24, 2017

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

cguldner commented Jul 24, 2017

ggetz commented Jul 24, 2017

cguldner commented Jul 24, 2017

ggetz commented Jul 24, 2017

cguldner commented Jul 27, 2017

hpinkos commented Jul 27, 2017

cguldner commented Jul 28, 2017

hpinkos commented Jul 28, 2017

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

cguldner Jul 31, 2017 • edited Loading

Choose a reason for hiding this comment

hpinkos commented Jul 28, 2017

cguldner commented Jul 28, 2017 • edited Loading

hpinkos commented Jul 31, 2017

cguldner Jul 31, 2017 •

edited

Loading

cguldner commented Jul 28, 2017 •

edited

Loading