Skip to content
This repository has been archived by the owner on Dec 22, 2023. It is now read-only.

TextureTransformTest "All" box is ambiguous #177

Closed
kebby opened this issue Jun 18, 2018 · 13 comments
Closed

TextureTransformTest "All" box is ambiguous #177

kebby opened this issue Jun 18, 2018 · 13 comments

Comments

@kebby
Copy link

kebby commented Jun 18, 2018

The lower right "All" box of https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/2.0/TextureTransformTest has got a slight problem: It yields a correct result with operation orders translate->scale->rotate as well as translate->rotate->scale. So it's currently not very good at showing if you got the order of operations right.

I'd suggest making the scale non-uniform so you get different results depending on whether the scaling comes before or after the rotation.

@andreasplesch
Copy link

copied over from #225

moved from:

KhronosGroup/glTF-Asset-Generator#531

The current texture transform example does not test for anisotropic scale, applied together with a rotation.

An improved, harder test with a scale of 2.0,1.5 and a rotation of 90 degrees:

https://github.com/andreasplesch/gltf-test/blob/textrafo/tutorialModels/TextureTransformTest/glTF/TextureTransformTest.gltf

Indeed, the texture transform reveals disagreement between Three and Babylon:

Three:
image

Babylon:
image

Please disregard the green check mark. It is deliberately left in a certainly wrong location.

I think Babylon is correct but I may be misunderstanding the transform construction, see discussion in KhronosGroup/glTF-Asset-Generator#531

Until there is resolution as to what is the correct application, the green check mark could be omitted.

@emackey
Copy link
Member

emackey commented Jun 5, 2019

@andreasplesch There's also a spec issue related to this, KhronosGroup/glTF#1563.

@emackey
Copy link
Member

emackey commented Jun 5, 2019

I do hope the BabylonJS implementation is deemed "correct", because we worked hard to make the Blender importer & exporter correspond exactly to that. But, see the counterarguments in the spec issue, the current interpretation doesn't allow non-uniform scaling without shearing the texture image.

But Blender is still in beta, there's still time to switch over to the ThreeJS way of implementing this, if that becomes the standard way.

@andreasplesch
Copy link

Ah, intuitively shearing occurs when scaling happens after rotation. But the extension language spec. says T x R x S which normally means scaling first. So I think that was at the least the intention.

@emackey
Copy link
Member

emackey commented Jun 5, 2019

Textures make this backwards though. The T x R x S ordering prevents the UV points themselves from being sheared when viewed as lines in a UV editor, but this results in the textured image appearing sheared when shown on a 3D model. The opposite order (S x R) makes the image appear non-sheared by allowing the UV coordinates themselves to shear.

@emackey
Copy link
Member

emackey commented Jun 5, 2019

This demo shows what happens if some horizontal scaling is applied from a global axis (emulating S x R). The UV map gets sheared, but the image on the 3D cube does not.

TextureShearing

@emackey
Copy link
Member

emackey commented Jun 5, 2019

Conversely, here the UV map lines do not shear, but the 3D image does.

TextureShearing_v2

@andreasplesch
Copy link

andreasplesch commented Jun 5, 2019

I think that's right. For UV the matrix multiplication order has to be inverted to avoid shearing of the mapped image. (X3D requires the same inverse multiplication order). So I think you are arguing for the spec. language to read S x R x T. (Three and X3D have an additional pivot or center which makes it easier to rotate around around a specific point, for example the center. It has to be applied and then unapplied, but backwards for UV).
This is very unintuitive and only possible to understand by carefully working through examples, and your nice gifs !

@emackey
Copy link
Member

emackey commented Jun 5, 2019

Yeah, actually I should have posted this on the spec issue instead. But anyway it may not be possible to have such a breaking change to the spec on this, it's already marked as complete. I don't know if we have a good process to make updates to released extension specs, short of releasing a new extension with a slightly different name.

@andreasplesch
Copy link

Ok, that is all very helpful. To me, it means that the extension never really considered non-uniform scaling together with rotation, and would require shearing in such a case. I am not sure how many engines actually fully intend to support the extension as it is but in any case it will be difficult to change anything.
In this case, it seems to me while Babylon conforms to the spec.,shearing and all, Three does not and would need to implement a special case for gltf separate from its native tex trafo handling. So far, Three did not do that and may never.
If Three does not do that, it will be very useful to add a note to the spec. language that non-uniform scaling along with rotation is not expected to be widely supported.
Finally, I am unsure how non-uniform scaling and just offset without rotation behaves.

@andreasplesch
Copy link

A scale of 2.0,1.5 and an offset of -1,-0.5 works as expected and results in the same UVs for both Babylon and Three:
U from (0 x 2.0)-1.0=-1.0 to (1 x 2.0)-1.0=1.0
V from (0 x 1.5)-0.5=-0.5 to (1 x 1.5)-0.5=1.0
So offset is applied last, as specified. Again, a negative offset has the effect of shifting the image towards the positive UV corner, in the inverted UV world.

@DRx3D
Copy link
Contributor

DRx3D commented Nov 2, 2023

@emackey : Does this issue need to remain open? If so, it needs to be transferred to glTF-Sample-Assets repo. The default is to close it by 27 Nov.,

@DRx3D
Copy link
Contributor

DRx3D commented Dec 3, 2023

[Fixed typo 5+ --> 4+]

No comments in 4+ years. Closing.

@DRx3D DRx3D closed this as not planned Won't fix, can't repro, duplicate, stale Dec 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants