Model load refactor

[sdumetz]

Fixes #325 and #326 with proper LoadingManager usage.

In the end it's arguably better than before because GLTFLoader counts 1 "item" per GLTF buffer, which generally doesn't make much sense and is a bit too verbose to my taste.

a581341 in particular may lead to some unexpected results : Those kind of algorithms are always a bit tricky to get right.

[gjcope]

Thanks for the PR! Looks like it's almost there. I'm not seeing any issues with regular use, but when reloading/replacing a model in place the loading spinner (and subsequently interaction prompt) disappears. This was not the case with #326 I'll try to dig in soon but feel free if you have any ideas.

[gjcope]

Could change this to signal.aborted ? this.loadingManager.itemEnd(url) : this.loadingManager.itemError(url); I think the only ramification here is the loaded file count.

[sdumetz]

We could. Do you think the AbortError should be silenced? Maybe we should let it surface at least initially to help troubleshoot if it aborts when it shouldn't.

[sdumetz]

Looks like proper use is to always call itemEnd even after an error

https://github.com/mrdoob/three.js/blob/dev/src/loaders/FileLoader.js#L261

[gjcope]

IMHO an AbortError is an abort without a reason. But I think it's fine how it is. There is still a little wonkiness with how we are handling the interaction prompt, but I think that's a separate issue that is just highlighted by this update.

[gjcope]

So turns out the issue I saw is only with a dev build due to the error thrown in the itemError handler:

dpo-voyager/source/client/components/CVAssetManager.ts

Line 208 in 5728312

console.error(`Loading error`);

We could just change that to a log statement or adjust the ModelReader error handler as shown in my inline comment.

[gjcope]

I think this is good to go. It will also need a node types update to package.json but I can add that in. I will also bump the "engines" node minimum to v16.

[gjcope]

Merged to https://github.com/Smithsonian/dpo-voyager/tree/rc-47
#325 and #326 can be closed, yes?

[gjcope]

@sdumetz We did some team testing of v0.47 today and some issues with the loading updates arose. I'd really love to push this update out, but I think at least a couple of these issues are stopping that. I'll try to list them out below as best I can. You can see some proposed solutions (or patches) here: https://github.com/Smithsonian/dpo-voyager/tree/dev-loading-fixes

High priority (these need to be resolved)

• Dragging/dropping .gltf packages is broken in standalone mode. This is due to the url transform being lost in the processing of the dependent files (.bin, textures) since the loader is using it's contextual loading manager rather than the main one. Adding the manager back to the GLTFLoader constructor fixes the issue and doesn't seem to have any other side affects other than going back to counting every buffer as an item.
• It seems pretty easy to get the loading spinner to malfunction (majorly delayed or just not visible) when rapidly reloading objects in a scene. Likely not a common use case, but users also reported seeing this in regular multi-model scenes.

Medium priority (would be nice to resolve)

• Issues have popped up when switching scenes due to independent textures (environment map, overlay map) having the models they are associated with disposed of before their loader completes. I added a couple of patches for this in the branch linked above, but they can result in those maps sometimes not getting the initial render they need (can be reproduced if you throttle your network speed). It could be good enough for now, but I think the real solution is to have a cancellable TextureLoader as well.

Low priority

• It seems to only happen when rapidly switching models, but the compileAsync function can throw an error. The dispose() code in Memory leak when using reloadDocument() #185 can also pretty easily trigger a weird renderer error as well but again just when rapidly switching. Curious if you are still using the Memory leak when using reloadDocument() #185 snippet. I can definitely see the memory leak without it and with this update.

[sdumetz]

Not sure I understand what happens in the first issue. I didn't test with non-binary gltf files. I'll try to do so to double-check what happen s but in any case I'm OK with the proposed solution. Maybe in that case also add it back to the Draco/KTX loaders for consistency?

users also reported seeing this in regular multi-model scenes.

I can confirm. I didn't take time yet to fully understand how/why though.

I'm using the snippet in #185 only in some specific cases. Never figured-out where the underlying problem really was but I think it is a stale reference somewhere that might only be there in very specific cases.

I'll try to reproduce.

[gjcope]

Not sure I understand what happens in the first issue. I didn't test with non-binary gltf files. I'll try to do so to double-check what happen s but in any case I'm OK with the proposed solution. Maybe in that case also add it back to the Draco/KTX loaders for consistency?

When using standalone mode, there is a url modifier in place

dpo-voyager/source/client/components/CVStandaloneFileManager.ts

Line 68 in 5728312

this.assetManager.loadingManager.setURLModifier( ( url ) => {

to translate to the in-memory blobs.

When loading .gltf with the additional sidecar files, those are not using our file loader instance, but another loader instance within the GLTFParser class. For that instance to also make use of the url modifier, it has to be initialized with the same file manager.

[gjcope]

I was able to fix a small memory leak with an undisposed material and also identify a leak related to shadows, but neither seems to be a significant contributor.

[sdumetz]

Done some research about the LoadingManager thing, it looks like having to register additional items into the loadingManager is common practice anyway (from GLTFLoader.js#L212). So I was wrong to consider this number should more or less represent the asset count in the scene. I couldn't find a place where we might still be missing an itemStart that would be causing the instabilities though.

13550c1 should take care of the created buffers if the request is cancelled or if compileAsync fails.
I think compileAsync can be factored into the request deduplication mechanism I backported from FileLoader, to handle possible parallel calls but that would require additional testing and benchmarking so it's better left for later.

By the look of it, the inner code of GLTFLoader does not clean up in case an error happens halfway through parsing. We have no way of doing so though, so not handling that. This shouldn't happen too much in normal use though.

[gjcope]

I think the instability ties into the CVAssetManager logic that sets busy to false when handling an error (in this case a cancelled load)

dpo-voyager/source/client/components/CVAssetManager.ts

Line 212 in 5728312

this._isBusy = false;

The issue is pretty easy for me to reproduce and it seems like when switching scenes files can get queued up for load before the error is handled meaning busy gets set to false for the majority of the load. This still doesn't explain why it happens sometimes when not switching scenes.

[sdumetz]

I never even noticed we used an extension of LoadingManager and not the base class itself. What is the reasoning behind using the _isBusy property instead of itemsLoaded === itemsTotal (What LoadingManager does to trigger onLoad)?

Switching back to this behavior would be consistent with THREE's internal handling and might solve the problem?

[gjcope]

I never even noticed we used an extension of LoadingManager and not the base class itself. What is the reasoning behind using the _isBusy property instead of itemsLoaded === itemsTotal (What LoadingManager does to trigger onLoad)?

Switching back to this behavior would be consistent with THREE's internal handling and might solve the problem?

I can't speak to the reasoning because I didn't write that code, but itemsLoaded and itemsTotal are only available in the onStart and onProgress callbacks so to have a queryable 'isBusy' we have to maintain that state somewhere in the derived class no?

FWIW just not setting _isBusy to false when handling an error seems to do the trick and hasn't had any side affects that I can see.

[sdumetz]

FWIW just not setting _isBusy to false when handling an error seems to do the trick and hasn't had any side affects that I can see.

Yes, they are in the constructor's closure so the only access we have to this is through the onload callback that gets called when itemsLoaded === itemsTotal, which is now the only place isBusy gets set to false.

I suppose the reasoning was so itemEnd can be skipped when itemError is called. But then we loose the counter side-effect so this is not a very good tradeof in more complex scenarios.

I don't see any reason why it would not work properly with your change, and if it does it's probably a missing itemEnd somewhere.

[gjcope]

Good point, i missed the onload. But it seems like we are effectively doing the same thing now. _isBusy is a proxy for itemsLoaded === itemsTotal which we need in that class so the associated component can check it, etc. Happy to do something cleaner down the road if proposed but I think this will work for now.