0beqz / realism-effects

SSGI, Motion Blur, TRAA - Effects to enhance your three.js scene's realism
https://realism-effects-obeqz.vercel.app/
MIT License
1.43k stars 67 forks source link

VelocityDepthNormalMaterial incompatible with TransformControls from Three #23

Open Nek opened 1 year ago

Nek commented 1 year ago

Thanks for the great project. I've just started using it and spent a lot of time trying to integrate it with react-three-fiber (Success!). Now, I'm just having fun.

TL;DR

I have found the issue that covers this one while I was writing it. :/ Feel free to close. I still think it's good for future reference if somebody has the same issue.

The related issue is https://github.com/0beqz/realism-effects/blob/eb03a9e4a50bd7f0242bafb77d8e9b0c4c73543b/src/ssgi/utils/Utils.js#L238C24-L238C24

Wasted effort

First of all, I'm using realism-effects with react-three-fiber and in general it works just fine. I discovered an issue though, that is architectural.

VelocityDepthNormalPass does deep replacement of materials in the whole scene. That's quite an intrusive operation as it's impossible to tell if some user of replaced material relies on custom property that is went missing.

In my case it's "color" on one of the handlers of TransformControls. The code tries to clone a color without doing undefined checks.

I've temporary fixed it by adding "color" to

const materialProps = ["vertexTangent", "vertexColors", "vertexAlphas", "vertexUvs", "uvsVertexOnly", "supportsVertexTextures", "instancing", "instancingColor", "side", "flatShading", "skinning", "doubleSided", "flipSided"];

Proxy can be used to fix at least part of property access errors but it is still going to be brittle. In my opinion it's good to implement both approches.

Partly related https://github.com/0beqz/realism-effects/issues/14 as it will allow to exclude some of the offending objects in scene graph.

Relevant code

TransformControls: https://github.com/pmndrs/three-stdlib/blob/4c04593ee49bb0b022025718844f3ce2b21f67bf/src/controls/TransformControls.ts#L1301C40-L1301C40

ssgi/.../Util: https://github.com/0beqz/realism-effects/blob/eb03a9e4a50bd7f0242bafb77d8e9b0c4c73543b/src/ssgi/utils/Utils.js#L238C24-L238C24

Nek commented 1 year ago

Well, the relatively obvious issue with proxying that it creates the opposite problem and starts pulling in values of properties that shouldn't be available.