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 add support for JSZip 3 #19097

Closed
wants to merge 3 commits into from
Closed

3MFLoader add support for JSZip 3 #19097

wants to merge 3 commits into from

Conversation

tentone
Copy link
Contributor

@tentone tentone commented Apr 10, 2020

Hello

Added support for JSZip 3 and 3MFLoader, since JSZip 2 is no longer being updated and for sometime is a bit tricky to have booth versions on the same project.

The loader still works as it used to when using it with JSZip 2, but when JSZip 3 is used the parse method has to be used with an asynchronous callback method.

Thanks a lot. Waiting for feedback.

@tentone
Copy link
Contributor Author

tentone commented Apr 10, 2020

The PR still needs a couple o changes i was testing it more carefully today, I will update the PR soon and provide an example of usage.

@tentone
Copy link
Contributor Author

tentone commented Apr 10, 2020

Updated the PR teste with more models seems to be loading everything properly.

@looeee
Copy link
Collaborator

looeee commented Apr 11, 2020

Related: #17864

@tentone tentone changed the title 3MFLoader Support for JSZip 3 3MFLoader add support for JSZip 3 Apr 11, 2020
@tentone
Copy link
Contributor Author

tentone commented Apr 11, 2020

Thanks. Didn't known about that one but It broke support JSZip2 and it changed the API.

In this PR i keep support for for JSZip2, when using JSZip2 it works exactly the same way.

Only if you use JSZip3 the parse() method receives a callback for retrieving the result. But thats the only change, required.

The target was to support booth versions, since some people already are using one JSZip version and dont want to import booth version of the same lib to use the loader.

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 13, 2020

Can you please run node utils/modularize.js once so the jsm version of 3MFLoader is updated, too? This will allow to debug the PR with this link:

https://raw.githack.com/tentone/three.js/3MF-JsZip3/examples/index.html

@tentone
Copy link
Contributor Author

tentone commented Apr 13, 2020

Update the JSM version of the loader. Thanks a lot. Seems to be working correctly.

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 22, 2020

@tentone Thank you for working on this! After thinking about this issue several times, I've come to the conclusion that it's no good approach to support multiple version of JSZip in loaders. Supporting several distinct version of external libraries in general (independently of JSZip) is not something I would promote doing.

We have discussed this topic earlier and I think the conclusion is still valid: We need a way to implement a sync pattern since the loader's API must not be affected by upgrading JSZip ( and yes, the parse() method is part of the public API). If necessary, we have to switch to a totally different zip library.

@tentone
Copy link
Contributor Author

tentone commented Apr 22, 2020

I totally understand, I'm not a big fan of these massive changes in library API. But since its a recurring problem i think that we should advance with some sort of solution for it.

Currently with the 3MF loader (and since we dont have a proper dependency management solution in place) can be a bit tricky to use when booth version of the library are required making it impossible to use the loader sometimes (e.g when we need to have two version of the library in global space).

On the PR you have mentioned the proposed solution only supported the async version with JSZip3, breaking the API usage for older projects.

The solution proposed does not respect the current parse() method interface properly when using JSZip3.

I think that throwing an Exception when using the parse() method and JSZip3 would be a good solution to have it working with booth version of the library without having a parse() method that follows a different interface. Only the load() method could be used with JSZip3.

Eventually will be necessary to have some sort of async alternative to parse() in order to comply with some of these loaders that can benefit from async (using workers or some sort of promise based browser API). But if we really want to have a generic way of using the loaders we will also need a method to check if the sync/async modes are supported.

When that happens we can come back and edit the loader to use the new async "parse" method when available.

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 22, 2020

can be a bit tricky to use when booth version of the library are required making it impossible to use the loader sometimes

Currently, it's necessary to import JSZip as a global script via jszip.min.js. Would it be helpful to you if we provide jszip.module.js? In this case, the module version of the respective loaders could automatically load JSZip without polluting the global space.

In fact, users wouldn't even notice JSZip anymore which is more or less what we want to achieve anyway.

@tentone
Copy link
Contributor Author

tentone commented Apr 22, 2020

That could solve, only if the user is using some sort of module management system winch is not always the case.

Also it would require to have two copies of the "same" library, aside from that fact that we not taking advantage of the async optimizations there.

I was taking a look at the Obj2Loader to see how the async is handled there (its using a callback method and the parse() method is called without a response).

Im trying to figure out a better solution that would work for booth scenarios but that would also comply decently with the current loader API, since this is/will be also a problem for other loaders eventually.

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 22, 2020

That could solve, only if the user is using some sort of module management system winch is not always the case.

The non-module example files will be hopefully deprecated (and removed) sooner or later (see #18749). So I don't think it's necessary to consider the use case when global scripts are imported.

@tentone
Copy link
Contributor Author

tentone commented Apr 22, 2020

Thats great, that pretty much solves the problem. 👍

But then it would be better to update the library to support the faster async method with JSZip3 only correct?

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 22, 2020

I'm open for any solutions if it's possible to retain the current loading pattern 😊 .

@tentone
Copy link
Contributor Author

tentone commented Apr 22, 2020

Well its pretty much impossible to keep it as is with the newer version xD

Anyway, thanks a lot! I think this PR can be closed.

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