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.

Sign up for GitHub

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

Jump to bottom

Enables coloring of ModelExperimental #9881

Merged
merged 14 commits into from
Oct 22, 2021
Merged

Enables coloring of ModelExperimental #9881

merged 14 commits into from
Oct 22, 2021

Conversation

sanjeetsuhag
Copy link
Contributor

@sanjeetsuhag sanjeetsuhag commented Oct 20, 2021 •
edited
Loading

Adds color, colorBlendAmount and colorBlendMode to ModelExperimental.

Sandcastle for setting color, colorBlendAmount, colorBlendMode on ModelExperimental

Depends on #9873 being merged first.

@cesium-concierge
Copy link

Thanks for the pull request @sanjeetsuhag!

  • ✔️ Signed CLA found.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.

ptrgags
ptrgags reviewed Oct 20, 2021
View reviewed changes
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 pretty solid overall, though it doesn't seem to work with instancing or the TriangleWithoutIndices? is this supposed to happen?

image

Source/Scene/ModelExperimental/ModelColorStage.js Outdated Show resolved Hide resolved
Base automatically changed from model-experimental-content to main October 21, 2021 23:48
@sanjeetsuhag
Copy link
Contributor Author

it doesn't seem to work with instancing or the TriangleWithoutIndices? is this supposed to happen?

@ptrgags and I compared with the Model.js implementation and discovered that this problem was happening due to the lighting stage being applied after the highlight.

image

@sanjeetsuhag sanjeetsuhag marked this pull request as ready for review October 22, 2021 17:49
@sanjeetsuhag
Copy link
Contributor Author

Updated Sandcastle for testing - includes color with alpha

ptrgags
ptrgags reviewed Oct 22, 2021
View reviewed changes
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 really close, though now I think we need to double check the REPLACE color mode.

Source/Shaders/ModelExperimental/MaterialStageFS.glsl Outdated Show resolved Hide resolved
@ptrgags
Copy link
Contributor

ptrgags commented Oct 22, 2021

@sanjeetsuhag looks good, merging!

@ptrgags ptrgags merged commit 251e279 into main Oct 22, 2021
@ptrgags ptrgags deleted the model-experimental-color branch October 22, 2021 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ptrgags ptrgags ptrgags left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sanjeetsuhag @cesium-concierge @ptrgags