KHR_texture_transform has the wrong rotation matrix

[scurest]

See KhronosGroup/glTF-Blender-IO#291 (comment)

The rotation matrix in the GLSL listing is

mat3 rotation = mat3(
    cos(Rotation), sin(Rotation), 0,
   -sin(Rotation), cos(Rotation), 0,
                0,             0, 1
);

(Note that GLSL is column-major.) This is a counter-clockwise rotation in the standard X-Right Y-Up coordinate space, but not in glTF's UV coordinate space where Y is down. The correct matrix is

ie. the same as a clockwise rotation in a Y-Up space.

[lexaknyazev]

@donmccurdy
Is it an error in the extension sample code?

[emackey]

FWIW, I'm still seeing some difference between Babylon's handling and ThreeJS's handling of this extension, particularly when all three channels (translate, rotate, scale) are in use at once. I suspect ThreeJS has different order there.

The Blender importer & exporter, and its unit tests, are in perfect alignment with Babylon's implementation, to the best of my ability to test.

[donmccurdy]

^Right, see mrdoob/three.js#15831. I think the three.js implementation needs a fix.

[emackey]

Interesting that WestLangley points out shearing on that thread. The ThreeJS implementation avoids texture shearing, but KHR_texture_transform (in Babylon and Blender) cause shearing when both rotation and non-uniform scaling are used together.

Blender's "Mapping" node does have a mode (called Texture) that avoids texture shearing, and the glTF extension is unable to reproduce non-uniform scaling from that mode as a result. There was discussed and even documented when the PR to fix the Blender exporter UV transform was under review. There's a note in the documentation saying you can't use non-uniform scaling in Texture transform mode, which is the only one that avoids shearing rotations, and thus can't be expressed correctly in glTF.

Probably too late to change the definition of the glTF extension, of course, but I can see why ThreeJS would be reluctant to switch to our definition of it.

[emackey]

On the left, Point mode in Blender, recommended as it's closest to what the KHR_texture_transform actually does (but the actual exported numbers still differ from what's shown on the UI). Rotation with non-uniform scaling leads to shearing.

On the right, Texture mode in Blender. This cannot export correctly to KHR_texture_transform, and the documentation for the glTF addon warns users not to use non-uniform scaling in this mode for this reason. But you can see the texture has been rotated and scaled without shearing: The grid lines are all at right angles to each other.

[andreasplesch]

Just link to the actual spec. language for reference: https://github.com/KhronosGroup/glTF/blob/master/extensions/2.0/Khronos/KHR_texture_transform/README.md

[emackey]

@andreasplesch Given your investigations into this today, would you agree that the sample GLSL code in the README here is incorrect, as described in the original post on this issue? Specifically, that sin(r) and -sin(r) are swapped in the sample GLSL snippet?

[emackey]

Hmm. For what it's worth, Cesium appears to have implemented this feature directly from the sample code, and is able to pass the sample test model rotation using the GLSL matrix as-is.

[andreasplesch]

@emackey First I would note that the original post got side tracked with the concerns about shearing which are unrelated, as far as I can see.

The spec. language defines ( in the description which is probably normative ) rotation as counterclockwise rotation of UV coordinates. But the spec. does not (late edit) say in which space, standard space, or UV space. I think a reasonable assumption is that the rotation is intended to be applied in standard space (this is what I did at first, by default) which then results in a clockwise rotation in UV space. As the spec. code snippet does this, it confirms that using standard space for the rotation is the intention.

I do not know if using standard space is correct or incorrect but it does seem to be what was intended. Only the spec. authors would know.

Beware late edits above, sorry about that.

[andreasplesch]

One complication is that engine implementations seem to already do what the original post suggests, often by inverting the sign of the rotation angle.

Cesium does this:

https://github.com/AnalyticalGraphicsInc/cesium/blob/c2362b89a5cbc459589c619ddc3e8dd67e60fe1a/Source/Scene/processPbrMaterials.js#L556

And I think Babylon does this as well as reverse engineering from examples would suggest (need to find the code).

This would make Babylon and Cesium actually non-conforming and probably the test scene actually misleading (KhronosGroup/glTF-Sample-Models#177 for more discussion on the test scene).

In other words, if the test scene rotation's green check mark is considered the 'correct' result then the original post is right in proposing a change in the spec.

[emackey]

Ah, I missed that line of Cesium code, thanks for pointing that out.

So in the glTF group, once there are multiple, concrete implementations of a spec, if some ambiguity is found in the wording of the spec, we prefer to clarify the spec to match what's been implemented, where possible. This isn't considered a violation of the spec, just that wherever the spec left some ambiguity that was uniformly interpreted the same way by its own early adopters, then we prefer to clarify the spec wording to match, without breaking those released pieces of software.

In this case, if it's just the GLSL sample code in the spec's README that's flipped, then we should correct the bug in the sample GLSL snippet, and further clarify the wording of the spec's normative text, to indicate which rotational sense we are talking about.

From personal testing, I believe BabylonJS, ThreeJS, CesiumJS, and Blender-IO are all in agreement on rotation, along with the existing sample model, as to which direction the rotation happens. So with that in mind, it seems clear that the README for the extension should be clarified to match.

[andreasplesch]

That makes a lot of sense.

In addition to fixing the sample code and adding a note that this is flipped from standard rotation, another clarification then would be to explain better the sense of rotation, in some way.

It would be also helpful to add, perhaps as an implementor note, that inverting the sign of the rotation angle and then using a standard rotation matrix is an alternative way of implementation since many engines may want to use a standard rotation matrix.