Replace our Three.js glTF loader fork with the official one for Hubs client

[takahirox]

This issue is a sub issue of #4108 to discuss how we can replace our Three.js glTF loader fork with the official one for Hubs Client.

Involved files in hubs repo

• avatar-editor.js
• gltf-model-plus.js
• HubsTextureLoader.js
• GLTFLoader@MozillaReality/three.js

Please edit if lack of the involved files

Why do we need our fork and how can we replace?

• We want to handle our custom glTF extensions (TODO: List up the extensions and clarify their specifications). Ideally we may replace the handlers with the ones using Three.js glTF loader extensibility mechanism, called plugin-system.
• We want to use UUID, not name, for specifying animation target node (For what?). IIRC, the official Three.js glTF loader devs seem to think of adding an option using UUID for animation target node. We may wait for it or we may be able to use glTF loader afterRoot hook to replace the target names with the target UUIDs after parsing the entire glTF file(?).

Please edit or update if lack of information

How can we safely replace?

• Rewriting our handlers to the ones using glTF plugin-system may not be so hard (hopefully). But we should carefully confirm that the change won't break Hubs. Especially glTF-model-plus is one of the core components in Hubs Client. If it's broken it has a big bad impact. We need to prepare the test sets to check it keeps working.

[netpro2k]

We want to use UUID, not name, for specifying animation target node (For what?).

I think this should only be something we need to care about at export time in Spoke. Once its in the GLTF animation targets are all based on node indicies, so they are guaranteed to be unique. It does look like the upstream GLTFLoader does still prefer name... I wonder why? https://github.com/mrdoob/three.js/blob/dev/examples/jsm/loaders/GLTFLoader.js#L3564 - It seems that could lead to problems if multiple nodes have the same name (Maybe this is not allowed in the spec?). It would seem you could just always use uuid and guarantee uniqueness. If we do that, we will need to make sure that clone() handles remapping uuid targets correctly.

TODO: List up the extensions and clarify their specifications

The 3 main ones I know of are MOZ_hubs_components (we have a few names for this we still support for backwards compat), MOZ_lightmap, and MOZ_texture_basisu (deprecated now in favor of KHR_texture_basisu but it should be fairly easy to maintain). They should be fairly easy to port to the new plugin API.

We also do some messing around with asset loading to run everything through our CORS proxy and to use HubsTextureLoader for loading textures (primarily to force using ImageBitmap when available, and clean up CPU side resources after they are uploaded to the GPU). It's possible this could be fully handled through a LoadingManager now but in the past it was a bit tricky because we

[takahirox]

We want to use UUID, not name, for specifying animation target node (For what?)

I had a talk with @robertlong and figured it out why it's needed in Hubs Client (I focus on Hubs Client in this thread. I would discuss for Spoke in another thread.)

A-Frame allows nested components so that a glTF model can be parent/child of other glTF models and multiple models can have the same name. Three.js animation system tries to find a target animation node by traversing a graph from a certain node. So if target node is specified by name, there is a chance that the animation system picks a wrong node. Using UUID can avoid this problem.

It does look like the upstream GLTFLoader does still prefer name... I wonder why?

They think the animation clip should be able to be applied to cloned target nodes. If the loader uses UUID, cloned nodes will have different UUIDs and the animation clip can't be applied.

It seems that could lead to problems if multiple nodes have the same name (Maybe this is not allowed in the spec?)

glTF specification allows duplicated names. But duplicated names are problematic in Three.js animation system (if the loader uses names for specifying animation target node) so Three.js official loader assigns unique names if duplicated names exist. Many or some of other engines seem to do so, too.

If we do that, we will need to make sure that clone() handles remapping uuid targets correctly.

I suggested in Three.js to let the loader use uuid and to add a helper clone() function for remapping uuid but they preferred using names at that moment. But lately some problems caused by renaming to unique names so they may revisit or add an option to use uuid.

The 3 main ones I know of are MOZ_hubs_components (we have a few names for this we still support for backwards compat), MOZ_lightmap, and MOZ_texture_basisu (deprecated now in favor of KHR_texture_basisu but it should be fairly easy to maintain). They should be fairly easy to port to the new plugin API.

I would try to rewrite the handlers to the ones with plugin API.

We also do some messing around with asset loading to run everything through our CORS proxy and to use HubsTextureLoader for loading textures (primarily to force using ImageBitmap when available, and clean up CPU side resources after they are uploaded to the GPU). It's possible this could be fully handled through a LoadingManager now but in the past it was a bit tricky because we

Yeah it sounds like being able to be replaced with LoadingManager (and plugin API).

So, it sounds like possible to replace our glTF loader fork with the official one.

[takahirox]

How can we safely replace?

Maybe we can ask our QA team.

[takahirox]

Note to self, and let me know if they are wrong:

The first things we should do is removing parser dependency from these lines in gltf-model-plus.js because the official loader doesn't have createParser() method.

• Regarding MOZ_hubs_components extension, they are applied to Hubs glTF entity after glTF object is created by the loader. The definitions of unknown glTF extensions for the loader are stored under object.userData.glTFExtensions while parsing glTF, and they are used to apply the components out of the loader. So basically we shouldn't need to change the official loader for MOZ_hubs_components. Only one change needed may be version migration done here and probably we can rewrite to the one with beforeRoot hook.
• Regarding MOZ_lightmap extension, probably the handler can be rewritten to the one with the plugin system like the other additional material properties extensions.
• Regarding MOZ_texture_basisu extension, probably the handler can be rewritten to the one with the plugin system like KHR_texture_basisu extension.
• Maybe we no longer need to save glTF index and instead we may use parser.associations in the plugin to find a corresponding glTF entry.
• glTF.asset is returned by default from the latest official loader so we no longer to save by ourselves.
• I guess parser.cache.removeAll() can be called in afterRoot hook?
• The official loader uses ImageBitmapLoader if it's available so we no longer need to hack.
• The official loader doesn't use ImageBitmapLoader for Firefox but we want let it to use ImageBitmapLoader even for Firefox. We may override parser.textureLoader in the plugin.
• Maybe texture data clean up can be done in afterRoot hook?
• Replacing animation target node name with the uuid can be done in afterRoot hook, or we may need to ask assignName hook for the official loader?
• Still unclear to me what jsonPreprocessor() is and is for but probably we can use beforeRoot hook?
• Proxy stuff is achieved via LoadingManager so it doesn't block the replacement.
• avatar-editor.js uses GLTFLoader to split gltf and bin from glb. Perhaps it can be rewritten as a plugin?
• avatar-editor.js uses MOZ_hubs_avatar extension but I guess the definition is stored in gltf.userData.glTFExtensions?

[netpro2k]

This is an asesome breakdown, nice work! Responded bellow to some of the points:

The definitions of unknown glTF extensions for the loader are stored under object.userData.glTFExtensions

Hmm yeah I guess registering as an actual extension might by tricky because components are expecting the entire GLTF to already be loaded into object3Ds by the time they are instantiated. I guess continuing to pull it out of userData like we do now is not too bad

Maybe we no longer need to save glTF index and instead we may use parser.associations in the plugin to find a corresponding glTF entry.

Cool, did not know this existed! The only thing we use this for is actually a mapping in the other direction https://github.com/mozilla/hubs/blob/master/src/components/gltf-model-plus.js#L223. If a component needs to reference some other entity in the gltf file it will use a node index, and then at runtime we need to be able to translate that to an object3d reference. This is done by passing the indexToEntityMap to the inflators in gltf-component-mappings.js. This is all internal implementation detail so we are free to change it solong as we retain some way to map from node index back to Object3D.

I guess parser.cache.removeAll() can be called in afterRoot hook?

We should actually revisit if we need this. We may also want to think about re-using a single gltf loader between loads instead of creating a new one for every object. This should be much more efficient.

Maybe texture data clean up can be done in afterRoot hook?

Well the acual cleanup here needs to happen after the texture is uploaded to the GPU (we do this to save ram usage since we don't actually need the CPU side texture once we have uploaded it to the GPU). Setting up the onUpdate handler could be done in afterRoot like you suggest. Does the GLTFLoader not allow overriding the TextureLoader?

Replacing animation target node name with the uuid can be done in afterRoot hook, or we may need to ask assignName hook for the official loader?

Yeah. I still feel like this should just be the default behavior... The fact that there are issues cloning these animations also just seems like a bug.

Still unclear to me what jsonPreprocessor() is and is for but probably we can use beforeRoot hook?

This was discussed in Discord, but putting here for completeness. This is used right now to pre-process some gltf files when used as an avatar. If a user sets their avatar to a gltf that does not have a skeleton, we generate a default skeleton for them and then assign the entire gltf as a child of the head. This is probably not super important behavior, but it is nice, and I think your suggestion of doing this in afterRoot seems good.

avatar-editor.js uses MOZ_hubs_avatar extension but I guess the definition is stored in gltf.userData.glTFExtensions?

Ah yes, forgot we even had this. It is an extension on the root of the gltf to provide some additional metadata about avatars. Right now we only used it to show some additional external editor links. I don't think we even have any avatars that still use this... So I think its fine to just drop it. In the future I would like us to put this sort of metadata in the VRM extension anyway.

[takahirox]

Hmm yeah I guess registering as an actual extension might by tricky because components are expecting the entire GLTF to already be loaded into object3Ds by the time they are instantiated. I guess continuing to pull it out of userData like we do now is not too bad

I first want to focus on replacing the loader although we may rewrite more elegantly with the plugin system or whatever so I want to keep going with userData solution so far.

Does the GLTFLoader not allow overriding the TextureLoader?

GLTFLoader doesn't have an API for overriding TextureLoader. It may be possible in beforeRoot with parser.textureLoader = new CustomLoader() but it is hacky. I think avoiding hack would be less problematic and provide better maintainability.

So, I started to try the replacement. I wanted to incrementally apply the change but it seems I need to change all at once because most of the changes need the latest GLTFLoader API. It would make the review harder and sound a bit danger. I would like to work with our QA team to avoid to embed bugs.

[takahirox]

I first tried to replace only the glTF loader and keep our Three.js core fork (r111) so far because I want to move it forward incrementally but the latest glTF loader seems to require the newer Three.js core, Hmmmm

[takahirox]

Figured out the root issue. In r111 or our fork, ImageBItmapLoader.prototype.isImageBItmapLoader = true property is missing but the latest official GLTFLoader refers to the property so the loader couldn't handle ImageBitmapLoader correctly. I'm thinking of adding this property in our Three.js core fork. It shouldn't hurt.

[netpro2k]

Yeah I think that makes sense. It gets our fork one step closer to mainline anyway since when we update ImageBitmapLoader it is going to contain that property anyway.

[takahirox]

Found some other changes we need to apply to our Three.js core.

• Add Loader.setRequestHeader()
• Add MathUtils
• Object3D.applyMatrix() is renamed to Object3D.applyMatrix4()

There may be others. I would try to keep listing up in this thread and making PRs.

[takahirox]

With these three changes, Three.js examples (r111) on our Three.js core fork (r111) + the latest glTF loader (r128dev) starts to work.

• https://github.com/MozillaReality/three.js/pull/53
• https://github.com/MozillaReality/three.js/pull/54
• https://github.com/MozillaReality/three.js/pull/55

I would work on replacing our glTF loader to the latest one when they are merged.

[netpro2k]

Cool, reviewed all those changes, looking good! Let me know if I can help with integration at all.

[takahirox]

I made a branch for replacing our glTF loader fork to the latest official one https://github.com/mozilla/hubs/compare/master...takahirox:BumpIntoTheLatestGLTFLoader

Would you please help the test?

$ git clone https://github.com/mozilla/hubs.git
$ cd hubs
$ git checkout BumpIntoTheLatestGLTFLoader
$ npm ci
$ npm run dev
# access https://localhost:8080 on your web browser

If basic functionality seems fine, I would ask the QA team for more detailed and better coverage tests.

[takahirox]

Closing in favor of #4198