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 pickPosition accuracy when depthTestAgainstTerrain is false #8207

Closed
wants to merge 4 commits into from

Conversation

lilleyse
Copy link
Contributor

@lilleyse lilleyse commented Sep 20, 2019

Fixes #8179. Merge #8205 first.

Local sandcastle for testing

This builds on top of #8205 by using the "everything-else" color texture to determine whether the pick depth should take the scene's depth value or the globe's depth value for a given pixel. If a pixel in the color texture is opaque it writes the scene depth, otherwise it writes the globe depth.

The one downside I noticed so far is that is breaks scene.pickTranslucentDepth since those commands are depth-only and the color texture can't be relied on. I tried a few ideas like writing color in that pass but it exposes a different bug that is very similar to #7429 (because my testing Sandcastle happens to use EDL). I have an idea for doing this whole PR differently by rendering a stencil bit for the depth plane and doing a two-pass copy. I'll see how that goes and then bump this PR for review.

@cesium-concierge
Copy link

Thanks for the pull request @lilleyse!

  • ✔️ Signed CLA found.
  • ❔ 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.

@mramato
Copy link
Contributor

mramato commented Sep 23, 2019

@lilleyse the only change in this PR is to CHANGES.md, did you PR into the right branch?

@lilleyse
Copy link
Contributor Author

@mramato looks like I accidentally pushed these same changes to the target branch. Should be fixed now.

@lilleyse
Copy link
Contributor Author

Need to check if this fixes #4368

@lilleyse lilleyse changed the base branch from fix-multifrustum to master October 21, 2019 13:55
@lilleyse
Copy link
Contributor Author

Decided to go with a different approach. See #8179 (comment).

@lilleyse lilleyse closed this Oct 21, 2019
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.

pickPosition isn't accurate when depthTestAgainstTerrain is false
3 participants