Prevent the code to fail when loading models without the "materials" property or having a mesh primitive without the material association

[alexdaube]

Fixes #3279

• Couple of null checks in the model and primitive-node constructors
• In space-opera, the material panel will shot "No materials" when the panel does not have any selectable materials found in the active model.

[elalish]

Thanks, this looks great. I especially appreciate all the added tests!

[elalish]

Thanks! I wonder if khronos_SimpleTriangle would work; it's a lot smaller and will load faster in the tests.

[alexdaube]

Thanks! I wonder if khronos_SimpleTriangle would work; it's a lot smaller and will load faster in the tests.

On the surface it would seems so, but khronos_SimpleTriangle generate a Default material in the correlatedSceneGraph object, which is not the case of the vase glb.

[elalish]

hmm, that's interesting. I thought we always generated a default material when none were specified. Can you take a look and let me know what the difference is? I'd like to make sure we're fixing this properly.

[alexdaube]

It does seems a bit fishy. I will check it out tomorrow morning no problem. Today is a bit busy on my end.

[alexdaube]

Looking at the associations in the gltf object generated from the ThreeGLTFParser, the triangle file has 2 associations (material and mesh) and the vase only has 1 (mesh). Looking at the code, the gltf.materials array and the default material is only added when there is a material in the associations.

What would you suggest we do with this?

[elalish]

Hmm, sounds like the bug might be in the GLTFLoader (since it's supposed to create a default material association, and apparently fails to). I think for now we should go ahead and create our default material also in the case there is no material association at all, for robustness. Separately, it'd be interesting to know why GLTFLoader has weird behavior here.

[alexdaube]

I took a deeper look into this and I haven't quite put my finger on exactly what is going on here.
So far, I know that the default material is indeed being created and put in the associations object by the GLTFLoader.
It somehow is not in the associations object when we try to access it through the parser in the CorrelatedSceneGraph. So obviously something is happening in between. We can actually see that the Material is associated to the mesh found in the associations object. I will continue debugging this when I find a bit more free time.

[alexdaube]

I wanted to get to this last week so it could be included in the next release, but I have been indisposed by Covid unfortunately.

Anyways, it seems like the associations entry to the DefaultMaterial is set in different places which explains the inconsistency. For the Triangle files it is done here, which is called when we load the meshes in the loader.
https://github.com/mrdoob/three.js/blob/8921bfaee413e809cb2d27f0aec917408d690a0f/examples/jsm/loaders/GLTFLoader.js#L3114

For the Vase(new file) it is done here, which is called when we load a material.
https://github.com/mrdoob/three.js/blob/8921bfaee413e809cb2d27f0aec917408d690a0f/examples/jsm/loaders/GLTFLoader.js#L3304

The problem that we have in model-viewer with this is that CorrelatedSceneGraph and PrimitiveNode objects are instantiated before loadMaterial is first called and this association is made in the Model constructor I believe.

I could probably address the issue by adding this block of code, which uses the parser's cached DefaultMaterial to setup the association.

The cache attribute is not public in the interface, so I am guessing this snippet is not exactly how we should use the library. I am not sure what direction you want to take with this? Perhaps this should be only fix/addressed three.js?

[elalish]

I wanted to get to this last week so it could be included in the next release, but I have been indisposed by Covid unfortunately.

No problem! Get well soon.

The cache attribute is not public in the interface, so I am guessing this snippet is not exactly how we should use the library. I am not sure what direction you want to take with this? Perhaps this should be only fix/addressed three.js?

Indeed, let's not mess with the internals of three.js. You make it sound like something is happening too soon? Might this just be a race condition of some kind on our side? Maybe we just haven't awaited the right thing.

[elalish]

Ping; did you ever find a deeper cause here? Would love to merge a fix for the root problem.

[alexdaube]

I haven't had much time to check this any further.

Off memory, I think it was basically that in this particular case three.js is only creating the associations for the default material once loadMaterial is being called which in model viewer happens after the constructors cited above. So the race condition assessment sounds about right, though I find it a bit weird as of why this association is done in different places in the three.js GTLFLoader's code to be honest.

I will try to make some time on Friday to try to find a good solution for this.

[elalish]

I'm still interested in what's going on here. Have you gotten stuck?

[alexdaube]

Sorry for the slow response (on vacation for the past few weeks).

I didn't put any time into this since March as my plate got really full, but I am interested in finding a good solution here. Let me put this back into my personal task board and come back to you with this.

[elalish]

It looks like this is actually a three.js bug that needs to be fixed upstream. Should we merge the rest of this PR?