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

Texture transform should be referenced by the material texture #1422

Open
UX3D-nopper opened this issue Aug 13, 2018 · 8 comments
Open

Texture transform should be referenced by the material texture #1422

UX3D-nopper opened this issue Aug 13, 2018 · 8 comments
Labels
needs discussion Issue or PR requires working group discussion to resolve.
Milestone

Comments

@UX3D-nopper
Copy link
Contributor

see https://github.com/KhronosGroup/glTF/tree/master/extensions/2.0/Khronos/KHR_texture_transform

Having the current specification, this implies, that for each texture we could potentially have a unique texture transformation matrix. This could result in - at this point of time - 5 uv coordinates.

What I do not like about this is, that a transformation matrix is calculated for each texture, even if they are using the same resulting transformation matrix. Beside the calculation overhead, obsolete uv sets need to be created.
Looking ahead - having more textures in the future - this does bloat up the final shader code.

Finally, if the usage in Unity and Three.js are carefully read, they also do not support "inifinite" texture transforms:
https://threejs.org/docs/#api/textures/Texture.offset
https://docs.unity3d.com/ScriptReference/Material-mainTextureOffset.html

@donmccurdy
Copy link
Contributor

Could you explain your suggestion of "referenced by the material texture" further? Does that mean only a single texture transform per material, which applies to all textures?

I don't think per-texture transforms require additional UV sets, just uniforms to store the transforms, which can be applied to UVs in the shader. You're correct that three.js supports only a single texture transform per material (with some nuances), although we're working on fixing this.

@andrewvarga
Copy link

I think, if the UV transformation is applied in the fragment shader, indeed just +1 uniform is required for each texture.
But if the UV transformation is applied in the vertex shader, which is I guess more common (?), you indeed need to have as many varyings as required.

For example if you have 3 textures, each having its own texture transformation, texture1 and texture2 using uv channel1 and texture3 using uv channel2, then you need 2 varyings for texture1 and texture2 even though they use the same vertex attribute (for uv channel1):
(varying) v_uv1_texture1 = texture1Transform * a_uv1;
(varying) v_uv1_texture2 = texture2Transform * a_uv1;

So you need a new set of varyings, even if texture1Transform and texture2Transform is the same. Maybe that's what @UX3D-nopper means?

@UX3D-nopper
Copy link
Contributor Author

I was just writing a long comment, deleteid it and to make it short: I dislike the possibility of all the permutations.
Probably there is no way around, but we should probably discuss this during the 3D formats meeting.

@UX3D-nopper
Copy link
Contributor Author

Need to be solved in the future.

@UX3D-nopper UX3D-nopper reopened this Oct 10, 2021
@UX3D-nopper UX3D-nopper added the needs discussion Issue or PR requires working group discussion to resolve. label Oct 10, 2021
@emackey
Copy link
Member

emackey commented Nov 9, 2021

I'm wondering if we eventually need a KHR_texture_transform2 (or whatever name) extension that completely breaks away from the existing transform extension. Things we could consider:

  • There could be a root-level list of available transforms, expected to be short.
  • Each individual texture could then supply a (small) index into this root list, so it could use one of the calculated transform matricies.
  • It would be awesome to allow animation of the transforms at the root level.
  • The multiplication order of the transforms needs to be corrected. In the current extension, if you have non-uniform scaling paired with rotation, you get non-sheared UV coordinates, but a sheared texture image on your model. This should be flipped, such that you see a non-sheared image on the model. (/cc KHR_texture_transform has the wrong rotation matrix #1563)

@donmccurdy
Copy link
Contributor

Each individual texture could then supply a (small) index into this root list, so it could use one of the calculated transform matricies.

Just to be sure it's explicit – it's not the total number of transforms in the scene that is a problem (for three.js anyway, and presumably Unity) but the number of transforms per material. three.js supports only 2 texcoord attributes per mesh primitive, and all textures used by a particular material must share a texture transform if they share a texcoord attribute. So that's a maximum of two transforms, with further restrictions based on texture slot and texture coordinates.

We have some long-term ideas of how to improve that, but I expect it to be a few years before that's generally available, and even then it may have performance tradeoffs.

It would be awesome to allow animation of the transforms at the root level.

Agreed that it would make sense to allow animation somehow if we were re-doing KHR_texture_transform. I'm not sure about implications of the "at the root level" part.

@emackey
Copy link
Member

emackey commented Nov 9, 2021

number of transforms per material

Interesting. If we were to specify the list of texture transforms at the material level (instead of glTF root), and then allow each texture reference within a material to index into that list, would ThreeJS fully support 2 transforms per material? (Meaning, not just the AO map, but all textures? Or perhaps I've mixed up my TEXCOORD indices with transform indices.)

@donmccurdy
Copy link
Contributor

donmccurdy commented Nov 9, 2021

The "root level" vs "material level" question is unimportant to three.js – maybe saves a bit of JSON if you have a lot of materials, but having lots of materials brings unavoidable performance problems quickly so that's probably a wash. I don't think that's what @UX3D-nopper is referring to but I could be wrong.

And yeah, unfortunately the tl;dr for three.js is:

  • occlusionTexture can have its own texcoord and UV transform
  • all other textures must share 1 texcoord and 1 UV transform

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion Issue or PR requires working group discussion to resolve.
Projects
None yet
Development

No branches or pull requests

5 participants