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

NormalPass, SSAOPass: Enable passes to work with flat shading, back side material settings #292

Closed
gkjohnson opened this issue May 9, 2021 · 6 comments
Labels
feature request New feature request

Comments

@gkjohnson
Copy link

Is your feature request related to a problem?

When setting a material to use "flatShading" or "BackSide" / "DoubleSide" SSAOPass does not currently work as expected because NormalPass does not respect those settings in the override material.

Describe the solution you'd like

NormalPass should use the "OverrideMaterialManager" and respect the original material "flatShading" and "side" properties.

Describe alternatives you've considered

None

Additional context

Here's a JSFiddle showing the current NormalPass results with the above material settings. The left cube uses "flatShading" with no normals while the right cube is using "side = BackSide":

https://jsfiddle.net/z4759qL8/1/

Thanks!

@gkjohnson gkjohnson added the feature request New feature request label May 9, 2021
@vanruesc
Copy link
Member

vanruesc commented May 9, 2021

Thanks for the report!

The NormalPass does make use of the OverrideMaterialManager and the side property should already be handled (see #265). The flatShading property is not supported yet. Handling this property will increase the amount of override materials from 9 to 18. This is, of course, awful, but I think this should be the last property that needs to be accounted for. The override material system is also about to become obsolete relatively soonish, now that the MRT PR has been merged.

I'll publish a fix for flatShading soon.

@vanruesc
Copy link
Member

vanruesc commented May 9, 2021

Modified fiddle with override material workaround enabled: https://jsfiddle.net/8georca4/

@gkjohnson
Copy link
Author

OverrideMaterialManager.workaroundEnabled = true;

Oops I missed that this needed to be explicitly enabled. Thanks!

The override material system is also about to become obsolete relatively soonish, now that the MRT PR has been merged.

I'm pretty excited for the improvements that MRT will bring but how do you anticipate it removing the need for an override workaround system? Three.js' scene.overrideMaterial will still have the same limitations so I would think you'd still have to use a similar kind of workaround. But now you'd be able to use an override material that draws to multiple buffers at once, instead. Maybe I've misunderstood, though!

@vanruesc
Copy link
Member

vanruesc commented May 9, 2021

how do you anticipate it removing the need for an override workaround system?

The workaround attempts to fix the shortcomings of the override material system, so it's only necessary because we use override materials. With MRT we should be able to render geometry data (normals, positions, roughness, etc.) into additional buffers during the initial RenderPass without having to re-render the scene with override materials. Since there are no other use cases for scene.overrideMaterial in this library, the NormalPass, DepthPass, LuminancePass and OverrideMaterialManager will eventually become obsolete.

Three's current built-in materials are quite rigid and hard to extend. I think we still have to wait for NodeMaterials and declarative shader manipulation features to actually make effective use of MRT in the RenderPass.

@vanruesc
Copy link
Member

vanruesc commented May 9, 2021

Fixed in postprocessing@6.21.5.

@gkjohnson
Copy link
Author

Three's current built-in materials are quite rigid and hard to extend. I think we still have to wait for NodeMaterials and declarative shader manipulation features to actually make effective use of MRT in the RenderPass.

Yes I'm excited to see what Node materials bring. I think they'll be pretty enabling in a lot of ways.

Thanks for the quick fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature request
Projects
None yet
Development

No branches or pull requests

2 participants