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

Add base color names option #637

Merged
merged 3 commits into from
Apr 20, 2023

Conversation

javagl
Copy link
Contributor

@javagl javagl commented Apr 19, 2023

Adds an option to specify the names of base color texture and factor uniforms when updating glTF with KHR_techniques_webgl extension to PBR.

By default, gltf-pipeline will try to convert materials that use the KHR_techniques_webgl extension into PBR materials. This is not possible generically, but it is possible to make some "educated guesses". Specifically, it is possible to guess which texture is supposed to be the PBR baseColorTexture by looking at the name of the uniform in the techniques definition: When the uniform is called u_diffuse (and refers to a texture), then it is very likely that this texture is supposed to become the baseColorTexture.

The set of names that suggest that a texture may become a base color texture is ... somewhat arbitrary. One can never know which names one might encounter in the wild.

This PR adds an option to specify the base color texture names at the command line and pass them to the update function in the options object. For example, when a GLB contains techniques where the uniform names are u_my_diffuse_tex and u_another_diffuse_tex, then the line

node ./bin/gltf-pipeline.js -i input.glb -o output.glb --baseColorTextureNames u_my_diffuse_tex u_another_diffuse_tex

will cause these names to be treated as indicators for the textures to become baseColorTexture.

(All this applies to the base color factors as well, with baseColorFactors as the command line option)


Things to consider:

The README marks these arguments as 'optional', but explicitly says that the set of "default names" for this is not specified. I'd hesitate to explicitly list things like "u_tex", "u_diffuse", "u_emission", "u_diffuse_tex" there...

The current approach is somewhat "fail silent" in nature. When someone has a GLB where such a name is u_diff_tex, then this texture will just be removed, and it may not be obvious for the user why this happened. Loading the model in a viewer might give the impression that the viewer can not display the texture for some reason (with that 'reason' being the too obvious one: It's not there...).

I considered to bring a few more high-level guesses into that conversion, and/or add some sort of warning or hint when something looks odd. For example:

  • If the technique values contains only a single texture reference
  • And if this texture reference does not have one of the known names
  • And (maybe:) If this texture becomes unused when the technique is omitted
  • Then print a warning:

    Warning: Omitting texture that is used via uniform u_diff_t, because it is not one of the known diffuse texture names. See (link to readme) for details.

I think that could help to avoid some confusion for users, some support requests, and save some debugging time...

Adds an option to specify the names of base color
texture and factor uniforms when updating glTF
with KHR_techniques_webgl extension to PBR
@lilleyse
Copy link
Contributor

This looks good. If this keeps coming up it may be worth doing the logging you described.

@javagl
Copy link
Contributor Author

javagl commented Apr 19, 2023

If this keeps coming up it may be worth doing the logging

One could expect the set of predefined names to "converge" towards a set that covers "most" assets that are out there (for example, I added u_diffuse_tex and u_diffuse_mat to the defaults in this PR). But I expect some people to try this directly (or indirectly, via the upgrade in the 3d-tiles-tools) and be suprised by the missing textures.

@lilleyse lilleyse merged commit 575bd6b into CesiumGS:main Apr 20, 2023
mramato added a commit to CesiumGS/cesium that referenced this pull request Jun 3, 2023
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.

2 participants