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

JSM: Exporters: Create ES modules. #15592

Merged
merged 6 commits into from
Mar 15, 2019
Merged

Conversation

donmccurdy
Copy link
Collaborator

@donmccurdy donmccurdy commented Jan 16, 2019

A couple issues had to be cleaned up by hand:

  • converting Math -> _Math
  • removing imports generated from comments containing unused THREE.Foo references.

@gkjohnson
Copy link
Collaborator

Should the modularize.js script be updated to generate these so they can be kept up to date? Or do you expect these will be the canonical versions?

@donmccurdy
Copy link
Collaborator Author

I left them out of the script because there were some manual changes that would have to be repeated, and I’m not sure how to include them in the script.

GLTFExporter has several active PRs, and OBJExporter has one somewhat stale PR. I think all except GLTFExporter could consider the ES modules canonical now.

@gkjohnson
Copy link
Collaborator

gkjohnson commented Jan 16, 2019

Would it be workable to add the THREE.Foo references found in comments to the ignore list for those particular scripts?

And for Math could maybe we could do something like the following for the method and constant replace blocks:

return `new ${ p1 === 'Math' ? '_Math' : p1 }(`;

And update the imports generation line to

var keys = Object.keys( dependencies )
    .sort()
    .map( value => '\n\t' + value )
    .map(key => key === 'Math' ? 'Math as _Math' : key)
    .toString();

I think Math should be the only special case like this and would make it easier to regenerate those files without remembering to do the manual changes.

@donmccurdy
Copy link
Collaborator Author

Done, although it's a little messy... Definitely seeing some value in getting these off of the modularize script and considering them canonical sooner than later. 😓

@gkjohnson
Copy link
Collaborator

@donmccurdy I've made a PR here to help with that. Feedback appreciated!

@donmccurdy
Copy link
Collaborator Author

Great, thanks! Let's review and merge #15599 and then I'll polish up this PR after that. :)

@donmccurdy donmccurdy changed the title Exporters: Create ES modules. JSM: Exporters: Create ES modules. Feb 15, 2019
@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 13, 2019

@donmccurdy #15582 is now merged. Would it make sense to update this PR/rerun the script before merging this one?

@donmccurdy
Copy link
Collaborator Author

I'm glad to update this, although note that some of my changes to the modularize script in this PR (e.g. support _Math) are also provided by #15832, which is probably more robust, if a bit more complex. If we want to merge that PR it may as well happen first.

Or if we would like to keep on with the existing script, I'll update this PR.

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 13, 2019

Well, I just want to ensure that people can use modularize.js^^. More and more users complain about the broken feature and it would be good to provide a working solution.

Maybe we fix modularize.js for now and later switch to something different if necessary?

@donmccurdy
Copy link
Collaborator Author

That's fine with me. 👍 Unless @mrdoob has another preference I'll rebase this PR and re-run the script.

@mrdoob
Copy link
Owner

mrdoob commented Mar 15, 2019

Sorry for the delay. Lets sort this out this cycle 🙌

@donmccurdy do you mind resolving the conflicts in this pr?

@mrdoob mrdoob added this to the r103 milestone Mar 15, 2019
@donmccurdy
Copy link
Collaborator Author

Ok, the PR is up to date. I've tested the four that have examples. The other three (MMDExporter, TypedGeometryExporter, PLYExporter) don't have examples or tests, but static analysis for unused or undefined variables is passing and the imports are all there.

I'll point out that the long ignoreList on GLTFExporter's entry is there because the regex can't distinguish references in comments, and using an AST (#15832) would fix that more robustly. I think I'll need to look at that PR more closely to make up my mind about it. Meanwhile feel free to merge this, or to wait to consider the PR, either way is fine.

@garyo
Copy link
Contributor

garyo commented Mar 15, 2019

Original exporter code. I found a case where it could emit a null set of primitive indices which causes an importer I'm using to crash, and I'm adding an option to allow the user of the exporter to strip geom attributes (again because an importer I'm using fails if the glTF file contains a PRIMITIVEID attr).
I know how to use modularize.js now though :-) so I'll regenerate as part of my PR.

@mrdoob mrdoob merged commit d7fd841 into mrdoob:dev Mar 15, 2019
@mrdoob
Copy link
Owner

mrdoob commented Mar 15, 2019

Thanks!

@donmccurdy
Copy link
Collaborator Author

@garyo Ok thanks! Maybe better to just open the PR against the original file for now, there could be some rebase churn for a while trying to include the generated JSM.

@mrdoob
Copy link
Owner

mrdoob commented Mar 15, 2019

The original file meaning the non-JSM file 😅

@garyo
Copy link
Contributor

garyo commented Mar 15, 2019

I did both before I saw this -- feel free to adjust as needed or let me know & I'll omit the JSM. (The JSM is what I'm using.)

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.

5 participants