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

3MFLoader needs async handling of JSZip #11583

Closed
1 task done
ericberberich opened this issue Jun 22, 2017 · 9 comments
Closed
1 task done

3MFLoader needs async handling of JSZip #11583

ericberberich opened this issue Jun 22, 2017 · 9 comments

Comments

@ericberberich
Copy link
Contributor

ericberberich commented Jun 22, 2017

Description of the problem

3MF Loader relies on JSZip(data) constructor, but newer versions of JSZip use loadAsync.

Three.js version

[x] r86

Browser

[x] All of them

OS
  • All of them
Hardware Requirements (graphics card, VR Device, ...)
@mrdoob
Copy link
Owner

mrdoob commented Jun 30, 2017

We rely on sync parsing though... We use this pattern:

var loader = new THREE.3MFLoader();
var model = loader.parse( data );

@ericberberich
Copy link
Contributor Author

It's in the parse function:

The new JSZIP(data) in line 48 returns undefined which makes zip undefined.

See https://github.com/Stuk/jszip/blob/master/lib/index.js

function JSZip() {
    // if this constructor is used without `new`, it adds `new` before itself:
    if(!(this instanceof JSZip)) {
        return new JSZip();
    }

    if(arguments.length) {
        throw new Error("The constructor with parameters has been removed in JSZip 3.0, please check the upgrade guide.");
    }

    // --> snip <--

}

@mrdoob
Copy link
Owner

mrdoob commented Jul 1, 2017

😕

@ericberberich
Copy link
Contributor Author

yeah sorry .... libs change more and more to async handling ..

@mrdoob
Copy link
Owner

mrdoob commented Jul 19, 2017

Seems like we'll have to stay with an old version or switch to a library that allows synchronous.

@takahirox
Copy link
Collaborator

takahirox commented Sep 21, 2019

@mrdoob Do you still prefer sync JSZip? Yeah, sync method is easy to use and if we switch to async we break compatibility of .parse(). But I think less blocking the main thread by offloading this type of heavy processing like decoding/encoding is getting popular and being a common technique so it may be a good time to rethink of the async option.

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 22, 2019

But I think less blocking the main thread by offloading this type of heavy processing like decoding/encoding is getting popular

Just to clarify: Because it's async it does not mean the code is executed in a worker. The code is still executed in the main thread.

Apart from that, what would be the benefits of an upgrade? Does it fix a bug? Or performance issue? I does not seem so.

As long as there is no compelling reason, I would not perform a change which does result in migration effort for the user.

@takahirox
Copy link
Collaborator

takahirox commented Sep 30, 2019

Just to clarify: Because it's async it does not mean the code is executed in a worker. The code is still executed in the main thread.

Apart from that, what would be the benefits of an upgrade? Does it fix a bug? Or performance issue? I does not seem so.

Stuk/jszip#195

rewrite the code into workers which are asynchronous

And this should be the benefit because of less main thread blocking.

The code is still executed in the main thread.

I'm not sure if I understand correctly, but do you mean the majority of JSZip 3 code still runs in main thread?

@takahirox
Copy link
Collaborator

takahirox commented Oct 1, 2019

Ah, wait. "Worker" in thir thread doesn't mean WebWorker but something their own class, which runs just asynchronously but not in WebWorkers ...?

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

No branches or pull requests

4 participants