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 classification when terrain depth writes are enabled #7422

Merged
merged 16 commits into from
Dec 19, 2018

Conversation

lilleyse
Copy link
Contributor

@lilleyse lilleyse commented Dec 17, 2018

The stencil buffer now stores a bit that says whether a pixel is 3D Tiles or not. This allows a classification primitive with classificationType = ClassificationType.CESIUM_3D_TILE to apply just to 3D Tiles even if the terrain depth is written. This also lets the depth plane get rendered immediately after the terrain pass which solves #6867, so 3D Tiles no longer show through the opposite side of the globe.

This is going into a staging branch and the follow up PR is going to be enabling textured poylgons and polylines on 3D Tiles, which is a lot easier with this change.

To do:

  • Fix unit tests
  • CHANGES.md

Next:

@cesium-concierge
Copy link

Thanks for the pull request @lilleyse!

  • ✔️ Signed CLA found.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@@ -1261,30 +1263,42 @@ define([
derivedCommands.originalCommand = deriveCommand(command);
command.dirty = false;
}
var originalCommand = derivedCommands.originalCommand;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of the changes in this function are just cleanup rather than bug fixes.

@lilleyse lilleyse force-pushed the stencil-tricks branch 2 times, most recently from acac7f8 to 3302245 Compare December 18, 2018 18:57
@bagnell
Copy link
Contributor

bagnell commented Dec 18, 2018

I see 71 test failures after resetting the branch to the latest commit.

There is an optimization you could make when the classification type is both terrain and 3D Tiles without the invert classification. You could render both terrain and 3D Tiles, render the first two commands for each of the classification types, and then issue a single color draw call. Given that classification primitives are expensive to render, this might be a significant performance improvement, but it doesn't have to hold up this PR.

@lilleyse
Copy link
Contributor Author

lilleyse commented Dec 18, 2018

I'm still fixing up the unit tests and will bump when they're ready.

Yeah that idea might work. It will be a good win too because in the next PR I might set all the classificationType defaults to BOTH.

I also have ideas for speeding up invert classification. The opaque version wouldn't need a separate FBO, just one full screen pass on the main framebuffer. I think the translucent version can maybe be shrunk to a single FBO rather than two, but I haven't thought it through enough yet.

@lilleyse
Copy link
Contributor Author

lilleyse commented Dec 19, 2018

@bagnell this is ready. All tests pass locally now and the Sandcastle demos looks normal. There was a bug with translucent invert classification that is now fixed.

One change I made while writing tests was to set Vector3DTileGeometry and Vector3DTilePolygons classificationType to ClassificationType.BOTH by default. I don't think this actually affects anything other than the tests.

@bagnell
Copy link
Contributor

bagnell commented Dec 19, 2018

This looks good to me. Merging this will close the bugs. Should they stay open until the staging branch is merged?

@lilleyse
Copy link
Contributor Author

Oh good call, I'll remove those lines from the summary.

@lilleyse
Copy link
Contributor Author

Done

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