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

GLTF: export same mimetype as import #23592

Merged
merged 9 commits into from
Mar 1, 2022
Merged

GLTF: export same mimetype as import #23592

merged 9 commits into from
Mar 1, 2022

Conversation

elalish
Copy link
Contributor

@elalish elalish commented Feb 25, 2022

Fixes: #23587

Description

This allows GLTFExporter to create textures of the same mimeType as the input, so keep file size from growing. @donmccurdy @Mugen87

@elalish
Copy link
Contributor Author

elalish commented Feb 25, 2022

With this change, the export of DamagedHelmet.glb drops from 17.9 Mb to 3.1 Mb, very near the input of 2.9 Mb. I understand this method isn't perfect, but I also don't want to throw the baby out with the bath water. This has the additional advantage that one can now specify any output format for each texture simply by setting userData.mimeType. If this is useful more generally (added to other import/export formats here) we could even make it a member of Texture.

Copy link
Collaborator

@Mugen87 Mugen87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a start this solution is okay!

@donmccurdy
Copy link
Collaborator

@Mugen87 now that the Source object represents the image data, is that a better location for this?

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 27, 2022

Basically yes. However, looking at the code I think using Texture.userData is fine for now.

@WestLangley WestLangley added this to the r139 milestone Feb 28, 2022
examples/jsm/loaders/GLTFLoader.js Outdated Show resolved Hide resolved
@mrdoob
Copy link
Owner

mrdoob commented Mar 1, 2022

Hmm, I still wonder though....

Isn't there a way to store the original image raw data in userData and just write that raw data in the output instead?

Recompressing jpgs/webms doesn't make me feel good 😅

@elalish
Copy link
Contributor Author

elalish commented Mar 1, 2022

For sure, this is a stop-gap solution. But honestly, recompressing a JPEG as a JPEG at the same resolution is pretty much lossless anyway (the high frequencies were already removed). And WebP to PNG is fine except for inflating the size (and it's a small use case at the moment). That's fixable by doing what @donmccurdy was suggesting.

@mrdoob mrdoob merged commit 848d474 into mrdoob:dev Mar 1, 2022
@mrdoob
Copy link
Owner

mrdoob commented Mar 1, 2022

Thanks!

@elalish elalish deleted the exportJPEG branch March 1, 2022 01:49
mrdoob pushed a commit that referenced this pull request Mar 1, 2022
* export same mimetype as import

* Update GLTFLoader.js

Clean up.

* remove webp for now

* fix webp in the proper place this time

* fix bad revert

* remove ??

* cleanup

* addressing feedback

* factor out function

Co-authored-by: Michael Herzog <[email protected]>
@mrdoob
Copy link
Owner

mrdoob commented Mar 1, 2022

Cherry-picked into master and included in 0.138.2.

@bildekrokodil
Copy link

Hi,

Since this change I have problem when my textures come from a canvas element.
map.userData is undefined so it crashes to get the mimeType of it.

Kind regards

@donmccurdy
Copy link
Collaborator

donmccurdy commented Mar 3, 2022

@bildekrokodil could you share the specific error and stack trace that is thrown when this error happens? All texture types should have .userData. But I think this error might fail if one of the three texture types is missing...

texture.userData.mimeType = material.roughnessMap.userData.mimeType || material.metalnessMap.userData.mimeType || material.aoMap.userData.mimeType;

This PR has since been restructured by #23616, so that issue is likely already fixed, but not yet published in a release.

EDIT: Actually it was published as 0.138.2, so I'd be curious if the error still happens and what the full error message is.

@elalish
Copy link
Contributor Author

elalish commented Mar 3, 2022

Oh, interesting; I thought userData always existed on Textures. Does it only exist on certain types of textures?

@donmccurdy
Copy link
Collaborator

donmccurdy commented Mar 3, 2022

All texture types do extend from Texture and so have .userData, I think the source of the error was probably when a property like material.roughnessMap was missing. #23616 already solved that probably.

@mrdoob
Copy link
Owner

mrdoob commented Mar 3, 2022

All texture types do extend from Texture and so have .userData, I think the source of the error was probably when a property like material.roughnessMap was missing. #23616 already solved that probably.

Yeah, I sneaked in a robustness fix in that PR. I should have mentioned it but I was brain dead 😅

@bildekrokodil
Copy link

Hi,

This is the error I got:
TypeError: Cannot read properties of undefined (reading 'mimeType')
at GLTFWriter.processTexture (GLTFExporter.js:1058:32)
at GLTFWriter.processMaterial (GLTFExporter.js:1146:18)
at GLTFWriter.processMesh (GLTFExporter.js:1516:27)
at GLTFWriter.processNode (GLTFExporter.js:1780:28)
at GLTFWriter.processNode (GLTFExporter.js:1801:30)
at GLTFWriter.processNode (GLTFExporter.js:1801:30)
at GLTFWriter.processScene (GLTFExporter.js:1852:29)
at GLTFWriter.processInput (GLTFExporter.js:1907:11)
at GLTFWriter.write (GLTFExporter.js:385:9)
at Proxy.parse (GLTFExporter.js:92:11)

I use the JS version in combination with A-Frame. I export an A-Frame model with canvas textures on it. It works until this commit. At this moment I use the previous version of the GLTFExporter to make it work.

Kind regards

@mrdoob
Copy link
Owner

mrdoob commented Mar 4, 2022

@bildekrokodil Any change you can do a jsfiddle reproducing the issue?

@bildekrokodil
Copy link

@mrdoob
I created a jsfiddle for you. (Sorry it took me some time)

https://jsfiddle.net/bigcroco/xq6hacw5/1/

In the code I have the default a-frame scene. But added a canvas as a resource and another box that uses this canvas as a texture.

When you run it it will give you that error.
If you remove the box with the canvas element as a texture. it works fine and a glft model will be downloaded.

Kind regards

@mrdoob
Copy link
Owner

mrdoob commented Mar 8, 2022

@bildekrokodil Please, report the issue in the a-frame repo instead: https://github.com/aframevr/aframe

@donmccurdy
Copy link
Collaborator

Important to note that the demo above uses an A-Frame version pinned to three.js r125. Pulling a newer version of THREE.GLTFExporter in with an older version of three.js is not really supported by three.js or by A-Frame. It might be best to use a copy of GLTFExporter from r125 until A-Frame updates to a newer version of three.js.

donmccurdy pushed a commit to donmccurdy/three.js that referenced this pull request Mar 10, 2022
* export same mimetype as import

* Update GLTFLoader.js

Clean up.

* remove webp for now

* fix webp in the proper place this time

* fix bad revert

* remove ??

* cleanup

* addressing feedback

* factor out function

Co-authored-by: Michael Herzog <[email protected]>
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.

GLTFExporter grows files 5x from import
6 participants