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 WebWorker Support #23857

Merged
merged 6 commits into from
Apr 8, 2022
Merged

Conversation

robertlong
Copy link
Contributor

Description

This PR makes the GLTFExporter work in a WebWorker with OffscreenCanvas support. All references to classes/methods not available on the WorkerGlobalScope have been removed or replaced.

This contribution is funded by Matrix.org.

@donmccurdy
Copy link
Collaborator

Looks great thanks! @takahirox any concerns?

@takahirox
Copy link
Collaborator

I haven't tested yet but looks good to me.

@robertlong
Copy link
Contributor Author

Let me convert this to a draft for just a bit longer. I'm still having challenges with the images being corrupted when exporting as a glb.

@robertlong robertlong marked this pull request as draft April 7, 2022 02:19
@robertlong
Copy link
Contributor Author

Found the fix, forgot to return the promise correctly. Everything seems good now, ready for review 👍

@robertlong robertlong marked this pull request as ready for review April 7, 2022 02:40
@takahirox
Copy link
Collaborator

takahirox commented Apr 7, 2022

@robertlong Do you have an online demo or test? It will help us review.

@Mugen87 Mugen87 added this to the r140 milestone Apr 7, 2022
@robertlong
Copy link
Contributor Author

@robertlong Do you have an online demo or test? It will help us review.

Yes, but let me create a smaller one in CodeSandbox.

@robertlong
Copy link
Contributor Author

examples/jsm/exporters/GLTFExporter.js Outdated Show resolved Hide resolved
canvas.toBlob( function ( blob ) {
} else {

toBlobPromise = canvas.convertToBlob( { type: mimeType } );
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's a fun switch... 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you'd think they would keep the same method name for OffscreenCanvas?

@mrdoob mrdoob merged commit 88ef71e into mrdoob:dev Apr 8, 2022
@mrdoob
Copy link
Owner

mrdoob commented Apr 8, 2022

Thanks!

@mrdoob
Copy link
Owner

mrdoob commented Apr 8, 2022

Minor clean up: 340d588

@repalash
Copy link
Contributor

Hi, was the merging of roughness and metalness map tested after this change?

It looks like the cached canvas is also being used in the buildMetalRoughTexture function which is set to the image source in the returned texture and is then exported by drawing that image on the same cached canvas which should result in an error.

@repalash
Copy link
Contributor

Just tested on https://threejs.org/editor/ : adding a roughness map to an object and exporting gltf, saves a blank roughness/metalness texture.

@mrdoob
Copy link
Owner

mrdoob commented May 30, 2022

Oh no...

@mrdoob
Copy link
Owner

mrdoob commented May 30, 2022

@robertlong Any chance you can take a look?

@mrdoob
Copy link
Owner

mrdoob commented Jun 7, 2022

@robertlong Never mind.

abernier pushed a commit to abernier/three.js that referenced this pull request Sep 16, 2022
* Make GLTFExporter work in a WebWorker

* Fix formatting

* Fix canvas.toBlob

* Fix OffscreenCanvas check

* Fix promise

* Update examples/jsm/exporters/GLTFExporter.js

Co-authored-by: Don McCurdy <[email protected]>

Co-authored-by: Don McCurdy <[email protected]>
@schwyzl
Copy link
Contributor

schwyzl commented Jun 28, 2023

Thanks for this @robertlong

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.

7 participants