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

Fix viewer.flyTo ignoring and resetting minimumZoomDistance #5668

Merged
merged 9 commits into from
Jul 31, 2017
Merged

Fix viewer.flyTo ignoring and resetting minimumZoomDistance #5668

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
04e31a8
Fix minimum zoom getting reset on flyTo
cguldner Jul 22, 2017
d1c876a
Fix flyTo going past minimum zoom for billboards
cguldner Jul 22, 2017
2676a5e
Remove console statement
cguldner Jul 22, 2017
bcb14c7
Handle case when outside maximumZoomDistance, or when the zoomDistanc…
cguldner Jul 23, 2017
e04bf0f
Write tests for Camera to not fly outside limits on flyToBoundingSphere
cguldner Jul 23, 2017
875b4c8
Change some variable names to camelCase
cguldner Jul 24, 2017
4ba2964
Simplify if statement
cguldner Jul 24, 2017
9d00cbd
Style changes
cguldner Jul 28, 2017
0ccf646
Style changes
cguldner Jul 28, 2017
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
1 change: 1 addition & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

viewer -> Viewer

* Updated `Billboard`, `Label` and `PointPrimitive` constructors to clone `NearFarScale` parameters [#5654](https://github.com/AnalyticalGraphicsInc/cesium/pull/5654)
* Added `FrustumGeometry` and `FrustumOutlineGeometry`. [#5649](https://github.com/AnalyticalGraphicsInc/cesium/pull/5649)
* Added an `options` parameter to the constructors of `PerspectiveFrustum`, `PerspectiveOffCenterFrustum`, `OrthographicFrustum`, and `OrthographicOffCenterFrustum` to set properties. [#5649](https://github.com/AnalyticalGraphicsInc/cesium/pull/5649)
Expand Down
3 changes: 0 additions & 3 deletions Source/DataSources/EntityView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

@mramato do you remember why this was added?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@cguldner cguldner Jul 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@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.


//The default HPR is not ideal for high altitude objects so
//we scale the pitch as we get further from the earth for a more
//downward view.
Expand Down
13 changes: 9 additions & 4 deletions Source/Scene/Camera.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2853,18 +2853,24 @@ define([
return Math.max(right, top) * 1.50;
}

var MINIMUM_ZOOM = 100.0;

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

offset.range = MINIMUM_ZOOM;
} else if(radius > MAXIMUM_ZOOM) {
offset.range = MAXIMUM_ZOOM;
} else if(radius === 0) {
offset.range = DEFAULT_ZOOM;
} else if (camera.frustum instanceof OrthographicFrustum || camera._mode === SceneMode.SCENE2D) {
offset.range = distanceToBoundingSphere2D(camera, radius);
} else {
Expand Down Expand Up @@ -2950,7 +2956,6 @@ define([
//>>includeEnd('debug');

options = defaultValue(options, defaultValue.EMPTY_OBJECT);

var scene2D = this._mode === SceneMode.SCENE2D || this._mode === SceneMode.COLUMBUS_VIEW;
this._setTransform(Matrix4.IDENTITY);
var offset = adjustBoundingSphereOffset(this, boundingSphere, options.offset);
Expand Down
2 changes: 0 additions & 2 deletions Source/Widgets/Viewer/Viewer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1914,8 +1914,6 @@ Either specify options.terrainProvider instead or set options.baseLayerPicker to
viewer.trackedEntity = undefined;

var boundingSphere = BoundingSphere.fromBoundingSpheres(boundingSpheres);
var controller = scene.screenSpaceCameraController;
controller.minimumZoomDistance = Math.min(controller.minimumZoomDistance, boundingSphere.radius * 0.5);

if (!viewer._zoomIsFlight) {
camera.viewBoundingSphere(boundingSphere, viewer._zoomOptions);
Expand Down
30 changes: 30 additions & 0 deletions Specs/Scene/CameraSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, use camel case

scene.screenSpaceCameraController.minimumZoomDistance = MIN_VALUE;

var sphere = new BoundingSphere(Cartesian3.fromDegrees(-117.16, 32.71, 0.0), 10.0);

camera.flyToBoundingSphere(sphere, {
duration : 0.0
});

var distance = Cartesian3.distance(camera.position, sphere.center);
expect(CesiumMath.equalsEpsilon(distance, MIN_VALUE, 0.1)).toBe(true);
});

it('flyToBoundingSphere does not zoom further than maximumZoomDistance', function() {
scene.mode = SceneMode.SCENE3D;
var MAX_VALUE = 10000;
scene.screenSpaceCameraController.maximumZoomDistance = MAX_VALUE;

var sphere = new BoundingSphere(Cartesian3.fromDegrees(-117.16, 32.71, 0.0), 100000);

camera.flyToBoundingSphere(sphere, {
duration : 0.0
});

var distance = Cartesian3.distance(camera.position, sphere.center);
expect(CesiumMath.equalsEpsilon(distance, MAX_VALUE, 0.1)).toBe(true);
});

it('distanceToBoundingSphere', function() {
scene.mode = SceneMode.SCENE3D;

Expand Down