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.

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 zooming out speed after reaching minimum zoom distance. #9932

Merged
merged 3 commits into from
Jan 25, 2022

Conversation

puckey
Copy link
Contributor

@puckey puckey commented Nov 16, 2021

The minimum zoom distance causes the zoom speed to slow as you arrive at the minimum zoom. This feels right, but when you zoom back out again from that minimum zoom, the zooming takes too long to get up to speed.

In the following demo, use your scroll wheel to zoom in as far as you can and then try zooming out again:

Sandcastle demo: https://sandcastle.cesium.com/#c=bY5PSwMxEMW/SsipBUkoBSuYLsL2KHgoeJBcptlRByfJkj9b6qc3u72Idg4Db+b33swESUyEZ0xiLwKeRY+Zqlevy2xlpVt0H0MBCpisXD/acHWo7DBg6wkxHEdw2IPHBDOcInMjPAXy1b/F6A+UCwSH7c79dvew2e5akLyTJpcLY2eDWOqJ/BhTETXxSild0I8MBbM+VfeFRbmc5w9m1OjfVjPQJGjY33hZOIac2+a9Mh/pG63sjG78PytHGCh8vEyYGC4z9rnpnq9DpZTRTd52lhj5BOlP8g8

When specifying a minimum zoom distance like so:

var viewer = new Cesium.Viewer("cesiumContainer");
viewer.scene.screenSpaceCameraController.minimumZoomDistance = 6378137;

This pull request avoids slowing down zooming as you zoom out.

@cesium-concierge
Copy link

Thanks for the pull request @puckey!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.

@puckey puckey force-pushed the feature/zoom-out-from-min-distance branch from c1a9c33 to a82aef3 Compare November 16, 2021 14:33
@youlinaa
Copy link

any news on this?

@puckey
Copy link
Contributor Author

puckey commented Jan 22, 2022

Note that you can see this in production on http://radio.garden - zoom in all the way and out again

@youlinaa
Copy link

Yeah, that's great, but is there any reason to stop merging to master, can you fix that?

@ggetz
Copy link
Contributor

ggetz commented Jan 24, 2022

Thanks @puckey! I was able to test this out with both the mouse and touch controls. Both are now snappier once the minimum has been reached.

Since this changes existing behavior, can you add a note to CHANGES.md?

// distanceMeasure should be the height above the ellipsoid.
// The zoomRate slows as it approaches the surface and stops minimumZoomDistance above it.
var minHeight = object.minimumZoomDistance * percentage;
var minHeight = diff < 0 ? 0 : object.minimumZoomDistance * percentage;
Copy link
Contributor

@ggetz ggetz Jan 24, 2022

Choose a reason for hiding this comment

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

For readability, can you either 1) break this out into a descriptive variable, or 2) expand on the comment above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it to:

  // distanceMeasure should be the height above the ellipsoid.
  // When approaching the surface, the zoomRate slows and stops minimumZoomDistance above it.
  var approachingSurface = diff > 0;
  var minHeight = approachingSurface ? object.minimumZoomDistance * percentage : 0;
  var maxHeight = object.maximumZoomDistance;

@puckey puckey force-pushed the feature/zoom-out-from-min-distance branch from a82aef3 to 8e313d9 Compare January 25, 2022 09:32
@puckey puckey force-pushed the feature/zoom-out-from-min-distance branch from 8e313d9 to 5b75a74 Compare January 25, 2022 09:53
@puckey
Copy link
Contributor Author

puckey commented Jan 25, 2022

@ggetz Thanks for reviewing – I added a note about the changes to CHANGES.md

@ggetz
Copy link
Contributor

ggetz commented Jan 25, 2022

Awesome, thanks again @puckey!

@ggetz ggetz merged commit 691b1c6 into CesiumGS:main Jan 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Issue/PR closed
Development

Successfully merging this pull request may close these issues.

4 participants