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

Adds styling for ModelExperimental and ModelExperimental3DTileContent #9896

Merged
merged 28 commits into from
Oct 29, 2021

Conversation

@cesium-concierge
Copy link

Thanks for the pull request @sanjeetsuhag!

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

@sanjeetsuhag sanjeetsuhag marked this pull request as ready for review October 27, 2021 17:11
@sanjeetsuhag sanjeetsuhag requested a review from ptrgags October 27, 2021 18:23
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.

@sanjeetsuhag the sandcastle works fine. I had some questions and comments about the code

Source/Shaders/ModelExperimental/FeatureStageVS.glsl Outdated Show resolved Hide resolved
Source/Scene/ModelExperimental/FeatureIdPipelineStage.js Outdated Show resolved Hide resolved
Source/Scene/ModelExperimental/buildDrawCommands.js Outdated Show resolved Hide resolved
Source/Scene/ModelExperimental/CPUStylingStage.js Outdated Show resolved Hide resolved
@sanjeetsuhag
Copy link
Contributor Author

@ptrgags Addressed your feedback.

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.

@sanjeetsuhag just one note to prevent exposing private API while this is still experimental

Source/Scene/Cesium3DTileset.js Outdated Show resolved Hide resolved
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.

@sanjeetsuhag looks good to me.

@lilleyse do you want to do a review pass before we merge?

@ptrgags ptrgags requested a review from lilleyse October 28, 2021 15:49
@lilleyse
Copy link
Contributor

@sanjeetsuhag do you have a sandcastle that tests styling on an individual model?

@sanjeetsuhag
Copy link
Contributor Author

sanjeetsuhag commented Oct 28, 2021

@sanjeetsuhag do you have a sandcastle that tests styling on an individual model?

@lilleyse Here you go

Copy link
Contributor

@lilleyse lilleyse left a comment

Choose a reason for hiding this comment

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

Looks much better from the architecture side. The main thing that's missing here, at least from the reviewer side, is sandcastles that more honestly test the behavior of code. What I would have liked to see is a sandcastle that tests styles that are fully opaque, fully translucent, and an opaque/translucent mix with the ability to select them from a dropdown. Also an option for "No style" to see what happens if a style is unset and then later set again. And a dropdown to change the color blend mode. Then I would like to see it working for 3D Tiles with features, individual models with features, 3D Tiles without features, and individual models without features, and some mix of feature id textures and feature id attributes. Without getting too deep into the code myself a sandcastle like that would really help validate that the code is working.

Source/Scene/Cesium3DTileset.js Show resolved Hide resolved
Source/Scene/ModelExperimental/CPUStylingStage.js Outdated Show resolved Hide resolved
Source/Scene/ModelExperimental/FeatureIdPipelineStage.js Outdated Show resolved Hide resolved
Source/Scene/ModelExperimental/ModelFeatureTable.js Outdated Show resolved Hide resolved
Source/Scene/ModelExperimental/buildDrawCommands.js Outdated Show resolved Hide resolved
Source/Scene/ModelExperimental/buildDrawCommands.js Outdated Show resolved Hide resolved
@lilleyse
Copy link
Contributor

The new collection of sandcastle examples is great. I just noticed one issue when picking some of the buildings in 3D Tiles - B3DM Sandcastle example:

glitchy

@sanjeetsuhag
Copy link
Contributor Author

sanjeetsuhag commented Oct 28, 2021

@lilleyse There's two problems happening here:

  1. Dynamic color of features without a style does not trigger the CPUStylingPipelineStage. That's what happens in the inspector - it sets the feature.color. This is why the picking is only working when a style is set.
  2. The "larger" primitives seem to work fine, the smaller ones don't. Still investigating this. The root cause of this was due to the passes not being set correctly when there were both translucent and opaque features in the model.

The fix was to add a _styleCommandsNeeded dirty flag in ModelFeatureTable that keeps track of the changes in the number of translucent features in the batch texture and if the types of commands needed are changed, it sets the flag, which is read by ModelExperimental, which calls resetDrawCommands in the post render event.

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.

@sanjeetsuhag sandcastles are working nicely!

Had a couple last questions for you.

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.

@sanjeetsuhag nearly there, just a couple last things

Source/Scene/Cesium3DTileBatchTable.js Outdated Show resolved Hide resolved
Specs/Scene/ModelExperimental/ModelFeatureTableSpec.js Outdated Show resolved Hide resolved
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.

changes look good, @lilleyse did you want to take a last look or should I merge?

@lilleyse
Copy link
Contributor

@ptrgags if you're happy you can merge

@ptrgags ptrgags merged commit 4f582c4 into main Oct 29, 2021
@ptrgags ptrgags deleted the model-experimental-styling branch October 29, 2021 16:31
@ptrgags
Copy link
Contributor

ptrgags commented Oct 29, 2021

Merged! Thanks @sanjeetsuhag

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.

4 participants