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

Added maximumTiltAngle to limit how much the camera can tilt #12169

Merged
merged 7 commits into from
Oct 9, 2024

Conversation

albek446
Copy link
Contributor

@albek446 albek446 commented Aug 29, 2024

Description

Added a maximumTiltAngle property in the ScreenSpaceCameraController which, if set, limits the camera's tilt.
The maximumTiltAngle value is expressed in radians.

Issue number and link

Fixes #9689

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have updated the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

Copy link

github-actions bot commented Aug 29, 2024

Thank you for the pull request, @albek446! Welcome to the Cesium community!

In order for us to review your PR, please complete the following steps:

Review Pull Request Guidelines to make sure your PR gets accepted quickly.

@DoisKoh
Copy link

DoisKoh commented Sep 19, 2024

Let's go! Sign it bro!

@albek446
Copy link
Contributor Author

Let's go! Sign it bro!

Still waiting for approval before I can send in the CLA. But I am still working on it

@ggetz
Copy link
Contributor

ggetz commented Sep 27, 2024

I can confirm we now have a CLA on file!

@lukemckinstry Could you please take a pass on reviewing this PR?

Copy link
Contributor

@lukemckinstry lukemckinstry left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this PR to fix this long running issue. This fix seems to work as intended.

I know I have encountered this error myself, but following the basic instructions from the original/primary forum thread https://community.cesium.com/t/how-to-limit-tilt/14459 I was surprised to be having trouble reproducing the error on a plain ellipsoid.

Using a mouse we can easily and consistently end up “under ground” or get “lost” in the scene by clicking on the globe and moving the mouse to tilt the scene to both extremes (+/- 90 degrees) repeatedly. The severity of the problem caused in the scene depends on the zoom level.

But shifting to some non plain ellipsoid examples, like

  1. this underground sandcastle example from a similar (possibly overlapping) issue View rotation gets stuck near ground #12137

  2. a basic example with Cesium World Terrain

const viewer = new Cesium.Viewer("cesiumContainer", {
 terrain: Cesium.Terrain.fromWorldTerrain(),
});

I was able to reproduce the error and confirm the fix worked when I set maximumTiltAngle equal to Math.PI / 2

One comment I would like to raise: I think putting the property in radians is a good call. It seems like most users want to prevent tilting/rotation underground (based on the github issues and forum comments) so I think we could do users a favor by adding an example to the doc string like I commented below to show how.
What do you think?

canvas.clientHeight / 4
);

moveMouse(MouseButtons.MIDDLE, startPosition, endPosition);
Copy link
Contributor

Choose a reason for hiding this comment

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

why repeat this mouse move? If it is valid can you add a comment to explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Certainly.
The reason why I repeated the mouse move was because I could not find a way to get the tilting angle to 45 degrees (0.25π radians) in just one move. I tried to set the endPosition to different values but could not get the final tilt angle high enough to the limit I wanted to test. Maybe I am missing something, and there is a better way?

@lukemckinstry
Copy link
Contributor

Hey @ggetz I think this is ready for a second review

@ggetz
Copy link
Contributor

ggetz commented Oct 3, 2024

Thanks @albek446 and @lukemckinstry!

@albek446 Please merge in the latest main branch. We recently merged a change to our formatter, so please follow these instructions to do so.

@albek446
Copy link
Contributor Author

albek446 commented Oct 3, 2024

@ggetz I have now merged in the latest updates from the main branch and run the prettier steps.

@albek446
Copy link
Contributor Author

albek446 commented Oct 9, 2024

@ggetz any update on this? Was my last update ok?

Copy link
Contributor

@ggetz ggetz left a comment

Choose a reason for hiding this comment

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

Thanks for the updates here @albek446!

I've tested this out, and it's working very well in both 3D and CV modes.

I made a few small adjustments to the inline documentation, and moved the change note to the next upcoming release. This should be good to merge once CI checks pass!

@ggetz ggetz merged commit 7aefae9 into CesiumGS:main Oct 9, 2024
4 checks passed
@albek446 albek446 deleted the maximum-tilt branch October 10, 2024 06:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Limiting Camera Tilting
4 participants