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

Add translucent tile classification #9399

Merged
merged 14 commits into from
Mar 9, 2021
Merged

Conversation

ebogo1
Copy link
Contributor

@ebogo1 ebogo1 commented Mar 2, 2021

  • Adds TranslucentTileClassification and specs for handling translucent tile classification
  • Adds DrawCommand. depthForTranslucentClassification and logic in Cesium3DTile for using it
  • Adds handling of translucent classification in Scene and View

@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.

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 2, 2021 23:06
@lilleyse
Copy link
Contributor

lilleyse commented Mar 4, 2021

I pushed a premultiplied alpha fix similar to the one in #9398 (comment): 43f6efd. The colors should look much better now.

Before After
before after
Sandcastle

@lilleyse
Copy link
Contributor

lilleyse commented Mar 4, 2021

Overall this looks pretty good! At some point it would be nice to unify this with GlobeTranslucency since they do kind of the same thing, but I'm relatively happy with this approach for now.

I went through the checklist in #8726 and all of those situations worked great. There are a couple artifacts I noticed separately though:

  1. The translucent classification framebuffer is always rendered above all translucent objects in the scene. I don't think this is fixable without some major changes to the approach. GlobeTranslucency doesn't have this problem because it renders classification to a framebuffer first and then creates a derived command for each TRANSLUCENT command that applies the classification. I.e. classification is not applied as a screen-wide post process.

order
Sandcastle

  1. I see a slight color change when I pick at certain angles

clicking

@likangning93 @ebogo1 do you have any ideas about quick fixes for these? If there no obvious solution we should open a separate issue for each of these artifacts and move on.

@likangning93
Copy link
Contributor

@ebogo1 here's a simpler localhost Sandcastle for stepping through the algorithm.

@ebogo1
Copy link
Contributor Author

ebogo1 commented Mar 8, 2021

@lilleyse @likangning93 I reverted the changes to the noClassificationModels and classificationType flags that were once part of https://github.com/CesiumGS/cesium-analytics/pull/620/ (these will be part of #9399). Tested this in Sandcastle and it seemed to work fine, so I think this should be ready once CI passes on 0398f15.

@lilleyse
Copy link
Contributor

lilleyse commented Mar 9, 2021

I realized I broke some unit tests with the premultiplied alpha change earlier. I fixed those in 397ece9.

Thanks @ebogo1 and @likangning93!

@lilleyse lilleyse merged commit 91ff930 into master Mar 9, 2021
@lilleyse lilleyse deleted the translucent-classification branch March 9, 2021 00:50
@lilleyse
Copy link
Contributor

lilleyse commented Mar 9, 2021

@ebogo1 can you open two separate issues for #9399 (comment)?

@lilleyse
Copy link
Contributor

lilleyse commented Mar 9, 2021

The second issue in #9399 (comment) is actually pretty serious when picking on mouse move. I confirmed that this problem happens even before the premultiplied alpha fix.

Even though this PR is merged we should address this bug in a follow up PR. CC @likangning93 @ebogo1

Peek 2021-03-08 21-37
Sandcastle

@lilleyse
Copy link
Contributor

lilleyse commented Mar 9, 2021

Just noticed another bug while testing #9398 (but not related to that PR)

In this Sandcastle if you repeatedly toggle tileset translucency on/off it will eventually crash.

Peek 2021-03-08 21-52

Selection_688

@lilleyse
Copy link
Contributor

lilleyse commented Mar 9, 2021

After offline discussion, @ebogo1 can you do the following:

@ebogo1
Copy link
Contributor Author

ebogo1 commented Mar 9, 2021

Thanks @lilleyse! Opened the three issues above.

@midnight-dev
Copy link

The changelog entry for this PR has a minor typo.

Added support for for drawing ...

@lilleyse
Copy link
Contributor

Thanks @midnight-dev, I pushed a fix directly to master: 0c31cdf

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.

5 participants