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 camera controller issue when tilt on terrain #9562

Merged
merged 10 commits into from
Jun 1, 2021

Conversation

renjianqiang
Copy link
Contributor

When i drag the mouse middle button diagonally along the screen, i find there are some wrong with camera controller:
bug

Then i find the tilt3DOnTerrain in ScreenSpaceCameraController.js have some problems. It first calculated verticalCenter, and do horizontal rotate first, but now verticalCenter has changed after horizontal rotate, so there are some errors introduced here. I fix this problems by change the rotation order.

@cesium-concierge
Copy link

Thank you so much for the pull request @renjianqiang! I noticed this is your first pull request and I wanted to say welcome to the Cesium community!

The Pull Request Guidelines is a handy reference for making sure your PR gets accepted quickly, so make sure to skim that.

  • ❌ Missing CONTRIBUTORS.md entry.
  • ✔️ 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.
  • Works (or fails gracefully) in IE11.

@ebogo1
Copy link
Contributor

ebogo1 commented May 25, 2021

Thanks @renjianqiang. I have to think a bit more about the changes but I can reproduce the issue you described and it looks like these changes fix it. Could you please update CHANGES.md in the "Fixes" category?

@renjianqiang
Copy link
Contributor Author

@ebogo1 CHANGES.md has been updated.

Copy link
Contributor

@ebogo1 ebogo1 left a comment

Choose a reason for hiding this comment

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

The idea behind this change makes sense to me, but I'm confused why the change doesn't introduce a new problem when using the old transform matrix before the call to rotate3D() with the horizontal flag set to true. It's an oversimplification, but if I understand correctly the current code computes the tilt like this:

setTransform(transform);
rotate3D(verticalOnly=false);

setTransform(verticalTransform);
rotate3D(verticalOnly=true);

And the changes like this:

setTransform(verticalTransform);
rotate3D(verticalOnly=true);

setTransform(transform);
rotate3D(verticalOnly=false);

The changes in this PR aim to fix the camera issue by performing the vertical rotate3D before the verticalCenter changes (and consequently, the verticalTransform) from the horizontal rotate3D. Maybe I'm missing something here, but wouldn't center change after the vertical rotation the same way?

I also have a minor suggestion for the wording in CHANGES.md.

@renjianqiang Do you have an idea about why it is safe to reuse the old transform matrix with these changes? I think I'm on board with your changes but it'd be nice to have a better intuition for why this works before merging.

CHANGES.md Outdated Show resolved Hide resolved
Co-authored-by: Eli Bogomolny <31491650+ebogo1@users.noreply.github.com>
@renjianqiang
Copy link
Contributor Author

@ebogo1 yeap, you are right, what I did was just changed the rotation order!

Maybe I'm missing something here, but wouldn't center change after the vertical rotation the same way?

Because the center is the point picked when the middle button down, and it is fixed in one drag period. But the verticalCenter will computed per frame when tilting on terrain. I will show you some picture:

1

As you see in the picture, the yellow verticalCenter is calculated by follow code:

var windowPosition = tilt3DWindowPos;  
windowPosition.x = canvas.clientWidth / 2;   // this is the horizontal center of the screen
windowPosition.y = controller._tiltCenterMousePosition.y; // _tiltCenterMousePosition is the position where mouse start clicked
ray = camera.getPickRay(windowPosition, tilt3DRay);
 
var mag = Cartesian3.magnitude(center);
var radii = Cartesian3.fromElements(mag, mag, mag, scratchRadii);
var newEllipsoid = Ellipsoid.fromCartesian3(radii, scratchEllipsoid);

intersection = IntersectionTests.rayEllipsoid(ray, newEllipsoid);

Now, if we rotate horizontally first, the ray will change because the pivot is the center , and the verticalCenter actully has been changed, but we didn't update it, finally the errors introduced.

But if we rotate vertically first, the verticalCenter will not change because the pivot is the verticalCenter, and then we rotate horizontally around center which is fixed in one drag period, now the camera is behaving normally.

In addition,we can fix this problem by recalculate the verticalCenter after the horizontal rotation.

Hope this helpful!

@ebogo1
Copy link
Contributor

ebogo1 commented Jun 1, 2021

@renjianqiang Thanks for the write-up - that makes sense. I pushed a couple changes to CHANGES.md but this will be ready after the CI passes.

@ebogo1 ebogo1 merged commit b17cef0 into CesiumGS:master Jun 1, 2021
j9liu pushed a commit that referenced this pull request Jun 2, 2021
fix camera controller issue when tilt on terrain
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.

3 participants