Page MenuHome

Fix T91064: Cycles low poly meshes having black edges when shade smoothed
ClosedPublic

Authored by Mikhail Matrosov (ktdfly) on Sep 5 2021, 1:17 AM.

Details

Summary

Fixes:T91064: Cycles Lower Poly Meshes Having Obvious Black Edges When Shade Smoothed

Caused by rBcd118c5581f4: Fix T90423: black pixels after shadow terminator geometry offset

  • Applies ensure_valid_reflection() to the normal input on all BSDFs for CPU and GPU.
  • This doesn't affect hair.
  • Removes ensure_valid_reflection() from the output of Bump Map and Normal Map nodes for CPU/GPU as it is not needed.
  • The fix doesn't touch OSL.

Diff Detail

Repository
rB Blender

Event Timeline

Mikhail Matrosov (ktdfly) requested review of this revision.Sep 5 2021, 1:17 AM
Mikhail Matrosov (ktdfly) created this revision.
Pratik Borhade (PratikPB2123) retitled this revision from Fix Cycles low poly meshes having black edges when shade smoothed to Fix T91064: Cycles low poly meshes having black edges when shade smoothed.
William Leeson (leesonw) requested changes to this revision.Sep 7 2021, 12:53 PM

Looks good, I'd just like an explanation for the *pdf = 0.2f; so that I understand it.

intern/cycles/kernel/closure/bsdf_principled_diffuse.h
43

Could you explain this better?

This revision now requires changes to proceed.Sep 7 2021, 12:53 PM
Brecht Van Lommel (brecht) requested changes to this revision.Sep 7 2021, 5:42 PM

The pdf value of 0.2 makes little sense to me. It's also not clear to me that doing this in the principled diffuse BSDF is enough, we also have the regular diffuse BSDF and other BSDFs.

My understanding is that what happens here is that we end up with a bumped/smooth normal such that the view direction lies outside of the hemisphere around that normal.

I think it may be better to use ensure_valid_reflection and adjust the normal to avoid that situation, and do it for all BSDFs instead of having each BSDF evaluation solve it on their own.

Mikhail Matrosov (ktdfly) edited the summary of this revision. (Show Details)

Adds ensure_valid_reflection(). Minor optimizations to the Diffuse BSDF I'll post as a separate patch.
Some Cycles tests don't pass, but I'm not sure if the result is actually worse then reference

A minor fix to clearcoat normal

William Leeson (leesonw) requested changes to this revision.Sep 27 2021, 2:30 PM

This revision looks good but I think you should keep your numerical change (I highlighted it in the comment) as it is way better than the current implementation IMHO.

intern/cycles/kernel/closure/bsdf_principled_diffuse.h
46–50

I think you should reinstate your original change for this at it has way better numerical properties due to the fewer number of dot products (a really good optimisation) and will give much better results.

This revision now requires changes to proceed.Sep 27 2021, 2:30 PM

Reinstated the original change for calculate_principled_diffuse_brdf()

Mikhail Matrosov (ktdfly) marked an inline comment as done.Sep 27 2021, 11:01 PM

The results look great and it appears to work. You just need to set the pdf value at the comment and this is ready to go.

intern/cycles/kernel/closure/bsdf_principled_diffuse.h
43

I think you still need the *pdf = 0.0f; here

@William Leeson (leesonw):

I think you still need the *pdf = 0.0f; here

In my experiments it was needed only for the case where dot(N, V) <= 0, but since N is ensured to be valid, it is not anymore.

You are correct, so no need to change this.

Brecht Van Lommel (brecht) requested changes to this revision.Sep 29 2021, 5:27 PM

Can we do ensure_valid_reflection only for triangle primitives? I'm seeing some changes in hair, but I don't think that needs to change since the shadow terminator offset is only for triangles.

Otherwise the results look good to me. There is a bit of a performance hit but I'm not seeing a way around it.

This revision now requires changes to proceed.Sep 29 2021, 5:27 PM
Mikhail Matrosov (ktdfly) edited the summary of this revision. (Show Details)
  • Disabled the fix for hair.
  • Removed OSL part of the fix, as it is actually not required.
This revision is now accepted and ready to land.Oct 4 2021, 12:36 PM