Page MenuHome

Fix T59915: Skin modifier produces inverted normals on end-cap faces when vertically aligned edge is assigned root
ClosedPublic

Authored by Sebastian Parborg (zeddb) on Dec 28 2018, 8:41 PM.

Details

Summary

Sometimes when the end caps pointed strait up (z axis) their faces would become inverted.

It seems like the code tried to rely on a certain vertex order to always happen. However this edge case did manage to produce an order that would produce inverted cap faces.
So now I introduce a normal check instead so we can be certain that the face will have the normal direction we want.

Diff Detail

Repository
rB Blender

Event Timeline

Jacques Lucke (JacquesLucke) requested changes to this revision.Mar 22 2019, 4:32 PM

That seems to solve the issue for me.

I don't know the code enough to know if this will break anything else though.

There is some normal related assert when start to modify the "Branch Smoothing" (when there is a branch in the source mesh). However, this might not be related.

source/blender/modifiers/intern/MOD_skin.c
528

The semantic might change when this is moved in the the else branch.

535

code style

This revision now requires changes to proceed.Mar 22 2019, 4:32 PM
source/blender/modifiers/intern/MOD_skin.c
528

I'm use sure what you mean with this comment. Can you elaborate?

source/blender/modifiers/intern/MOD_skin.c
528

*unsure

source/blender/modifiers/intern/MOD_skin.c
528

Previously this code:

if (nodes[v].flag & MVERT_SKIN_ROOT)
	skin_nodes[v].flag |= ROOT;

would be executed no matter what emap[v].count == 0 is.
With your change, it will only be executed when emap[v].count != 0.
I don't know if this has an impact.

Seems to work fine. :)

source/blender/modifiers/intern/MOD_skin.c
528

Right, that doesn't matter as the normal flip check only needs to be run on end frames.

Sebastian Parborg (zeddb) marked an inline comment as done.Apr 2 2019, 12:44 PM

Updated to master branch.

@Jacques Lucke (JacquesLucke) can you take a look at it again. There has been more reports regarding this issue. So it would be nice to have this merged.

This revision is now accepted and ready to land.Mon, Jun 17, 4:19 PM
This revision was automatically updated to reflect the committed changes.