Fix for clipping and camera navigation issues below ellipsoid

[chris-cooper]

After re-looking at #9198 it seems to be related to DepthPlane usage in Scene. It seems the DepthPlane is used for the following purpose:

https://github.com/CesiumGS/cesium/blob/master/Source/Scene/Scene.js#L3243-L3245

I tried a quick workaround to offset down by an arbitrary amount. It seems to address the reported issue.

Assuming this is a valid fix, how should it be implemented?

• Simple magic number (as shown in b96d673) Perhaps it should be -11000m to cater for bathymetry?
• Viewer constructor options argument that is passed Viewer=>Scene=>DepthPlane
• Other

You should find the issues are improved or resolved running a local Cesium from this branch and the same sandcastle code from a local server:
local sandcastle link

[cesium-concierge]

Thanks for the pull request @chris-cooper!

• ✔️ 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.

[OmarShehata]

Thanks for opening a PR Chris! There was some discussion here about pushing the depth plane down or other techniques that may be relevant here: #7879

[chris-cooper]

Thanks @OmarShehata . After looking at https://github.com/CesiumGS/cesium/pull/8398/files it looks like the default for frameState.minimumTerrainHeight could be set differently (e.g. an option argument for Scene) and then used by DepthBuffer. I've tweaked this PR as an example.

[cesium-concierge]

Thanks again for your contribution @chris-cooper!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

[cesium-concierge]

Thanks again for your contribution @chris-cooper!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

[cesium-concierge]

Thanks again for your contribution @chris-cooper!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

[chris-cooper]

What would you like to do with this PR @OmarShehata ?

[grantis]

What would you like to do with this PR @OmarShehata ?

Is this PR waiting on anything, this PR would possibly fix an issue we are having.

[cesium-concierge]

Thanks again for your contribution @chris-cooper!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

[grantis]

@OmarShehata @chris-cooper Hey folks, is this PR going to be in the next release? This appears to relate to a issue our customers face in locations such as Florida Keys.

I've looked at the travis-ci and it looks like the tests are failing is that what is currently blocking the merge?

[dzungpng]

We are looking into this PR and other possible ways to resolve the issue. The current estimate date is the May release. @grantis

[slchow]

We got another bump for this issue (and request for an updated ETA) from the forum: https://community.cesium.com/t/new-cesiumjs-release/11110/15

[katSchmid]

Any news on this? Thanks

[cesium-concierge]

Thanks again for your contribution @chris-cooper!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

[cesium-concierge]

Thanks again for your contribution @chris-cooper!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

[ggetz]

Hi @chris-cooper and all, we just talked through this PR with @lilleyse. This is definitely a problem we've run into ourselves. While this does technically address the issue, it's a bit of a band-aid fix. We agree that 1000 seems a bit arbitrary. Can we get a real world constant, keeping in mind that some users are not using earth as their ellipsoid?
Also it may be better for this constant to live on the depth plane.

Also generally for bathymetry we disable the depth plan entirely.

[lilleyse]

Also generally for bathymetry we disable the depth plan entirely.

Slight correction - we disable the depth plan when the camera is underground or the globe is translucent, but not necessarily for bathymetry.

[chris-cooper]

Thanks @ggetz and @lilleyse . I've updated this PR to the latest master + tried to address your comment @ggetz . Is this what you meant?

local sandcastle example

[ggetz]

Thanks for the update @chris-cooper! This approach looks better. Would you be able to add documentation and unit tests, and update CHANGES.md?

[chris-cooper]

I've added some of those things @ggetz . For now it is just testing the option gets passed through. I might need some help from someone with knowledge of that area of the rendering code if we want to test it more rigorously.

[ggetz]

Thanks @chris-cooper! I suppose to test this more rigorously, we could add a new test that confirms the depth plane draw command has boundingVolume set to the expected value. But no similar tests currently exist for DepthPlane. @lilleyse Any thoughts?

[lilleyse]

A test like that could be added SceneSpec.js

[chris-cooper]

I've tried adding a spec for that thanks @ggetz @lilleyse .

[ggetz]

Awesome! Thanks for your patience with this PR @chris-cooper !

[v8gx]

Without the logarithmic depth buffer this issue still reproduces. Here is a sandcastle where you can see the error by tilting the camera up.

https://sandcastle.cesium.com/#c=ZZJfb9MwFMW/ipWXZlLqJGvLRJtVQFuJB1gnNsFLJOTaN4mFY0e2k6mgfXdu/hTYyJPv9fndnHOTjlnSSXgCS26JhieyAyfbmn4deuGMD+XOaM+kBjuLyK9cE3w8WIute2s6KcCuLyC3wDx8M1aJx1ESXkUjIqDx1b1iGg5KycYZKY5F4cCvyTxNkiTXz1ebXOd6NEQdBw20VOYEdGAfwfn3JY50fpqNpgumHGxeQcqUzEpf1ZLve/JDWxRDxIsa9ZgcsDNxoL30EhxlQoRDxsY47BhN/mTbMevxxPSCFtbUeygtgAvnaXpD0+VqcZNEZHFN3ywX12+XJCLzVTJmr40AhXOm3bVWYjGjNH5gdaNgzzyLB42Lx1d9Zno6fccj7uA0m5ZYgSwr/wUwDmgOf819fHlB7453h4lxnClYk1XSl8/Tmqfc3jL+A8Shj3/GdcAmiILM+bOC7Ui/k3VjrEfXKkTLHtAyfmIXn1okPeXO9fOy+AJlQnZEits8ePX35AHhijmHN0Wr1IP8CXmwzWLUv8CUYULq8tiBVezcS6p0+2lsUkqzGMv/KW+MOjH7z8Tf

[ggetz]

Thanks for the report @v8gx. I opened #10166 to track. Would you possibly be able to contribute a fix?