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: Prepare KHR_mesh_quantization support. #20762

Closed
wants to merge 1 commit into from

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Nov 25, 2020

Related issue: #20474

Description

This PR makes the current restrictions in the exporter mentioned in #20474 (comment) more visible.

@donmccurdy
Copy link
Collaborator

Am I correct in understanding that this PR allows some models to be exported with KHR_mesh_quantization, but doesn't fix all of the cases mentioned by #20474 (comment), and so some meshes (that three.js can render, and KHR_mesh_quantization would allow) still won't export?

I'm not sure what you mean about making restrictions more visible. The code changes look OK to me — should we be adding the KHR_mesh_quantization extension automatically when it's needed, as well?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Nov 28, 2020

Am I correct in understanding that this PR allows some models to be exported with KHR_mesh_quantization, but doesn't fix all of the cases mentioned by #20474 (comment), and so some meshes (that three.js can render, and KHR_mesh_quantization would allow) still won't export?

No models using KHR_mesh_quantization that are imported via GLTFLoader can be exported with GLTFExporter even after this PR.

I'm not sure what you mean about making restrictions more visible.

Right now, the exporter throws an error when Int16Array and Int8Array are used. However, this is not the real problem when KHR_mesh_quantization should be supported.

Functions like isNormalizedNormalAttribute() have to be updated if the normal buffer attribute is already quantized. The routines for decoding existing quantized buffer attributes and encoding un-quantized data will introduce the actual complexity to the exporter in context of KHR_mesh_quantization. I think it's easier to review if these changes go in a separate PR.

@mrdoob mrdoob added this to the r124 milestone Dec 2, 2020
@mrdoob mrdoob modified the milestones: r124, r125 Dec 24, 2020
@mrdoob mrdoob modified the milestones: r125, r126 Jan 27, 2021
@donmccurdy
Copy link
Collaborator

Sorry if I'm delaying this, the code in this PR looks fine and I have no objection to it, but I cannot tell what this change is supposed to do.

If the goal it to support KHR_mesh_quantization then perhaps we should wait until #20842 goes in? If you'd rather merge it now, feel free to do so.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Feb 1, 2021

This can be revisited after #20842. In this way, a merge of this PR will not produce conflicts.

@Mugen87 Mugen87 closed this Feb 1, 2021
@Mugen87 Mugen87 removed this from the r126 milestone Feb 1, 2021
@carstenschwede
Copy link
Contributor

carstenschwede commented Apr 20, 2021

I also encountered this error with a particular model. Since #20842 has been merged in the meantime, is there any hope for KHR_mesh_quantization support?

@donmccurdy can gltf-transform be used to convert models to not utilize mesh quantization?

@donmccurdy
Copy link
Collaborator

@carstenschwede could you clarify what error you are seeing, under what conditions? I don't think there are any known problems with three.js loading a model using KHR_mesh_quantization and GLTFLoader, we just can't export them back out with GLTFExporter.

If you did need to remove KHR_mesh_quantization from a model, you could do that with the gltf-transform scripting API. I'll start a thread with what I think the code would be, in donmccurdy/glTF-Transform#233, feel free to comment there if that does or doesn't work for you!

@carstenschwede
Copy link
Contributor

carstenschwede commented Apr 20, 2021

@donmccurdy I also only get the error while exporting, i.e. Error: THREE.GLTFExporter: Unsupported bufferAttribute component type: Int8Array, so removing the mesh quantization from the original model would help me modifying and exporting the model via THREE.js. Thanks a lot for the gltf-transform code, will test ASAP.

Update:
The gltf-transform script in donmccurdy/glTF-Transform#233 can be used to remove KHR_mesh_quantization in case you depend on exporting such models via THREE.js until GLTFExporter is updated. Thanks @donmccurdy!

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

Successfully merging this pull request may close these issues.

None yet

4 participants