-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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 water mask material #12149
Add water mask material #12149
Conversation
Thank you for the pull request, @ggetz! ✅ We can confirm we have a CLA on file for you. |
@jjspace A reminder to review this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay getting back to this. As far as I can tell (with my more limited knowledge of shaders/materials) all the code looks good. I was also able to test and the sandcastles work as expected.
I think the only thing I might add is that it could be nice to demonstrate the testing plan you outlined in the new sandcastle example. Just a simple material picker like this for people (like me) that are less experienced with our Material api to know how to do that.
Thanks @jjspace!
I would generally agree with this feedback. However, for this particular example, I think that distracts from the intended use case and make things a bit more complicated in the code example, which I'd like to avoid if possible. |
(hasExaggeration << 29) | | ||
(showUndergroundColor << 30) | | ||
(translucent << 31) | | ||
(applyDayNightAlpha << 32); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(applyDayNightAlpha << 32)
will return 0 or 1, which conflicts with sceneMode
and may introduce potential bugs, see #12260
Description
Adds a new water mask globe material.
The motivation is so that custom globe materials can be written that shade only the land or only the sea, as demonstrated in the new Sandcastle example.
In order to do this, I separated the concerns of having a water mask from displaying the water effect in
packages/engine/Source/Scene/GlobeSurfaceShaderSet.js
, and added the new material shader.Issue number and link
N/A
Testing plan
customElevationMaterial
(such as by using thewaterMaskMaterial.diffuse
value) to see how the mask interacts with the diffuse channel.Author checklist
CONTRIBUTORS.md
CHANGES.md
with a short summary of my change