Conversation
| await onLoad; | ||
| }); | ||
| test('applies no environment map on unlit model', async function() { | ||
| expect(!modelHasEnvMap(scene)).to.be.ok; |
There was a problem hiding this comment.
Nit: for the case of checking a boolean, prefer expect(...).to.be.true (expect(...).to.be.false also works) or expect(...).to.be.equal(true)
src/three-components/Model.js
Outdated
| // Unlit models (using MeshBasicMaterial) should not apply | ||
| // an environment map, even though `map` is the currently configured | ||
| // environment map. | ||
| if (obj && obj.isMesh && obj.material && !obj.material.isMeshBasicMaterial) { |
There was a problem hiding this comment.
For some edge cases, obj.material could be an array of materials. We should probably avoid creating those in GLTFLoader at some point, but for now it's worth handling.
wouldn't that require needing to fetch all the models before being able to run tests? UnlitTest.glb is 4kb as well.
Any models that you know that do this to test? |
Yes, and we do this (models are fetched as part of In practice this is probably fine. We have set up Travis to cache the directory they are checked out to, and it doesn't seem to take much time. We also update in place if we detect that the models are already fetched. |
|
FYI looks like |
|
Using the Khronos sample models now, and handle when |
src/three-components/Model.js
Outdated
| materialStack.push(obj.material); | ||
| } | ||
|
|
||
| for (let material of materialStack) { |
There was a problem hiding this comment.
Did you mean to do iteration of the material stack on every invocation of the traverse callback?
There was a problem hiding this comment.
Mm, yes, but missing clearing it out. I don't like any way that attempts to not duplicate the logic for the (untested) edge case as well. Will change
|
It's a bit contrived, but here's an example that will result in |
instances where meshes may have multiple materials defined.
|
| }); | ||
| }); | ||
|
|
||
| suite('on a model with multi-material meshes', () => { |
Currently with a default env map, image env map, and PMREM'd env map currently:
With this patch:
Note that
UnlitTest.glbis included (4kb) -- can reference the Khronos models if/when they land. The Astronaut is not unlit AFAICT (or, does not use the unlit extension), maybe should change the background-image demo describing it as such.