google / model-viewer

Easily display interactive 3D models on the web and in AR!
https://modelviewer.dev
Apache License 2.0
7k stars 825 forks source link

TRex GLB causes console error related to ModelKernel.deserialize #1347

Closed stevesan closed 3 years ago

stevesan commented 4 years ago

Description

Use the tester (https://modelviewer.dev/examples/tester.html), then drag this GLB into it: https://github.com/toji/xr-dinosaurs/blob/master/media/models/tyrannosaurus/compressed.glb

The model displays fine, but there are errors in the console. When loading this GLB into space-opera, it seems to break material editing (which is likely related to the errors?)

Here is the console error: space-opera.js:108917 Uncaught TypeError: Cannot destructure property 'id' of 'serialized' as it is undefined. at ModelKernel.deserialize (space-opera.js:108917) at new Material$2 (space-opera.js:108464) at ModelKernel.deserialize (space-opera.js:108927) at new Model$1 (space-opera.js:108531) at ModelKernel.deserialize (space-opera.js:108927) at new ModelKernel (space-opera.js:108869) at MessagePort.SceneGraphModelViewerElement.<computed> (space-opera.js:162091)

Browser Affected

OS

Versions

modelviewer.dev as of Jul 13, 2020

elalish commented 4 years ago

That file has 16,000 some errors in the gltf-validator. If we can find a model that repros this while passing validation, I'll investigate.

jeff-chamberlain-vntana commented 4 years ago

@elalish The file attached here passes validation but experiences the same issue. It also includes materials that use the KHR_materials_pbrSpecularGlossiness extension, which is supported by other ThreeJS viewers, but doesn't appear to be in model-viewer. The line that the error stems from is attempting to deserialize a material's pbr-metallic-roughness property.

Tailored shirt_Light Grey.zip

elalish commented 4 years ago

@jeff-chamberlain-vntana Ah, I see; I think this is because our scene-graph API doesn't support spec-gloss materials yet (though the renderer does). Is this causing any noticeable pain besides the console error?

jeff-chamberlain-vntana commented 4 years ago

@elalish Well, we're seeing the same (or a very similar) error even when rendering with just the <model-viewer> tag and no scene graph. The resulting view appears to have materials rendering without their diffuse textures. This is the error that is output in that situation:

Uncaught TypeError: Cannot destructure property 'id' of 't' as it is undefined. at Ib.deserialize (model-viewer.js:20) at new material (model-viewer.js:1538) at Ib.deserialize (model-viewer.js:20) at new model (model-viewer.js:1538) at Ib.deserialize (model-viewer.js:20) at new Ib (model-viewer.js:20) at MessagePort.<computed> (model-viewer.js:1774)

elalish commented 4 years ago

@jeff-chamberlain-vntana Ah, that's much more serious! Can you show side-by-side screenshots of model-viewer and something like the babylon sandbox, so I can see the problem?

jeff-chamberlain-vntana commented 4 years ago

Sure, here is the same model in Babylon vs model-viewer

image image

I will see if I can find a model where the difference is more pronounced, but you can see the coloring (provided by a texture) in Babylon while in model-viewer the shirt looks completely white

elalish commented 4 years ago

Wow, I'm getting a much worse result on my macbook with a chrome collar: image

elalish commented 4 years ago

@jeff-chamberlain-vntana This is definitely a bug, but what I can say is that the spec-gloss extension is a bit iffy on support. I've seen a bunch of strange driver bugs with it in three.js and glTF is busy with new specular extensions to replace this because it's pretty non-physical. I'd recommend converting to the metallic-roughness workflow if at all possible.

jeff-chamberlain-vntana commented 4 years ago

@elalish Yeah, I was actually using model-viewer v1.1. I just upgraded to v1.2.1 and I see the same thing you posted above. We have several models with similar issues where it appears to be forcing met/rough, resulting in lots of "shininess" with default metallic values of 1.

Thanks for the explanation. We unfortunately don't have control over the workflow, but maybe we can convert the models to met/rough

elalish commented 4 years ago

@jeff-chamberlain-vntana The shiny shirt looks to be fixed by #1574. The colors are still slightly different compared to Babylon, but I'm guessing that's just differences in lighting or tone mapping.