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

Inconsistent behaviour when non-existing UV indices are accessed #27552

Closed
hybridherbst opened this issue Jan 12, 2024 · 8 comments · Fixed by #27608
Closed

Inconsistent behaviour when non-existing UV indices are accessed #27552

hybridherbst opened this issue Jan 12, 2024 · 8 comments · Fixed by #27608

Comments

@hybridherbst
Copy link
Contributor

hybridherbst commented Jan 12, 2024

Description

Currently, when a material says "I need UV2" and the geometry doesn't have it, rendering just stops with shader errors. This can happen manually or when loading glTF files that are strictly speaking invalid (validator complains) but are nonetheless produced by applications because it's better for interoperability to have one material than having to duplicate it.

I did a few tests with various viewers and the behaviour is slighty different, but the files attached below "mostly work" elsewhere: KhronosGroup/glTF#2360

The behaviour is inconsistent right now, sometimes accessing missing UV index data works (UVMismatch_UV1 below) and sometimes it fails (UVMismatch_UV2 below). Maybe some place in the code already sanitizes specifically UV1, but not UV2?

Reproduction steps

  1. Load this file in https://threejs.org/editor or https://modelviewer.dev/editor:
    UVMismatch_UV2.glb.zip

  2. Note shader errors in the console (UV2 is accessed, both objects don't have UV2)

  3. Load this alternative file:
    UVMismatch_UV1.glb.zip

  4. Note file renders for some reason (UV1 is accessed, one object doesn't have UV1)

  5. This file also breaks with just UV1 missing: LOUVRES.005.glb.zip

Version

r161

@donmccurdy
Copy link
Collaborator

donmccurdy commented Jan 12, 2024

Personally I'm grateful for the glTF specification's strictness in this area. There are already a handful of cases where we need to 'clone' one glTF material into multiple three.js materials, and those cases have been recurring sources of challenges and bugs. I'd be sad to see the glTF specification encourage more of that, and I'm not sure I agree that the popular use of glTF for interchange requires these changes, or justifies the added cost on runtimes.

By way of example, see:

This represents a number of long-standing issues in both React Three Fiber and Threlte, for which the only solution I've come up with is to pre-process the glTF files and ensure that materials are duplicated in the source glTF file for every case where THREE.GLTFLoader might otherwise need to clone that material.


All of that said ... I don't necessarily feel strongly about what three.js should do given material.map.channel = 2 on a mesh with fewer UVs. If we'd like to fall back to another UV set, rather than throw an error, I have no objection!

@hybridherbst
Copy link
Contributor Author

hybridherbst commented Jan 12, 2024

While I personally think a runtime should always try to render something, I understand that may not be in scope for three.js. That's why I labeled the issue with "inconsistent", not "broken/wrong" – I'm not sure why having some objects that have that UV attribute and some that don't have it results in a working scene, while when all objects don't have it it breaks.

Requiring duplication in the source glTF is very much the opposite of interchangeability :) may be one more case where a more strict spec separation between "using glTF for pipelines" (where names can and should occur multiple times, materials can be reused, ...) and "using glTF for strict GPU-friendly delivery" (where names may need to be unique and in extreme cases every object has a unique material) would have been good...

I think: we want three.js to fall back to an existing UV set or to 00, to align with how other renderers handle missing UV attributes.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 15, 2024

Note file renders for some reason (UV1 is accessed, one object doesn't have UV1)

To recap: UVMismatch_UV1.glb contains two meshes sharing the same material. The material holds a texture with channel 1. The first mesh has geometry data with two uv attributes. This one is rendered first so the shader has USE_UV1 defined which means the uv1 attribute is available. The second mesh shares the same shader but without defining data for uv1 so the rendering is broken (but without a shader compilation error).

That is not true for UVMismatch_UV2.glb. No one of the meshes defines uv2 so the shader compilation fails.

Maybe some place in the code already sanitizes specifically UV1, but not UV2?

No sanitization happens in the renderer. That UVMismatch_UV1.glb does not produce a shader compilation error is a coincidence. With a different render order, UVMismatch_UV1.glb fails as well.

The thing that should be fixed is to ensure UVMismatch_UV1.glb fails the same way as UVMismatch_UV2.glb. However, this is expensive since since we have to add additional checks in the renderer to detected mismatching uv-indices. We could add something like this in setProgram():

} else if ( materialProperties.vertexUv1s !== vertexUv1s ) {

	needsProgramChange = true;

} else if ( materialProperties.vertexUv2s !== vertexUv2s ) {

	needsProgramChange = true;

} else if ( materialProperties.vertexUv3s !== vertexUv3s ) {

	needsProgramChange = true;

}

However, this check is too strict since difference sets of uv attributes do not necessarily require a different shader. The question is whether textures actually use specific uv attributes which are not present in all geometries. Testing for this is complicated and we have to do it for all render items each frame.

For now, I would prefer to not add any changes to the renderer. As long as assets are defined in a glTF compliant way, everything should work as expected.

but are nonetheless produced by applications because it's better for interoperability to have one material than having to duplicate it.

TBH, this is a bad pattern and should be reported as a bug at the respective glTF exporter. What is the purpose of having glTF validation if exporters do not honor it? Sorry, but not even optimization purposes justify the production of invalid assets. This is just causing issues everywhere in the ecosystem.

@hybridherbst
Copy link
Contributor Author

hybridherbst commented Jan 15, 2024

TBH, this is a bad pattern and should be reported as a bug at the respective glTF exporter.

I generally agree but if enough users/renderers do it, it should also bubble back up to Khronos to point out flaws in the spec, which I did (referenced in the original post). The runtime behaviour in three.js is the outlier right now.

@Mugen87 Mugen87 closed this as not planned Won't fix, can't repro, duplicate, stale Jan 19, 2024
@hybridherbst
Copy link
Contributor Author

hybridherbst commented Jan 19, 2024

@Mugen87 I don't think there was a real resolution here...

I also want to note that this will become more common with wider MaterialX / NodeMaterial usage – somehow "this node needs UV2 but it doesn't exist" should be handled or material interchange won't work. MaterialX has fallbacks for missing attributes.

@Mugen87 Mugen87 reopened this Jan 19, 2024
@donmccurdy
Copy link
Collaborator

donmccurdy commented Jan 19, 2024

MaterialX has fallbacks for missing attributes.

What is that fallback mechanism?

In terms of the glTF proposal — I'm really not sure about falling back to uv - 1, it doesn't feel to me like a solution for interchange. Using uv - 1 feels arbitrary (why not uv = 0?) where an explicit mapping would be more appropriate. If an interchange format needed the ability to reuse materials with different UV assignments, perhaps the UV assignments should not be part of the material definition, but e.g. an extension to the primitive?

While perhaps Blender can load files with missing UVs, it does (to my understanding) still require duplicating materials to assign distinct UV channels to different textures arbitrarily. I'm not really on board with characterizing three.js as an "outlier" here — different implementations fail in different ways, which is a strong argument for why glTF prohibits the case.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Jan 19, 2024

If we do want to change this behavior in three.js, I think the two options would be —

  • (A) WebGLRenderer should detect when a texture requires a UV channel the geometry doesn't have and fall back the previous UV channel
  • (B) GLTFLoader should detect this case, clone the material and its textures, and fix the UV assignments

In both cases, I worry that we'd be hiding user error more often than providing a useful behavior. 😕

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 20, 2024

I've figured out a small change that produces the same visual result like BabylonJS (tested with UVMismatch.glb and UVMismatch_UV2.glb).

The shader attributes uv1, uv2 and uv3 are now defined based on the used texture channels, not on the available geometry attributes. In this way, there are no shader compilation errors since the attributes are defined but just not feed with data. uv1, uv2 and uv3 now behave similar to the default uv attribute.

Mugen87@ce92e7e

three.js:

image image

BabylonJS

image image

Can we consider this as an improvement?

If so, I'll file a PR with the change.

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

Successfully merging a pull request may close this issue.

3 participants