Page MenuHome

Fix T95649: Corrective Smooth modifier can cause vertices to pop
AbandonedPublic

Authored by Campbell Barton (campbellbarton) on Apr 27 2022, 7:08 AM.

Details

Summary

Calculate a tangent per loop, apply the transformation in multiple spaces and accumulate the result weighting by the loop-angle.

Note that previously only the tangents Z axis (normal)
was properly accumulated, the X and Y axes only contained the values
from a single loop (the last one written to), meaning only two edges
contributed to the result. Now the transformation from all loops are
taken into account.


Note

  • In my own tests this resolves T95649 and gives more stable, less erratic results, although if geometry is deformed to flip faces ~180degrees in a small region - there is still the effect of "popping", which we can only do so much to resolve using this method of smoothing (as far as I can see).
  • Using polygons normal instead of the cross product of the two edge-vectors might be worth investigating further. The polygon normals are more stable avoiding popping when the corner of a face becomes concave (which was flipping the tangents direction).

    Some examples don't look so good though, so I'll keep that change out of the patch.

Diff Detail

Repository
rB Blender
Branch
TEMP-FIX-CORRECTIVE-SMOOTH-JUMP (branched from master)
Build Status
Buildable 23770
Build 23770: arc lint + arc unit

Event Timeline

Campbell Barton (campbellbarton) requested review of this revision.Apr 27 2022, 7:08 AM
Campbell Barton (campbellbarton) created this revision.
  • Remove the ifdef for polygon normals (kept for reference / testing early on)
  • Remove weight calculation from calc_tangent_loop (simpler to calculate outside this function)
  • Account for zero tangent weights
  • Account for zero weights (error in last update)
  • Disable polygon normals, this is a bigger change that can be tested separately from this patch.
  • Remove ifdef'd polygon normal use for tangents.

Won't this break backward compatibility? I'd consider the bound state to be data that requires compatibility, because rebinding in general requires conscious manual action, e.g. ensuring the character is definitely in rest pose etc, and can't be automatic.

Edit: e.g. when considering fixing this via a simple hack that simply removes that conditional, I contemplated adding a hidden flag whether the bind is new or 'legacy', and keeping the conditional as is in the legacy mode.

@Alexander Gavrilov (angavrilov) the binding isn't impacted by this patch. (delta's are run-time only and could even be removed from DNA as far as I can see).

Ah, I see, binding works by recording the rest vertex coordinates, not calculated deltas. I thought it's more similar to Surface Deform. Scratch that then.

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)
  • Simplify logic to make the tangent matrix orthogonal
Campbell Barton (campbellbarton) abandoned this revision.EditedSep 15 2022, 8:01 AM
Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)

Closing this patch as the main reason I posted it was to get user feedback and since it's been over 4 months without much interest, I think it's reasonable to commit the fix and in my own testing it gives more stable and generally better results.