Bump into the latest glTF loader

[takahirox]

Requires: Hubs-Foundation/three.js#57
From: #4117

This PR replaces our Three.js glTF loader fork with the latest official Three.js one and updates gltf-model-plus.js and avatar-editor.js to follow the latest official Three.js GLTFLoader API.

Benefits

• Less cost for maintenance because we will no longer need to maintain our GLTFLoader fork
• We will be easily able to add or manage new and existing glTF related features with the official GLTFLoader plugin API for example LOD

Please refer to #4117 (comment) for the changes.

Note that the changes in this PR is just for replacing the glTF loader. There may be a chance that they can be further cleaned up or optimized with the new API. But I would like to do them if needed in another PR because I don't want to make this PR bigger and the review harder.

TODO

• merge Bump GLTFLoader into the r128 three.js#57
• update package.json to point back at hubs/master for three
• update package-lock.json to point back at hubs/master and the merged commit sha

[takahirox]

If Hubs-Foundation/three.js#57 is merged, I would update this line back to master in this PR or follow up PR. package-lock.json is same.

[netpro2k]

I usually do it just before merging, and I leave a [ ] todo in the PR description to remind me

[takahirox]

What I have tested so far.

• Enter a room and see an environment is rendered (it means the loader succeeds in loading a glTF scene)
• Upload a glTF object including moz light map extension
• See avatar preview in Avatar settings
• Upload a custom avatar and change the avatar to it

[netpro2k]

This looks great! Definitely feels like a lot more of this is now being don the "right way'. Still some hacks in place but I think we have a clear path forward to improve most of them. Nice work!

[netpro2k]

I usually do it just before merging, and I leave a [ ] todo in the PR description to remind me

[netpro2k]

I know this was just copied over from our existing code, but it still does seem like the GLTFLoader should be respecting this default.

[takahirox]

I think GLTFLoader should respect the default configuration, too. I'm going to suggest it to Three.js at some point. If it will be merged, we can drop this code.

[netpro2k]

ImageBitmapLoader returns an ImageBitmap not a Texture instance I thought. Doesn't this need to be a some kind of texture loader?

[takahirox]

GLTFLoader makes CanvasTexture from ImageBitmap if textureLoader is ImageBitmapLoader so this line should be fine.

But this line may be confusing for people who don't know the inside of GLTFLoader. I will add comment.

[netpro2k]

We are already overriding parser.textureLoader in the constructor. Seems like we can still use the HubsTextureLoader to take care of this no?

[takahirox]

I didn't really want to override parser.textureLoader because GLTFLoader doesn't have a proper API to override the texture loader. Overriding parser.textureLoader = new CustomLoader() is hacky and the GLTFLoader doesn't expect the texture loader to be overridden. There might be a chance that it causes future errors especially when updating to the future glTF loader upstream.

I did override the texture loader because there doesn't seem to be other options to let GLTFLoader use ImageBitmapLoader for Firefox. Yes, overriding the texture loader is hacky as I wrote but GLTFLoader expects that the texture loader can be ImageBitmapLoader so I think it would be less problematic than using our custom texture loader. And if Firefox and Three.js will be updated to support ImageBitmapLoader we will be able to simply drop this hacky code.

But if we realize that this traverse is slow I want to revisit.

[netpro2k]

Just a mental note I re-remember while reading through this PR. This is to clean up the root level resources we create (from Sketchfab zip files) to load the GLTF.

Does the official GLTFLoader have any method of cleaning up the resources it creates?

[takahirox]

If a plugin manage resources afterRoot hook would be a good place to release the resources.

[netpro2k]

Is this guaranteed to be set now? I think the reason we had it originally is that not GLTFs are not required to have a default scene (though almost all I have encountered in practice do).

[takahirox]

Yes, it's guaranteed in the latest GLTFLoader. IIRC the official loader didn't do that before so I think you encountered the problem with the old loader.

[netpro2k]

hah nice workaround

[takahirox]

Good to know that this workaround seems acceptable.

[netpro2k]

Something about this feels a bit weird but I don't really have a much better idea. I maybe would have had the plugin just store it on itself rather than have it passed in and then done something like this after load:

this.inputFiles.gltf = binarySplitterPlugin.gltf;
this.inputFiles.bin = binarySplitterPlugin.bin;

But not sure that is really much better.

[takahirox]

How about this?

await new Promise((resolve, reject) => {
  // GLTFBinarySplitterPlugin saves gltf and bin in gltf.files
  gltfLoader.load(gltfUrl, result => {
    this.inputFiles.gltf = result.files.gltf;
    this.inputFiles.bin = result.files.bin;
  }, onProgress, reject);
});

[brianpeiris]

I will remove my need to review, since you are both already experts in this code and Dom has done a thorough review. Thanks for getting this through!

[takahirox]

Thanks for the reviews.

Just in case, I would like to inform you folks that please don't merge this PR and Hubs-Foundation/three.js#57 until we finish the test with the QA team.

[takahirox]

I just noticed (remembered) that the latest GLTFLoader has dropped DDS texture support because MSFT_texture_dds extension is a vendor extension, not a Khronos KHR extension, and they encourage to use more standard compressed texture KHR_texture_basisu extension now.

If we want to keep supporting DDS texture, we can use an external DDS texture plugin I wrote

https://github.com/takahirox/three-gltf-extensions/tree/main/loaders/MSFT_texture_dds

Any opinion on this? We should encourage KHR_texture_basisu, too, but also I think we don't need to drop DDS texture support and require the asset authors to update their works.

[takahirox]

Ah wait. I noticed that the current gltf-model-plus.js doesn't set DDSLoader to GLTFLoader. That means we don't support DDS texture even now. Then no DDS texture support won't be problem.

[takahirox]

I reverted the removal of HubsTextureLoader from gltf-model-plus.js because it removes the resolved texture image url from Three.js Cache and I couldn't come up with a workaround with the more proper API like plugin API.

As we internally discussed on Discord, we may disable Three.js entire cache so we may want to revisit later.

I added a comment about why we use HubsTextureLoader and we should replace it later so far.

[takahirox]

Somehow the glTF loader replacement resolves the Specular/Glossiness material problem #1613 The Specular/Glossiness material is rendered correctly on the testing server.

[takahirox]

Regarding parser.cache.removeAll(), I thought I added it in GLTFHubsPlugin but it seems I somehow accidentally delete it.

I think it's ok because glTFLoader and parser aren't saved anywhere so the resources used by them should be garbage collected automatically.

https://github.com/mozilla/hubs/blob/693dc621377734a5391e78e18c4d7a2c947b169d/src/components/gltf-model-plus.js#L515-L539
https://github.com/mozilla/hubs/blob/693dc621377734a5391e78e18c4d7a2c947b169d/src/components/gltf-model-plus.js#L638-L718