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 crash when toggling translucency #9447

Merged
merged 2 commits into from
Mar 29, 2021
Merged

Conversation

ebogo1
Copy link
Contributor

@ebogo1 ebogo1 commented Mar 25, 2021

Fixes #9421.

The idea behind this fix is that it is possible for the translucent command in TranslucentTileClassification to have a previously destroyed shader by the time getDerivedCommand is called from executeTranslucentCommands. One clue that supports this idea is that the crash only seems to happen when the translucency checkbox is turned on, and not off (see the Sandcastle). If the command is dirty by the time getDerivedCommand is called, the shader should be recreated instead of reusing the outdated one from when the tileset was opaque.

I'm still not 100% sure when the command state gets messed up, but I believe the bug happens because:

  • Toggling on translucency changes the translucency command's shaders.
  • The translucency command now has dirty = true.
  • Scene never checks whether the translucent commands are dirty (i.e. like it does with shadow maps in updateDerivedCommands), and the state is only visible from the loop over commands inside executeTranslucentCommands. For this reason, checking for ... || command.dirty inside TranslucentTileClassification#getDerivedCommand makes sense to me (as opposed to somewhere higher up like Scene).
  • If the derived command is never updated in a dirty command, the shader program can be pointing to an outdated shader that has already been destroyed.

@cesium-concierge
Copy link

Thanks for the pull request @ebogo1!

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

@ebogo1 ebogo1 requested a review from lilleyse March 25, 2021 22:18
@lilleyse
Copy link
Contributor

Unfortunately due to how convoluted derived commands are the simple fix ended up not being enough. Stepping through the code the log depth command was always dirty and the original command was always not dirty. This meant that when log depth was enabled it would create a new depth-only derived command every frame, and when log depth was disabled the fix wouldn't work, both bad.

Scene.prototype.updateDerivedCommands is what handles deriving commands and it's called well before TranslucentTileClassification.prototype.executeTranslucentCommands, so relying on the dirty property won't work in this case.

I realized there was a simpler fix that could use the existing depth-only derived command that's created in updateDerivedCommands in Scene. This depth-only command is created for all commands except when pickOnly = true. pickOnly will be false for opaque/translucent 3D Tiles commands so it's safe to rely on it existing.

The last fix was addressing the render states. Previously the TranslucentTileClassification derived command would set rs.stencilTest = StencilConstants.setCesium3DTileBit(); to order for classification to work at all. The better fix was to set the stencil state when the translucent render states were originally created.

Because of the idiosyncrasies of derived commands, stencil state, etc I just pushed the changes directly. At some point we should refactor derived commands because they are very difficult to navigate.

@lilleyse lilleyse force-pushed the translucent-crash branch from f0ebb88 to 83ca0bb Compare March 26, 2021 22:12
@lilleyse lilleyse merged commit 73d886b into master Mar 29, 2021
@lilleyse lilleyse deleted the translucent-crash branch March 29, 2021 15:27
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.

Repeatedly toggling tileset translucency causes Sandcastle crash
3 participants