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.

Sign up for GitHub

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

Jump to bottom

Fix GLTFExporter when exporting in GLTF with OffscreenCanvas #24031

Merged
merged 7 commits into from
May 11, 2022
Merged

Fix GLTFExporter when exporting in GLTF with OffscreenCanvas #24031

merged 7 commits into from
May 11, 2022

Conversation

LeviPesin
Copy link
Contributor

Related issue: #24024

Description

Fix GLTFExporter when exporting in GLTF with OffscreenCanvas and also simplify promises.

@donmccurdy donmccurdy mentioned this pull request May 8, 2022
Closed
@donmccurdy
Copy link
Collaborator

This was referenced May 9, 2022
Closed
Merged
@aardgoose
Copy link
Contributor

Does the fact that FF currently doesn't support offscreenCanvas need documenting?

@donmccurdy
Copy link
Collaborator

I believe the underlying issue was fixed by #24035, let me know if that's incorrect. Thanks!

@donmccurdy donmccurdy closed this May 9, 2022
@takahirox
Copy link
Collaborator

Hm, if I'm right #24035 doesn't resolve #24024 because the root issue is OffscreenCanvas doesn't support .toDataURL(). Even with #24035, canvas.toDataURL() is still used regardless of canvas type (OffscreenCanvas or regular Canvas) here

https://github.com/CodyJasonBennett/three.js/blob/9944ecd7765ee8a1a010c66a6c5be7bdc2b6511c/examples/jsm/exporters/GLTFExporter.js#L1148-L1152

@LeviPesin
Copy link
Contributor Author

@donmccurdy No, it wasn't fixed (it is still possible to export a scene to GLTF in a web worker with OffscreenCanvas).

@mrdoob
Copy link
Owner

mrdoob commented May 10, 2022

@LeviPesin Sorry, do you mind doing a new PR?

@LeviPesin
Copy link
Contributor Author

@mrdoob But what is wrong with this one?

@mrdoob
Copy link
Owner

mrdoob commented May 10, 2022 •
edited
Loading

I think similar parts of the file have changed since you created this PR?
There were 2-3 PRs at the same time changing the same things in different ways.

@LeviPesin
Copy link
Contributor Author

But there were not any merge conflicts, this PR still works.

@mrdoob mrdoob reopened this May 10, 2022
@mrdoob
Copy link
Owner

mrdoob commented May 10, 2022

I see... Sorry, it's hard to see what's going on...

@mrdoob mrdoob added this to the r141 milestone May 10, 2022
@mrdoob
Copy link
Owner

mrdoob commented May 10, 2022

So much better! 🙏

@mrdoob mrdoob merged commit 9981694 into mrdoob:dev May 11, 2022
@mrdoob
Copy link
Owner

mrdoob commented May 11, 2022

Thanks!

@mrdoob
Copy link
Owner

mrdoob commented May 11, 2022

Does this deserve a 0.140.3 patch release or can it wait until 0.141.0?

@LeviPesin LeviPesin deleted the patch-1 branch May 11, 2022 06:59
@LeviPesin LeviPesin restored the patch-1 branch May 11, 2022 06:59
@LeviPesin LeviPesin deleted the patch-1 branch May 11, 2022 07:00
@LeviPesin LeviPesin restored the patch-1 branch May 11, 2022 07:00
@LeviPesin
Copy link
Contributor Author

I think it is a considerably rare use case, so it can wait until r141...

abernier pushed a commit to abernier/three.js that referenced this pull request Sep 16, 2022
@LeviPesin @mrdoob @abernier
Fix GLTFExporter when exporting in GLTF with OffscreenCanvas (mrdoob#…
0ef7b5b 
…24031)

* Fix GLTFExporter when exporting in GLTF with OffscreenCanvas

And simplify promises

* Remove identation before `.then()`

* Get `toBlobPromise` from a function

* Move `getToBlobPromise` function

* Fix `getToBlobPromise`

* Clean up.

Co-authored-by: mrdoob <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@takahirox takahirox takahirox left review comments

@donmccurdy donmccurdy donmccurdy left review comments

@mrdoob mrdoob mrdoob left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
r141
Development

Successfully merging this pull request may close these issues.
5 participants
@LeviPesin @donmccurdy @aardgoose @takahirox @mrdoob