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

Clarify descriptions of rotation direction in KHR_texture_transform #1624

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

emackey
Copy link
Member

@emackey emackey commented Jun 6, 2019

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
Copy link
Member Author

emackey commented Jun 6, 2019

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

@andreasplesch
Copy link
Contributor

andreasplesch commented Jun 6, 2019

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.

@emackey
Copy link
Member Author

emackey commented Jun 7, 2019

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.

@andreasplesch
Copy link
Contributor

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.

@scurest
Copy link

scurest commented Jun 7, 2019

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

@emackey
Copy link
Member Author

emackey commented Jun 7, 2019

Thanks @scurest and @andreasplesch.

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

@lexaknyazev
Copy link
Member

+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?

@emackey
Copy link
Member Author

emackey commented Jun 11, 2019

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)

@scurest
Copy link

scurest commented Nov 10, 2019

Ping.

@donmccurdy
Copy link
Contributor

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

@emackey
Copy link
Member Author

emackey commented Oct 3, 2022

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 emackey requested a review from lexaknyazev October 3, 2022 14:34
0b5vr added a commit to 0b5vr/vrm-specification that referenced this pull request Oct 4, 2022
We might want to wait until the merge of the PR on the KHR_texture_transform side.
I still have low confidence about the wording

KhronosGroup/glTF#1624
0b5vr added a commit to pixiv/three-vrm that referenced this pull request Oct 4, 2022
The rotation direction is in which rotates UV coordinates counter-clockwise (in U-right, V-down space)

Ref: KhronosGroup/glTF#1624
@TokageItLab
Copy link

TokageItLab commented Nov 4, 2022

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?

@emackey
Copy link
Member Author

emackey commented Nov 4, 2022

@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.

@TokageItLab
Copy link

TokageItLab commented Nov 4, 2022

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.

@emackey
Copy link
Member Author

emackey commented Nov 7, 2022

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

@0b5vr
Copy link

0b5vr commented Nov 24, 2022

@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}$$

@emackey
Copy link
Member Author

emackey commented Jan 12, 2023

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.

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

Successfully merging this pull request may close these issues.

KHR_texture_transform has the wrong rotation matrix
8 participants