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

instancedMesh + postprocessing + Suspense Heisenbug #215

Closed
s3ththompson opened this issue Jul 14, 2020 · 9 comments
Closed

instancedMesh + postprocessing + Suspense Heisenbug #215

s3ththompson opened this issue Jul 14, 2020 · 9 comments
Labels
external bug A bug that can't be fixed in this project

Comments

@s3ththompson
Copy link

I am running into a bizarre bug that has something to do with the interaction of 3 components in react-three-fiber. (Per @drcmda's suggestion in pmndrs/react-three-fiber#570 (comment), I'm opening a issue here since it seems like postprocessing is a key component of the bug.)

Codesandbox (important section reproduced below)

<Particles count={100} />{/* instancedMesh */}
<Suspense fallback={null}>
    <Box />{/* mesh */}
    <StandardEffects bloom={{ luminanceThreshold: 0.99 }} />{/* postprocessing effects pass, from drei */}
</Suspense>

Running the code throws the following error:

Uncaught TypeError: Cannot read property 'isInterleavedBufferAttribute' of undefined
   at Object.get (three.module.js:11557)
   at setupVertexAttributes (three.module.js:12415)
   at Object.setup (three.module.js:12244)
   at WebGLRenderer.renderBufferDirect (three.module.js:17278)
...

Here's the weird part: removing any one of <Particles/> (which is an instanced mesh), <Box/> (just a regular mesh, but crucially one inside a <Suspense/> wrapper) or <StandardEffects/> (a drei wrapper around postprocessing) seems to fix the issue.

I'm not even sure where to start debugging this one... the TypeError comes from three.js trying to read object.instanceMatrix which doesn't exist. This seems to point to an issue with InstancedMesh and postprocessing, although as I mentioned above, removing the regular mesh from inside the react-three-fiber Suspense wrapper also gets rid of the error... 🤷‍♂️

@s3ththompson
Copy link
Author

It looks like the issue is related to the Normal Pass. Here's a version of the Codesandbox that works with the normal pass commented out.

Is it possible it has something to do with the fact that the NormalPass uses a material that always sets morphTargets, morphNormals, and skinning to true?

I'm a little over my head, but reading around the area in three.js where the TypeError is thrown (renders/webgl/WebGLBindingStates.js:L295) it looks like setupVertexAttributes is looking for an instanceMatrix on a mesh (L384) because the program's attributes say it should be there (L311)

cc @drcmda

@vanruesc
Copy link
Member

vanruesc commented Jul 14, 2020

Is it possible it has something to do with the fact that the NormalPass uses a material that always sets morphTargets, morphNormals, and skinning to true?

Yes, that's probably it - I haven't seen three crash with this error before, though.

Enabling the override material workaround fixes the issue: https://codesandbox.io/s/r3f-bug-fix-standardeffects-btfxe?file=/src/StandardEffects.js

@vanruesc
Copy link
Member

I found that moving the <Box /> out of the <Suspense /> also fixes it. Unfortunately, I don't know React well enough to determine the root cause of the issue. According to the React docs, Suspense is an experimental feature which might have something to do with the weirdness.

@vanruesc vanruesc added the investigating A potential bug that requires further research label Jul 14, 2020
@drcmda
Copy link
Member

drcmda commented Jul 14, 2020

suspense is just for fetching the smaa textures. they load async, so the suspense block may conclude after the other scene stuff. moving box out means that "box" exists immediately while effects needs some time until the fetch request is through.

@s3ththompson
Copy link
Author

Thanks for the fast response! This will definitely help patch things in the short term.

Part of the weirdness is definitely related to Suspense, but I don't think it's an issue with it being experimental... I would guess it's more to do with the way that the react-three-fiber reconciler adds new objects to the scene (when the Suspense resolves) after rendering has already started (or replaces existing objects if the fallback is not null).

@vanruesc is there anything about the way the Normal Pass is set up that relies on objects / materials already existing at the time the constructor is called? (@drcmda is this line of reasoning way off base?)

@vanruesc
Copy link
Member

Thanks for the additional information. Three compiles override materials based on which mesh gets rendered first.

I should've been more precise when I said that moving the Box out of the Suspense "fixes" it. My guess is that this causes the Box to be rendered before the Particles which means that the override material is compiled against the simple mesh. The error disappears because three doesn't look for instancing data anymore, but the normals of the instanced mesh won't be rendered correctly in this situation.

The only real solution for scenes that use a mix of instanced, skinned and common meshes is to enable the override material workaround which assigns individual override materials to different types of meshes.

@s3ththompson
Copy link
Author

Aha! That makes perfect sense. Thanks for explaining. I'm not sure there is a bug then, per se, aside from documenting the issue. unless you think that three should be able to handle the case in which the override material is compiled against the instanced mesh, without throwing an error?

@drcmda
Copy link
Member

drcmda commented Jul 15, 2020

@vanruesc should i enable the workaround by default in the abstractions? does it have any downsides? there's standardeffects in a lib we publish called "drei" but there's also react-postprocessing which is in the works, a react specific wrapper for all of pp's effects.

@vanruesc
Copy link
Member

unless you think that three should be able to handle the case in which the override material is compiled against the instanced mesh, without throwing an error

I think the override material system should be replaced with MRT support and more flexible, extendable built-in materials.
Related: #194 (comment)

should i enable the workaround by default in the abstractions?

Well, it depends on the use case. If you have a scene that only contains static meshes, then the override material mechanism is the most efficient solution. In reality, however, scenes often contain static meshes as well as instanced or skinned meshes and in this case the global override material solution completely fails.

I recommend enabling the workaround.

@vanruesc vanruesc added external bug A bug that can't be fixed in this project and removed investigating A potential bug that requires further research labels Jul 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external bug A bug that can't be fixed in this project
Projects
None yet
Development

No branches or pull requests

3 participants