search

KhronosGroup / glTF-Blender-IO

Blender glTF 2.0 importer and exporter
https://docs.blender.org/manual/en/latest/addons/import_export/scene_gltf2.html
Apache License 2.0
1.5k stars 319 forks source link

KHR_texture_transform with rotation possibily wrong #2366

Closed timknip closed 2 months ago

timknip commented 2 months ago

Describe the bug A glTF file with the KHR_texture_transform extension looks good in some (ugh) online viewers but looks wrong after importing into blender. Viewers showing ok:

So basically the question is who's got it right.

To Reproduce Steps to reproduce the behavior:

  1. import this file xxxx49519510811a3a8e7b686fcfb4152d0ff26f_09.COLOR_MORSECHARCOAL.zip
  2. look at the texture which is wrongly applied: bad

Expected behavior The texture should look good: good

Version

I suspect the conversion to the Mapping node can be wrong in some cases.

Due to inconsistent behavior in different viewers, it's unclear which implementation is correct.

scurest commented 2 months ago

https://gltf-viewer.donmccurdy.com/ is known to have a wrong implementation of KHR_texture_transform, see https://github.com/mrdoob/three.js/issues/15831 (note that despite being closed, the issue was never fixed). Since Babylon and Blender agree, they seem likely to be correct.

In practical terms, you should probably avoid rotations since they never received a consistent implementation.

julienduroure commented 2 months ago

@donmccurdy Do you have any inputs about that? Thanks!

donmccurdy commented 2 months ago

In practical terms, you should probably avoid rotations since they never received a consistent implementation.

If I understand correctly, the problem described by https://github.com/mrdoob/three.js/issues/15831 occurs only if the UV transform contains both:

  1. non-uniform scaling
  2. non-zero rotation

If scaling is uniform, or there's no rotation, then Scale[s] x Rot[-r] = Rot[-r] x Scale[s] should be equivalent. Does that sound right?

I agree that three.js does not implement KHR_texture_transform according to the spec when both conditions above are true, but I don't expect that to change any time soon. It would be a breaking change that affects more than just the three.js glTF loader, and I worry that the fix may be worse than the bug for us, given the shear problem. Perhaps three.js could log a warning when a glTF UV transform uses both rotation and non-uniform scale, saying this is not supported.

julienduroure commented 2 months ago

Hello, Thanks for your feedback. I think we can close this ticket, as there is no bug on Blender side