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

Examples: Fix webgl_worker_offscreencanvas. #23365

Closed
wants to merge 1 commit into from
Closed

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Jan 28, 2022

Related issue: #23255

Description

It seems the spec of Import Maps has not yet defined how they should work with Web Workers (see WICG/import-maps#2). Hence webgl_worker_offscreencanvas is currently broken.

This PR is a hotfix to make webgl_worker_offscreencanvas work again. However, the PR is essentially wrong since using import * as THREE from '../../../build/three.module.js'; does not allow the usage of example modules.

I'm afraid the missing import maps support in workers was not discussed in #23255.

/cc @marcofugaro

@marcofugaro
Copy link
Contributor

This PR looks good to me.

examples/jsm/offscreen/ is not supposed to be imported by users anyway.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 28, 2022

Yeah, but the problem is the introduction of import maps breaks all usage of example modules in workers. For example it's not possible to use OrbitControls in a worker anymore like in the following demo:

HTML: https://github.com/mrdoob/three.js/blob/dev/manual/examples/offscreencanvas-w-orbitcontrols.html
Worker Script: https://github.com/mrdoob/three.js/blob/dev/manual/examples/shared-orbitcontrols.js

@marcofugaro
Copy link
Contributor

Hmmm, might be worth looking into an import map polyfill for web workers, see guybedford/es-module-shims#206

@marcofugaro
Copy link
Contributor

Looks like polyfill still needs some work to be able to funciton in web workers.

@Mugen87 do you think we could use skypack for that particular example in the meantime? Like this: https://jsfiddle.net/b9fcn3dL/

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 28, 2022

Yes, there is no other choice anyway. When the manual's editor is fixed (see #23363 (comment)), this approach can be applied to the worker demos.

However, I'm not sure what to tell devs who run into this issue with their app. Asking to stay with r136 is only acceptable if the polyfill works with workers soon (e.g. in one or two month). If not, we need a different solution.

I think we have to clarify if #23255 would have been merged if we would be aware of this restriction. If we say yes, we have to figure out something for the worker scenario.

@mrdoob
Copy link
Owner

mrdoob commented Jan 28, 2022

Cherry-picked in master.

@mrdoob mrdoob added this to the r137 milestone Jan 28, 2022
@mrdoob mrdoob closed this Jan 28, 2022
@mrdoob
Copy link
Owner

mrdoob commented Jan 28, 2022

Thanks!

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.

3 participants