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

Don't use enormous offset for second log depth frustum near plane. #8727

Merged
merged 2 commits into from
Apr 6, 2020

Conversation

kring
Copy link
Member

@kring kring commented Apr 6, 2020

When log depth is enabled, and multiple frustums are still needed, Scene previously made the next near plane overlap the previous one by 10% for opaque rendering (as compared to 0.01% overlap when log depth is disabled) . With the default setup, this is 10 million meters. When a scene has a mix of opaque and translucent objects, 10 million meters of eye-space difference is easily visible as incorrect depth testing. This PR changes log depth to use the same overlap as non-log-depth (0.01%).

Since I have no idea why this was set to 10% in the first place, there's a decent chance this will introduce a bug elsewhere. But multiple frustums are rare with log depth anyway, and any artifacts would be seams between frustums, which I expect to be much less offensive than rubbish depth testing.

This PR also includes a small tweak to how v_depthFromNearPlusOne is calculated that might make it more numerically stable, but I couldn't see a difference either way from it.

Fixes #8725

@cesium-concierge
Copy link

Thanks for the pull request @kring!

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

@lilleyse
Copy link
Contributor

lilleyse commented Apr 6, 2020

Thanks @kring

@lilleyse lilleyse merged commit 0298fa0 into master Apr 6, 2020
@lilleyse lilleyse deleted the log-depth-frustum-offset branch April 6, 2020 14:23
@TJKoury
Copy link
Contributor

TJKoury commented Apr 6, 2020

Thanks @kring!

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.

Occlusion issue with changes to log depth equation
4 participants