GLTFLoader: Lazy dependencies loading

[Zielon]

In this PR I would like to revoke the problem of lazy loading in GLTFLoader. The problem hasn't been solved for a long time. Below I listed a few PRs which addressed it before.

PRs

1. GLTFLoader .createParser() #15508
2. Add lazy option to GLTFLoader. #14492
3. GLTFLoader: Efficient load #14779

Milestone
#15477

I made a gentle change for too eagerly resolving dependencies and if the situation is right pass the responsibility to a user. The current approach always downloads the whole scene in GLTFParser.parse() which is called in GLTFLoader.load(). I would like to avoid the call of

Promise.all( [
    this.getDependencies( 'scene' ),
    this.getDependencies( 'animation' ),
    this.getDependencies( 'camera' ),
] )

from GLTFParser.parse()

[takahirox]

Hi, Thanks for making PR. Some thoughts.

1. I agree with lazy loading support. It's useful to partially load or on demand load especially from a big glTF file or library purpose file (e.g. including only materials, no nodes).

2. Regarding exposing the parser, I'm still on the fence because it's primarily designed for internal use. For example, we may want to change some methods/behaviors at some point for optimization or something. It breaks user code relying on them. It'd be safer for example if a. we expose a wrapper, instead of parser, calling parser's .getDependency or b. making parser's methods private other than .getDependency, and we keep the public methods?

3. Adding resolveDependencies parameter to .load/parse breaks the existing user code. loader.setLazyLoad() or something can be safer?

[donmccurdy]

We're already returning the parser with GLTFLoader's response – I think it's fine for this PR to return the same parser sooner. Per discussion in #15508, I agree that parse() and getDependency() are probably the only methods that should be considered public. With the API that small, we can always add a wrapper later, although I prefer the concept of public/private methods instead of wrapping classes here.

[takahirox]

I agree with considering private/public methods or wrapper later in another PR if needed.

[Zielon]

Since this change is very small and backwards-compatible could anyone from the participants take another look into it? Thanks :)

[donmccurdy]

I'm happy with the implementation, but having a hard time deciding what to name it. The "lazy loading" term implies some amount of automatic-ness to me, like it'll be there when it's needed, or it'll load the whole scene graph but stream the textures in gradually. In this case, the user has to manually request every resource they want, when they need it. Other suggestions:

• .setIncremental( true )
• .setManual( true )
• .setPartial( true )
• .setParserOnly( true )

The last seems most explicit, and might be my vote. @takahirox @Mugen87 thoughts?

[takahirox]

I agree with renaming to more explicit one.

I have a question before deciding the name. I think the original purpose of returning the parser via onLoad() of .load() is for handling custom extension in user side. We may introduce the extensibility in #18484 which provides more appropriate way to handle custom extension. If we go with #18484, do we want to keep returning the parser via onLoad()?

I'm considering it may be ok to stop returning the parser via onLoad(). Because if there are two ways to handle custom extension, it may be a bit confusing to users. And the parser is primarily designed for internal use so it would be safer to hide from normal users, (and expose to only devs who know well inside of glTF loader like with #18484). Otherwise if normal users find that parser is returned via onLoad() and somehow write the code relying on the parser their code can be fragile because there always be a chance that we change the internal API for optimization or other purposes.

If we stop returning the parser, that .load() can return two type objects, one is parsed glTF object when .setParserOnly(false) and another one is parser when .setParserOnly(true), may not sound intuitive. I think adding a new method to get the parser, like .getParser(), for on-demand loading may be more straightforward to users.

So, what do you think of these two?

1. Stop returning the parser via onLoad() (if we go with Introduce GLTFLoader plugin system, first round #18484)
2. Adding an explicit method to get the parser (if we go with 1.)

If we don't go with the above two and we go with .setXXX( flag ), I vote for .setParserOnly(flag).

[donmccurdy]

It might be OK not to return parser with the .load() result someday, but I'd rather not decide yet. I do think it's good to provide some way of getting only a parser — like in this PR — and if we're doing that, parser.getDependency(...) is part of the public API.

Between these two...

// (A)
loader
  .setParserOnly( true )
  .load( url, ( parser ) => {
    // ...
  }, onProgress, onError );

// (B)
loader.getParser( url, ( parser ) => {
  // ...
}, onProgress, onError );

...either one could be fine? I might prefer createParser or loadParser instead of getParser, because getters don't typically take arguments like callbacks.

[takahirox]

I'd rather not decide yet

Yeah, probably we don't need to decide now because at least we don't have #18484 yet. We can revisit later if needed.

If I forget about stopping to return the parser with the .load() result so far, I don't have strong opinion between .setParserOnly(true) and .createLoader(). But I'm inclined a bit to have only the former one (this PR) for now because of less code addition. And currently the result of .load() includes the parser. Only returning the parser if .setParserOnly() is true doesn't sound strange.

BTW, nit. onLoad is async so user can call .setParserOnly() between .load() call and onLoad() fire. For example in the following example code, which should the result have, (a) only the parser (respect to the .setParserOnly flag when .load() is called) or (b) the full parsed assets (respect to the flag when internal .parse() is called)? (b) is the current behavior in this PR.

const loader = new GLTFLoader();
loader.setParserOnly(true);
loader.load(url, gltf => {

});
loader.setParserOnly(false);

I guess switching setParserOnly flag is rare case so documenting this behavior (or documenting we don't recommend switching) may be a reasonable solution.

[Zielon]

@donmccurdy and @takahirox is this change ok for you?

[donmccurdy]

It seems like the code has been lost from this PR. Perhaps a rebase accident?

[Zielon]

It seems that all changes are there. Why do you think something is missing?

[Mugen87]

Isn't this PR a duplicate of #18662?

[mrdoob]

Yeah, seems like a rebase gone wrong...

[Mugen87]

Closing then 👍.