glTF icon indicating copy to clipboard operation
glTF copied to clipboard

Clarify descriptions of rotation direction in KHR_texture_transform

Open emackey opened this issue 5 years ago • 11 comments

I've tried to clarify the wording about what "counter-clockwise" means in a space where the vertical axis points down instead of up.

Fixes #1563, see detailed discussions there and in linked issues.

/cc @andreasplesch @scurest

emackey avatar Jun 06 '19 18:06 emackey

My hope is that this brings the spec un-ambiguously into alignment with the existing implementations and sample model.

emackey avatar Jun 06 '19 18:06 emackey

I cannot come up with a better explanation of 'counter-clockwise' in U-right V-down space which stays concise. But I fear that saying 'counterclockwise' when it actually looks like 'clockwise' when you look at a drawing may generate confusion also. The other fear is that implementers may start to update their code with the updated snippet but forget that now the non-negated rotation angle has to be used. But this is covered by the test scene. Until somebody has better language, this certainly does the job.

andreasplesch avatar Jun 06 '19 21:06 andreasplesch

I fear that saying 'counterclockwise' when it actually looks like 'clockwise' when you look at a drawing may generate confusion

It does look 'counter-clockwise' when viewed as a UV map, as opposed to being viewed on a 3D model. I don't think the spec needs to explain why the two rotational directions are different, just acknowledge that they are and identify which one is counter-clockwise (the UV one).

Because the UV map defines what parts of an image a given polygon may sample, it ends up acting a little bit like a camera. If a 3D camera is looking at only one model, and the camera moves or rotates in a direction, a viewer watching the rendered movie may think they're seeing the model move or rotate in the opposite direction.

That's actually not why the rotation matrix is non-standard. The non-standard matrix comes from the vertical axis starting at the top of the image and flowing down, instead of starting at the bottom of the image and flowing up. Similar to flipping the handed-ness of a coordinate system, this causes the rotation math to flip. But if you had a glTF-enabled UV editor, showing the UV map lines on a right-side-up image the normal way, it would appear as a counter-clockwise rotation on the UV map editor.

The other fear is that implementers may start to update their code

We can just tell existing implementations that they don't need any change. If the sample model works, it's good.

emackey avatar Jun 07 '19 14:06 emackey

Yeah, it does look counterclockwise on a drawing with x-right, y-down. You are right, the spec. is not the place to explain how the final image on the model is rotated. To me, the simplest, non-ambiguous way is just to specify the transform matrix calculation as m = T x R(-angle) x S, and omit implementation details. But this may be considered an actual change in the spec., rather than a clarification.

andreasplesch avatar Jun 07 '19 14:06 andreasplesch

Lgtm. I think it's fine as is, but if you want to further clarify you can always include a picture.

scurest avatar Jun 07 '19 20:06 scurest

Thanks @scurest and @andreasplesch.

@lexaknyazev I know your plate is full, but do you have time to look at this too? No rush.

emackey avatar Jun 07 '19 21:06 emackey

+1 on including a picture.

non-uniform texture scaling is not supported by all clients that implement this extension

What's the intent of this note? Should all new implementations also support only uniform scaling?

lexaknyazev avatar Jun 11 '19 17:06 lexaknyazev

What's the intent of this note? Should all new implementations also support only uniform scaling?

Perhaps this needs rewording. The problem is that this extension can't express both rotation and non-uniform scaling at the same time without shearing the texture image on the final 3D model. Frustratingly, the extension spec is worded as if to prevent shearing via an affine transformation, but this only protects lines on the UV map itself from shearing. The actual image on the 3D model will become sheared if the UV math is followed as specified here, which is why ThreeJS has declined to bring their implementation into alignment with this spec. (see thread)

emackey avatar Jun 11 '19 17:06 emackey

Ping.

scurest avatar Nov 10 '19 03:11 scurest

This is a fairly important correction to the KHR_texture_transform extension I think, it would be great to get this merged!

donmccurdy avatar Oct 03 '22 13:10 donmccurdy

I've fixed the merge conflicts and cleaned this up a bit. I do not have time to design and create a full diagram to add to this spec, and I don't think it's required here.

I think this is ready to go now. At least one approval (other than me) is required.

emackey avatar Oct 03 '22 14:10 emackey

~~Wouldn't this definition cause the appearance to change depending on the platform? If glTF is based on OpenGL, wouldn't it be better to define it to be U-Right V-Up on all platforms?~~ I was forgotten that the textures were flipped internally in OpenGL.

I think that this should correct about offset as well as rotation. When offset U is positive, should the texture be moved to the right and when offset V is positive, should the texture be moved down?

TokageItLab avatar Nov 04 '22 03:11 TokageItLab

@TokageItLab It's tricky to communicate because moving the texture coordinates is the opposite of moving the texture. For example, if all of the UV coordinates slide towards the right side of an image, a user watching the render output will see the texture image on the polygon sliding to the left. Likewise for clockwise vs. counter-clockwise rotations.

We do have test models to get everyone on the same page, and those have not changed since this extension was first introduced (and will not change as a result of this PR's clarifications).

  • TextureTransformTest - This model tests translation, rotation, and uniform scaling all on the base color channel, individually and combined.
  • TextureTransformMultiTest - This model tests combined T*R*S across all of the core channels and clearcoat channels.

emackey avatar Nov 04 '22 12:11 emackey

I prefer this sentence.

(in U-right, V-down space)

So for more clarification, I would like this sentence to apply to the entire KHR_texture_transform, not just the rotation only.

TokageItLab avatar Nov 04 '22 13:11 TokageItLab

I've added a note that these transformations are applied to the texture coordinates as supplied by glTF, not to device-local coordinates.

emackey avatar Nov 07 '22 14:11 emackey

@emackey How is going with this change?

My coworker recommends that showing a matrix on the specification is very useful to understand the spec correctly. How about showing like this? (needs a strong proofreading though):

\begin{align}
& \bf T = \begin{bmatrix} 1 & 0 & {\rm Offset}.x \\ 0 & 1 & {\rm Offset}.y \\ 0 & 0 & 1 \end{bmatrix} \\
& \bf R = \begin{bmatrix} \cos({\rm Rotation}) & \sin({\rm Rotation}) & 0 \\ -\sin({\rm Rotation}) & \cos({\rm Rotation}) & 0 \\ 0 & 0 & 1 \end{bmatrix} \\
& \bf S = \begin{bmatrix} {\rm Scale}.x & 0 & 0 \\ 0 & {\rm Scale}.y & 0 \\ 0 & 0 & 1 \end{bmatrix} \\
\\
& \begin{pmatrix} {\rm uv}'.x \\ {\rm uv}'.y \\ 1 \end{pmatrix}
= \bf T \bf R \bf S 
\begin{pmatrix} {\rm uv}.x \\ {\rm uv}.y \\ 1 \end{pmatrix}
\end{align}

0b5vr avatar Nov 24 '22 09:11 0b5vr

I'm not planning to make additional changes to this branch. Maintainers, please either merge this or close this. If there are additional concerns, they can be opened as a new issue, to be addressed in a new PR.

emackey avatar Jan 12 '23 20:01 emackey