Skip to content

Add silhouettes to ModelExperimental #10457

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

Merged
merged 12 commits into from
Jun 21, 2022
Merged

Conversation

j9liu
Copy link
Contributor

@j9liu j9liu commented Jun 14, 2022

This PR adds support for silhouettes in ModelExperimental, like so:
image

I also found that ModelExperimentalSpec had a lot of invalid unit tests and was missing some for ModelExperimental.color, so I fixed those as well.

Here's the Sandcastle for testing silhouettes.

Testing Notes

  • Silhouettes don't work properly for 2D / CV in Model, so they won't work here.
  • One bug I still have to figure out: the "Instanced Box" model doesn't work with translucent silhouettes for some reason, even though all the other models work. I can't compare the results to Model either because Model doesn't support GPU instancing, but the silhouette works for this model if the draw command passes are the same (opaque model color + silhouette, translucent model color + silhouette). FIXED

@cesium-concierge
Copy link

Thanks for the pull request @j9liu!

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

@j9liu
Copy link
Contributor Author

j9liu commented Jun 14, 2022

Figured out what the bug was: in CPUStylingStageVS and CPUStylingStageFS, the model wouldn't show up if the pass of the command was translucent, but the feature color wasn't. This was preventing the translucent silhouette from rendering for an opaque model with feature IDs. Should be fixed with the recent commit.

@j9liu j9liu requested a review from ptrgags June 14, 2022 20:11
Copy link
Contributor

@ptrgags ptrgags left a comment

Choose a reason for hiding this comment

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

@j9liu I had a few comments.

Also I tried the sandcastle and it seems to be rendering only the silhouette:

image

Though it seems to work fine for invisible models:
image

@j9liu
Copy link
Contributor Author

j9liu commented Jun 21, 2022

@ptrgags - updated, let me know if the uniform change from float to bool resolved the bug or not.

@j9liu
Copy link
Contributor Author

j9liu commented Jun 21, 2022

@ptrgags ready!

Copy link
Contributor

@ptrgags ptrgags left a comment

Choose a reason for hiding this comment

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

@j9liu this is getting close, it works even with custom shaders! though a point cloud without normals failed:

Sandcastle

@j9liu
Copy link
Contributor Author

j9liu commented Jun 21, 2022

@ptrgags updated with a fix for the point cloud crash!

@ptrgags
Copy link
Contributor

ptrgags commented Jun 21, 2022

Looks good, thanks @j9liu!

@ptrgags ptrgags merged commit b50026d into main Jun 21, 2022
@ptrgags ptrgags deleted the model-experimental-silhouette branch June 21, 2022 20:11
@j9liu j9liu mentioned this pull request Jun 28, 2022
51 tasks
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