Allow attaching components that add Object3Ds to meshes

[keianhzo]

The loader crashes when a component that adds a mesh is attached to a mesh node (ie. particle system or water attached to a mesh). This works in the AFrame loader.

I'm not sure why we want to replace the object instead of just adding the new object to it, I seems to me that that would be more aligned with artists wishes as you might want to attach a particle system to an mesh for example but that seems to be how it worked in AFrame too so I focused on parity here.

[takahirox]

I first want to know the original purpose of this check...

It looks like indicating that if a Hubs component in a glTF node generates Three.js Object the glTF node shouldn't be Mesh (or other object) node. Do we have such a limitation in Hubs components?

// glTF
"nodes": [
  {
    // Valid
    "extensions": {
      "MOZ_hubs_components": {
        // foo generates Three.js Object3D in Hubs
        "foo": ...
      }
    },
    "children": ...
  },
  {
    // Invalid
    "mesh": 0,
    "extensions": {
      "MOZ_hubs_components": {
        "foo": ...
      }
    }
    "children": ...
  }
]

This limitation may make sense because we don't assign multiple objects to a single entity.

If we want to allow Hubs component that adds Three.js Object on glTF Mesh node, Hubs Client may need to form the object structure as like

Three.js Group (assigned to Entity A) corresponding to glTF node
  - children[0]: Three.js Mesh (assigned to Entity B) corresponding to glTF.node.mesh
  - children[1]: Three.js Object (assigned to Entity C) generated from Hubs component
  - children[2]- : glTF child nodes (assigned to Entity D, E, ...)

and each object is assigned to different entity?

[keianhzo]

My interpretation of this is that for some reason we don't want components that create meshes to replace a mesh but I'm not sure why that limitation and more importantly this is not how the old loader works and that can break existing scenes.

This patch won't make us assign multiple objects to a single entity if I'm correct, this just allows replacing meshes if the have a component attached that creates a mesh.

Maybe @johnshaughnessy has more info about why it's important that we don't override meshes (again, my main concern if scene breakage as this is not how the AFrame loader works)

[takahirox]

just allows replacing meshes

If I understand correctly...

glTF mesh in a node will be ignored if Hubs components that generate Three.js objects are set in the same node, if we allow replace meshes.

// glTF
"nodes": [
  {
    "extensions": {
      "MOZ_hubs_components": {
        // foo generates Three.js Object3D in Hubs
        "foo": ...
      }
    },
    // this mesh will be ignored because the mesh object will be
    // replaced with objects generated from `foo` component
    mesh: 0
  }
]

Is it an expected behavior? For me, it would be a weird behavior tho.

Perhaps we may need to learn the history from @johnshaughnessy

If my assumption is correct and this behavior is weird but we want to add a hack for old loader compatilibity, we may need to add a comment describing that this is a hack and should be removed when we get rid of the old compatibility.

[johnshaughnessy]

This limitation is intentional. We should not remove it.

In the new code, each entity in hubs is associated with at most one Object3D in the scene graph. This restriction simplifies the code in a number of places.

Our old code would insert Groups into the scene graph whenever the components on a node called for multiple Three.js objects. These changes to the scene graph added a performance cost and increased complexity: We needed to add code to rename nodes, retarget animations, and copy morphTargetInfluences.

We want each component added to a node in a gltf file to unambiguously target an entity when loaded into Hubs. Suppose we allow a node to have a mesh and an audio component and a text component, and then we add a Foo component. To which entity/object3D should we assign the Foo? The mesh, the audio, the text, or a Group node containing them? We decided that there is no good answer to this question in general, and we should reject these gltf files.

As you have rightly pointed out, though, there are lots of gltf files that already break this rules. We should solve this "at the edge", rather than here "in the middle" of the code. Specifically, we should try to transform an invalid gltf into a valid one as early as possible, (probably within a GLTFLoader plugin).

You can see an example of how we already "patch" gltf files before loading them here: https://github.com/mozilla/hubs/blob/31373a2c660e6a349a5548ff5de31bcdf1c9fce6/src/components/gltf-model-plus.js#L508-L521

I think we should similarly try to introduce whatever changes are necessary to a gltf at this earliest possible stage, and then throw informative warnings to the user to let them know what's wrong. Similarly, our hubs blender exporter should warn on these issues and tell the author how to fix them.

In order to keep the client as simple as possible, we need a simple scene graph. We should not enable gltf authors to create a Light that is also an Audio that is also a Mesh, because we cannot represent such a thing in Three. If they want an entity with three children (a light, an audio, and a mesh), that's OK. Or if they want a mesh entity with two children, a light and an audio, that's OK too. But whatever they want, they should explicitly create that structure.

[takahirox]

Perhaps we may also add validation when exporting (with Hubs Blender add-on or from Spoke).

Update: Oops, already mentioned by John

Similarly, our hubs blender exporter should warn on these issues and tell the author how to fix them

[keianhzo]

Thanks for the background on this decision, it makes sense. As you said we can add some warnings on the Blender side (Spoke doesn't have this issue) when adding a "mesh" component and in the migration report, that is something that could help the user to be aware of this limitation and also help them understand which scenes require updates. I'm going to close this PR and add it to the add-on for the next release.

[keianhzo]

Closing this as discussed in the comments. I'll open a PR in the Blender add-on to add warnings in the components panels and the migration report to surface this limitation.

[keianhzo]

Thinking about this again, wouldn't it be better than crashing that we just log the error to the console and continue rather than crashing? I'm concerned about a lot of scenes that weren't designed with this in mind to not load.

[johnshaughnessy]

Thinking about this again, wouldn't it be better than crashing that we just log the error to the console and continue rather than crashing?

Throwing an error doesn't mean crashing. When the model fails to load, the calling code decides how to handle it:

• The media loading system handles it by replacing it with an ErrorObject():
  https://github.com/mozilla/hubs/blob/31373a2c660e6a349a5548ff5de31bcdf1c9fce6/src/bit-systems/media-loading.ts#L196-L199
• The scene loading system handles it by displaying a scene loading error:
  https://github.com/mozilla/hubs/blob/31373a2c660e6a349a5548ff5de31bcdf1c9fce6/src/bit-systems/scene-loading.ts#L172-L177

I'm concerned about a lot of scenes that weren't designed with this in mind to not load.

Yes, your concern is valid. But we should fix the issue by patching the gltf file in a preprocessing step (probably in a GLTFLoader plugin), not at this point in the codebase. That way the places where we have to be concerned about these "invalid" gltfs is minimized: It is only at the "boundary" of the application, not "in the middle".

Another way to think about it is: Our app would be simplest if all gltf files were guaranteed to have the properties we want. We can't assume they are in the correct shape, so we have to do something. We might as well do that transformation / migration first, as early as possible in the pipeline.

Eventually if we move to support a specific avatar format like vrm, we'll want to do the same thing: For any gltf file that we are loading as an avatar, check that it is in vrm format. If it's not, transform it to a vrm, then proceed as normal. That way all of our legacy avatars continue to work correctly despite the client using vrm related code.

We will need to fix this issue before enabling newLoader widely.

[keianhzo]

Got it, patching the glTF seems like good option. I've added some code in the loader HubsPlugin to make sure that we add child nodes for those extensions that add objects if the node is not a Object3D. I have to add an explicit list of the extensions that require this as right now we don't have any other way of getting that info, let me know if you have any better ideas.

This should make all scenes compatible with the new loader.

I'll add also the warnings to the Blender add-on UI so users can avoid that in origin.

[takahirox]

Added some (nit) comments, but basically the change looks good to me.