-
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
A proposed implementation for passing imagery layers data to Fragment Shaders #10187
base: main
Are you sure you want to change the base?
Conversation
… globe material vertex shader for further processing. A current limitation is the way the layers are being iterated in GlobeFS.glsl before setting values to the czm_materialInput->layerColor array. This induces long shader compilation times and requires to set a fixed number of processed layers (set as the GLSL constant layerColorNumber)
Thanks for the pull request @xtassin!
Reviewers, don't forget to make sure that:
|
#ifdef APPLY_MATERIAL | ||
if (textureAlpha > 0.0) { | ||
// itterate over the layerColor array to check for matching layerIndex and send color value to materialInput | ||
//TODO: Find a different way to set pass layerColor values. Here, only the first two layers are handled as this loop leads to performance problems (shaders compilation time) | ||
for (int i = 0; i < czm_layerColorNumber; i++) { | ||
if (i == layerIndex) {materialInput.layerColor[i] = value; break;} | ||
} | ||
} | ||
#endif |
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.
It's unfortunate that czm_layerColorNumber
would be capped to the first two layers due to slow compilation.
I can't think of a good way to avoid this in WebGL 1 unless layerIndex
were somehow made to be a constant expression, and that seems tricky since GlobeSurfaceShaderSet.prototype.getShaderProgram
doesn't have access to the imagery layers.
Since WebGL2 allows dynamically indexed arrays this would simply be materialInput.layerColor[layerIndex] = value
. Another reason to switch to WebGL 2 soon.
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.
Thanks for the feedback @lilleyse. This pull request is just me thinking out loud about this feature and how best to implement it. In my use case, only one layer is made available to the shader. To work around the limitation of a hard coded number of layers, could this number be set by the user in the form of a "define"?
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.
To work around the limitation of a hard coded number of layers, could this number be set by the user in the form of a "define"?
Possibly. I considered that as well. The only problem is that materialInput.glsl
is used by other systems that don't have access to the define — though they could mock the define and set it to 1.
I'm not sure about the exact requirements of your application, but would it be acceptable for the material to have a single color after all the imagery layers have been resolved rather than an array of layer colors? That would simplify things quite a bit.
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.
My application needs the color of one specific layer, so using the resulting mix of all layers will not be acceptable. In any case, this pull request aims at finding a general solution that can be useful to Cesium users at large. I will investigate a "define" based approach to the problem and post results in the PR.
@@ -25,4 +25,5 @@ struct czm_materialInput | |||
float height; | |||
float slope; | |||
float aspect; | |||
vec4 layerColor[czm_layerColorNumber]; |
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.
Just confirming - to check that a layer exists would the material have code like materialInput.layerColor[index] != vec4(0.0)
?
layerColor
should be added to the documentation above.
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.
This can be a way to test it, indeed. If ever the number of layers passed to the material could be set by the user, it would be more obvious which layer colors are made available to the material.
@@ -25,4 +25,5 @@ struct czm_materialInput | |||
float height; | |||
float slope; | |||
float aspect; | |||
vec4 layerColor[czm_layerColorNumber]; |
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.
Should there be a layerColorNumber.glsl
file? I had to create one locally to test this. Also a sandcastle demo would be helpful. I modified the Globe Materials sandcastle.
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, I forgot to check in layerColorNumber.glsl
I would like to find the best way to implement this and make sure it just makes sense at all before writing any demo. This is why your input is very valuable here.
Thanks again for your contribution @xtassin! No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy? I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with |
I haven't given up on this feature and am still in the process of evaluating performance from a live implementation. Handling the number of layers dynamically is still very much an issue that need to be addressed in a acceptable manner. |
Thanks again for your contribution @xtassin! No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy? I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with |
4 similar comments
Thanks again for your contribution @xtassin! No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy? I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with |
Thanks again for your contribution @xtassin! No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy? I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with |
Thanks again for your contribution @xtassin! No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy? I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with |
Thanks again for your contribution @xtassin! No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy? I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with |
A proposed implementation for passing imagery layers raster data to a globe material vertex shader for further processing. A current limitation is the way the layers are being iterated in GlobeFS.glsl before setting values to the czm_materialInput->layerColor array. This induces long shader compilation times and requires to set a fixed number of processed layers (set as the GLSL constant layerColorNumber)