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

VelocityDepthNormalMaterial incompatible with TransformControls from Three #23

Open
Nek opened this issue Aug 6, 2023 · 1 comment
Open
Labels
bug Something isn't working

Comments

@Nek
Copy link

Nek commented Aug 6, 2023

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 #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
Copy link
Author

Nek commented Aug 6, 2023

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.

@0beqz 0beqz added the bug Something isn't working label Aug 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants