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

A proposed implementation for passing imagery layers data to Fragment Shaders #10187

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

xtassin
Copy link
Contributor

@xtassin xtassin commented Mar 10, 2022

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)

… 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)
@cesium-concierge
Copy link

Thanks for the pull request @xtassin!

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

@ggetz
Copy link
Contributor

ggetz commented Mar 14, 2022

Thanks @xtassin!

@lilleyse Would you be able to review the initial approach here before we get into the technicalities?

Comment on lines +207 to +215
#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
Copy link
Contributor

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.

Copy link
Contributor Author

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"?

Copy link
Contributor

@lilleyse lilleyse Mar 15, 2022

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.

Copy link
Contributor Author

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];
Copy link
Contributor

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.

Copy link
Contributor Author

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];
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@cesium-concierge
Copy link

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 @cesium-concierge stop. If you want me to start again, just delete the comment.

@xtassin
Copy link
Contributor Author

xtassin commented Jun 13, 2022

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.

@cesium-concierge
Copy link

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 @cesium-concierge stop. If you want me to start again, just delete the comment.

4 similar comments
@cesium-concierge
Copy link

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 @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

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 @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

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 @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

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 @cesium-concierge stop. If you want me to start again, just delete the comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Blocked
Development

Successfully merging this pull request may close these issues.

None yet

4 participants