Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JS error when switching material variants if not all model parts have variant definitions (intentionally) #3086

Closed
3 of 13 tasks
milesgreen opened this issue Dec 29, 2021 · 11 comments · Fixed by #3090
Closed
3 of 13 tasks
Labels
type: bug Something isn't working

Comments

@milesgreen
Copy link
Contributor

milesgreen commented Dec 29, 2021

Description

I noticed that some models with particular material variants cause an uncaught JS error when changing variantName.

It appears that it is caused when there are multiple nodes in the GLB and some nodes have variant definitions and some parts don't.

This is an intentional scenario where there might be a model with multiple variants for some parts, but other parts don't change between variants. EG. For a model of a car, the bodywork might change paint colour, but the wheels wouldn't.

See demo:

https://modelviewer-materialvariant-test.glitch.me/

View the demo. Select the 'Primitives XOX' model from the first dropdown (default). Then select the 'Green' material from the second dropdown. A JS error is thrown. Cone and Cube should turn green, Sphere should remain black. But only Cube turns green. Now select another material. Now select 'Green' again, and this time no error occurs, both Cone and Cube are updated, as expected. So it appears to only affect the first time that variant is applied and subsequent applications work fine.

See the JS Console for the following error:

Screenshot 2021-12-29 at 18 44 16

All the other models in the demo link work, as they all define variants for every node.

Live Demo

https://modelviewer-materialvariant-test.glitch.me/

Version

  • model-viewer: v1.9.1

Browser Affected

  • Chrome, version: 96.0.4664.110
  • Edge
  • Firefox
  • IE
  • Safari, 15.1

OS

  • Android
  • iOS
  • Linux
  • MacOS
  • Windows

AR

  • WebXR
  • SceneViewer
  • QuickLook
@milesgreen
Copy link
Contributor Author

I think I might have found a more definitive case.

In the above demo, I have added one more model: 'Primitives XOX'. This also causes the JS error.

I noticed in the original Faucet model that 2 of the 3 nodes had variants defined in the GLB, but the third had no variants, as it doesn't change material between variants.

I created a new model, 'Primitives XOX', with 3 basic primitives. 2 of the primitives (cone and cube) have material variants defined, the sphere has no material variants defined.

This new model causes the same JS error.

@milesgreen milesgreen changed the title Some Material Variants cause uncaught JS error JS error when switching material variants if not all model parts have variant definitions (intentionally) Dec 29, 2021
@milesgreen
Copy link
Contributor Author

Updated title to be more specific to what I now think the issue is caused by

@milesgreen
Copy link
Contributor Author

Modified initial error description to be more specific to what I now think the issue is caused by.

@elalish elalish added the type: bug Something isn't working label Dec 29, 2021
@elalish elalish assigned ghost Dec 29, 2021
@elalish
Copy link
Collaborator

elalish commented Dec 29, 2021

@timmmeh I can repro this on master; would you mind taking a look when you're around?

@ghost
Copy link

ghost commented Dec 30, 2021

@milesgreen thanks for tracking this down

@ghost
Copy link

ghost commented Dec 30, 2021

@milesgreen can you provide the Primitives XOX model, its blocked from download in the glitch sample

ghost pushed a commit that referenced this issue Dec 30, 2021
Resolve an async bug that occurs when two nodes reference the same material. The first node starts loading a material and marks it as loaded, then while waiting for Three to complete the actual material setup the second node begins loading the same material and which is now marked as loaded thus it tries to fetch the cached material which is not actually available because the first node was still waiting on Three.
@ghost ghost mentioned this issue Dec 30, 2021
@ghost
Copy link

ghost commented Dec 30, 2021

I setup a local test to reference the model and was able to patch this bug.

@milesgreen
Copy link
Contributor Author

Sorry @timmmeh. Been AFK and only just seen your request for the GLB. But looks like you’ve already resolved it? Let me know if you still need the sample GLB.

@ghost
Copy link

ghost commented Dec 31, 2021

No problem and it looks resolved so I shouldn't need that anymore, I can always reference it directly, just can't drop it into the editor but no biggie, thanks again for pointing this issue out.

@milesgreen
Copy link
Contributor Author

Just in case:
Primitives-XOX.glb.zip
Thanks for the prompt fix.

elalish added a commit that referenced this issue Jan 4, 2022
Resolve an async bug that occurs when two nodes reference the same material. The first node starts loading a material and marks it as loaded, then while waiting for Three to complete the actual material setup the second node begins loading the same material and which is now marked as loaded thus it tries to fetch the cached material which is not actually available because the first node was still waiting on Three.

Co-authored-by: timmmeh <tirichards@google.com>
@milesgreen
Copy link
Contributor Author

Can confirm that pulling from master after this commit has indeed fixed the issue I was seeing on the above demo page.
Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants