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

GLTFExporter: Implement KHR_texture_transform. #15486

Closed
donmccurdy opened this issue Dec 29, 2018 · 7 comments
Closed

GLTFExporter: Implement KHR_texture_transform. #15486

donmccurdy opened this issue Dec 29, 2018 · 7 comments

Comments

@donmccurdy
Copy link
Collaborator

Via https://stackoverflow.com/questions/53942232/three-js-gltf-exporter-resets-the-u-v-to-default-1-1-after-exporting.

@pailhead
Copy link
Contributor

pailhead commented Jan 1, 2019

See #14174

Are there any canonical gltfs with this feature (extension?)?

Any thoughts on the extension part of this spec? If it's an extension for gltf, and gltf is one of the formats that three works with, how would you be implementing this? Could it extend to other textures? Ie. from the answer on SO, could we change how repeat works so it's more in line with this extension?

@donmccurdy
Copy link
Collaborator Author

ThreeJS supports a strict subset of the features used by this extension — one transform per material, rather than one transform per texture. So in terms of implementing export, there's no problem and minimal work required, just the serialization in GLTFExporter.

We've implemented loading the extension already, which (due to the subset supported) does have the limitations mentioned under GLTFLoader • Extensions, which you've written about elsewhere.

Are there any canonical gltfs with this feature (extension?)?

There are one or two buried in KhronosGroup/glTF#1015, or the latest Blender exporter and Adobe Dimension 2.0 also support it, I think.

If it's an extension for gltf, and gltf is one of the formats that three works with, how would you be implementing this?

Think of extensions as features of glTF that aren't yet ubiquitous, rather than as a new format or something. We list the supported extensions in the GLTFExporter/Loader documentation, other than that it's just bit of additional JSON to read/write when needed.

Could it extend to other textures?

I'm not sure what this means?

Ie. from the answer on SO, could we change how repeat works so it's more in line with this extension?

I'd like to see threejs support per-texture transforms eventually, but it's not necessary for implementing export. I don't think anything in three.js core needs to change for adding this feature to GLTFExporter.

@looeee
Copy link
Collaborator

looeee commented Jan 3, 2019

I'd like to see threejs support per-texture transforms eventually

Don't we already have some level of support, with texture.matrix? Rotation, translate (offset) and moving the center of rotation are supported.

@pailhead
Copy link
Contributor

pailhead commented Jan 3, 2019

Sorry i realized i wrote in a confusing manner.

I'm not sure what this means?

You got it with this:

I'd like to see threejs support per-texture transforms eventually

Also:

Don't we already have some level of support, with texture.matrix?

We do and we don't, i think that three is somewhere in between, because while we expose this property, it's being consumed in a certain way. I was hoping if we could revisit the Texture class itself. #12788

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Jan 3, 2019

Don't we already have some level of support, with texture.matrix?

Mainly the limitation is that all textures on a material must share the same transform, despite its being a per-texture property in the API. There have been PRs to fix it, but they were a bit too complex I think. Not too high-priority yet; the export thing mentioned on SO can be solved now regardless.

@pailhead
Copy link
Contributor

pailhead commented Jan 3, 2019

It would be nice to track all those issues. Yeah the exporter should not be blocked especially if its simple, but i think the texture.matrix or rather the texture part there is worth the revisit. The way the material chooses which of the transforms to use seems clunky and more of a glitch then a feature, ie. i don't think it allows for much (very similar as if it were material.textureMatrix) and gets in the way of things like this.

@donmccurdy
Copy link
Collaborator Author

This issue is a simple fix, see #15507.

I think #12788 describes the rest of it well enough.

NodeMaterial solves the same issues (per-texture transforms, per-texture UV slots), so personally that's where I'm going to invest my time, but I'm not opposed to other changes if there's agreement on the API.

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

No branches or pull requests

3 participants