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

Generate tighter near far bounds in View.js #8850

Merged
merged 15 commits into from
Jun 28, 2020
Merged

Conversation

IanLilleyT
Copy link
Contributor

@IanLilleyT IanLilleyT commented May 13, 2020

Fixes #8346
Fixes #8439

View.js is responsible for deciding which commands get rendered into which frustums, and every frame it calculates the global near / far values of all commands in order to figure out how many frustums are needed. Problem is, the heuristics it was using to decide if the frustums needed to be adjusted was not triggered when switching to orthographic camera, resulting in the orthographic transform using near / far bounds meant for logarithmic depth (since perspective camera uses logdepth and we just switched away from it). This resulted in an extremely tiny value in the orthographic matrix which ended up becoming 0 by the time it was converted to float32 for the shader. So all triangles were considered to have the same Z value post-transformation, overwriting each other.

This PR recalculates the frustums whenever the near / far changes, resulting in a much more sane transformation matrix. It also has the added benefit of making our depth values more accurate for all kinds of projections, though I haven't noticed any visual quality difference.

This can be seen from displaying the camera's frustum before and after using tighter near / far (using perspective + log depth):

Before:

Screen Shot 2020-05-13 at 6 22 57 PM

After:

Screen Shot 2020-05-13 at 6 18 26 PM

One thing to watch out for is now the near / far change constantly when you move the camera. I don't think this is a problem though.

In working on this a bug, another bug popped up with point cloud eye dome lighting. Somehow, the farther near value created some floating point imprecision issues when reading and writing the log depth value. Then @lilleyse realized we could just passthrough the depth value without unpacking and repacking it, solving the problem. This hit a snag with derived commands because it expects to see czm_writeLogDepth, otherwise it appends log depth code to the shader. Had to add a LOG_DEPTH_WRITE define to inform the system that log depth was truly being written already.

@cesium-concierge
Copy link

Thanks for the pull request @IanLilleyT!

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

@IanLilleyT IanLilleyT requested a review from lilleyse May 13, 2020 23:02
@IanLilleyT
Copy link
Contributor Author

IanLilleyT commented May 15, 2020

Turns out this PR fixed the polyline subissue in #5110 (which are technically not ground polylines, which is why they weren't affected by the fix here: #8854)

@hpinkos
Copy link
Contributor

hpinkos commented May 29, 2020

bump @lilleyse

@IanLilleyT
Copy link
Contributor Author

I think it makes sense to move this to the July release, but we should merge it as soon as possible after the June release is out.

@hpinkos
Copy link
Contributor

hpinkos commented Jun 15, 2020

@IanLilleyT @lilleyse bump

@lilleyse
Copy link
Contributor

I like the approach and I like how much the simpler the code got. Performance is comparable with master - this sandcastle shows createPotentiallyVisibleSet using about 0.2% less in the chrome profiler on this branch vs. master with built versions of sandcastle.

The only thing that should be addressed is that commandExtents may hold onto old draw command references like ManagedArray did before #8889. It'll still be important to have a CommandExtent pool but extents past the end of the current length should set have their command set to undefined.

@lilleyse
Copy link
Contributor

This also appears to fix #8439. I added "Fixes #8439" to the PR description.

@IanLilleyT
Copy link
Contributor Author

@lilleyse updated

@lilleyse
Copy link
Contributor

The latest changes look good. Thanks @IanLilleyT

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.

The positions from #scene.pickFromRay not on model surface Toggling Orthorgraphic can break everything
4 participants