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

GLTFLoader: exposing GLTFParser class and Constants #15477

Closed
yomotsu opened this issue Dec 26, 2018 · 25 comments
Closed

GLTFLoader: exposing GLTFParser class and Constants #15477

yomotsu opened this issue Dec 26, 2018 · 25 comments

Comments

@yomotsu
Copy link
Contributor

yomotsu commented Dec 26, 2018

Currently, GLTFParser and constants in GLTFLoader are scoped and cannot be used in user-land.
However, sometimes it is needed.

Usecases:

  • glTF-ish file may need special parsing using modified(extended) GLTFParser.
  • glTF models may be supplied in JSON or JSONP format from a WebApp API rather than files and need GLTFParser directly.

Also, WEBGL_CONSTANTS and other constants would be useful if available under THREE or somewhere

What I would like to have is:

  • THREE.GLTFParser

and maybe:

  • gltfLoaderInstance.gltfParser
  • THREE.WEBGL_CONSTANTS.FLOAT = 5126 and so on
@donmccurdy donmccurdy changed the title [Feature request] GLTFLoader: exposing GLTFParser class and Constants GLTFLoader: exposing GLTFParser class and Constants Dec 26, 2018
@donmccurdy
Copy link
Collaborator

One option available now is the loader.parse() method — it accepts JSON or an ArrayBuffer you've already downloaded and parses it, which is helpful for cases like the JSONP API you mention.

We've also discussed (#14492) adding a getParser(...) method to GLTFLoader, so you can access a parser instance before content is downloaded. The loader creates a new parser for each asset it loads, to contain caching information etc.

Between the two, I think that would cover everything you suggest except the constants? Exposing the constants could be OK, and has been suggested before (#11978). Another option would be to put these constants on NPM separately, similar to DefinitelyTyped/DefinitelyTyped#29801.

@yomotsu
Copy link
Contributor Author

yomotsu commented Dec 26, 2018

Thanks for your suggestions and comments.

I didn't know that the loader can call parse() and It could cover most.
(Sorry, I should have read the doc and code more)

What I wanted to do is inject some function in loader.parse().
getParser() and modifying the parser before parse() could do that as you said. (related to #15418)

But using GLTFParser directly without GLTFLoader can also cover that and helpful. I was wondering if you could consider this too.

Anyway looking forward to getParser() 👍

Regarding the constants, I think ideally it should be in one place, rather than in both GLTFLoader.js and NPM, but it still sounds good!

@mrdoob mrdoob added this to the r101 milestone Dec 28, 2018
@mrdoob
Copy link
Owner

mrdoob commented Dec 28, 2018

Wouldn't be opposed to move GLTFParser outside of the closure and add it to the THREE namespace.

The module version of the file could eventually export GLTFLoader and GLTFParser.

@donmccurdy
Copy link
Collaborator

Rather than another top-level object, maybe just GLTFLoader.Parser? Even when using the parser directly users should generally not construct it themselves, but use loader.getParser() when that is implemented. Constants could be something like GLTFLoader.WebGLConstants.FLOAT.

... ideally it should be in one place, rather than in both GLTFLoader.js and NPM...

The idea here would be that there should be official glTF constants on NPM (e.g. maintained by Khronos, not just for three.js), so users can install them in the rare case that they need custom parsing. But threejs examples cannot have dependencies, so the constants will have to be duplicated for GLTFLoader.

@donmccurdy
Copy link
Collaborator

^Feel free to open a PR for moving the parser and constants out of the closure if this is still helpful. I would wait on other PRs for the rest.

@takahirox
Copy link
Collaborator

glTF-ish file may need special parsing using modified(extended) GLTFParser.

Just curious to know what "glTF-ish file" means by.

@takahirox
Copy link
Collaborator

takahirox commented Dec 29, 2018

Rather than another top-level object, maybe just GLTFLoader.Parser?

Voting for non top-level object.

I think putting under THREE may require better maintenance, for example writing document and keeping compatibility as much as possible. It may be too early for the parser yet. I guess we still optimize/refactor/update the parser.

@yomotsu
Copy link
Contributor Author

yomotsu commented Dec 29, 2018

@takahirox GLTFLoader.Parser could be okay for that?

Just curious to know what "glTF-ish file" means by.

I meant VRM and maybe eVRM (can be seen in ニコニ立体), or othrer Web App oriented models.

screen shot 2018-12-29 at 12 28 49 pm

@takahirox
Copy link
Collaborator

takahirox commented Dec 29, 2018

Making .getParser() PR. I think I can share soon. We can start with that.

@takahirox
Copy link
Collaborator

@yomotsu My understanding is VRM consists of glTF 2.0 core spec with some limitation + VRM custom extension. And parser in user side is necessary for handling the custom extension, correct? "glTF-ish" sounded a bit vague to me so I just want it to be more specific.

@yomotsu
Copy link
Contributor Author

yomotsu commented Dec 29, 2018

That's right. VRM is a glTF based extended format, and the Parser may need to handle HumanBones, Facial expression blendings, MToon Materials, and others.

In addition, some Web App like ニコニ立体 may use encrypted VRM like "eVRM" in order to protect user upload models, and injecting user-land function to the parser may be needed as well.

@donmccurdy
Copy link
Collaborator

To be clear, we can't guarantee that the parser's API will remain the same over time. Internal methods might be renamed or removed, and extending them may be fragile. If you're really customizing the loader for new formats and extensions, it's very possible you'll want to just fork the loader.

But if you're OK with all that, a PR is still fine with me.

@yomotsu
Copy link
Contributor Author

yomotsu commented Dec 29, 2018

Thanks for your suggestions. I understand and getParser would be safer to use.
Even though, having the official Parser class as GLTFLoader.Parser would be great for me.

@takahirox Would it be okay?

@takahirox
Copy link
Collaborator

I wanna suggest that we first go with .getParser(). And thinking of the option exposing GLTFLoader.Parser when we realize we really need.

@yomotsu
Copy link
Contributor Author

yomotsu commented Dec 29, 2018

Okay, I will wait for it 👍

@mrdoob mrdoob modified the milestones: r101, r102 Jan 31, 2019
@mrdoob mrdoob modified the milestones: r102, r103 Feb 28, 2019
@mrdoob mrdoob modified the milestones: r103, r104 Mar 27, 2019
@mrdoob mrdoob modified the milestones: r104, r105 Apr 24, 2019
@mrdoob mrdoob modified the milestones: r105, r106 May 30, 2019
@mrdoob mrdoob added this to the r115 milestone Feb 29, 2020
@mrdoob mrdoob modified the milestones: r115, r116 Mar 25, 2020
@mrdoob mrdoob modified the milestones: r116, r117 Apr 30, 2020
@mrdoob mrdoob modified the milestones: r117, r118 May 27, 2020
@mrdoob mrdoob modified the milestones: r118, r119 Jun 24, 2020
@mrdoob mrdoob modified the milestones: r119, r120 Jul 29, 2020
@mrdoob mrdoob modified the milestones: r120, r121 Aug 25, 2020
@mrdoob mrdoob modified the milestones: r121, r122 Sep 29, 2020
@mrdoob mrdoob modified the milestones: r122, r123 Oct 28, 2020
@mrdoob mrdoob modified the milestones: r123, r124 Nov 25, 2020
@donmccurdy
Copy link
Collaborator

Do we still need this, with access to the gltf.parser object and the newer extension API?

@yomotsu
Copy link
Contributor Author

yomotsu commented Dec 2, 2020

It would be better for me.
@fms-cat What do you think?

@0b5vr
Copy link
Collaborator

0b5vr commented Dec 2, 2020

Yes, exposed parser and the plugin system is working well.

@takahirox
Copy link
Collaborator

takahirox commented Dec 2, 2020

The plugin system provides the extensibility of the loader. How do you want to use the exposed parser class? In other words, What are the use cases the plugin system still doesn't cover?

@mrdoob
Copy link
Owner

mrdoob commented Dec 2, 2020

If I understand @fms-cat correctly, he's saying that this issue can be closed.

@0b5vr
Copy link
Collaborator

0b5vr commented Dec 2, 2020

I mean, the constructor of GLTFParser is okay to not be exposed I think. my bad.

@takahirox
Copy link
Collaborator

OK, thanks for clarifying.

Then, let's close this issue for now. And let's reopen or create a new one if you folks encounter a use case the plugin system doesn't cover.

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

6 participants