Normal Editing Tools - GSoC 2017
Needs ReviewPublic

Authored by Rohan Rathi (RohanRathi) on Oct 24 2017, 4:18 PM.

Details

Summary

Contains all work done on Normal Editing Tools in GSoC 2017.

The revision contains work from my branch 'soc-2017-normal-tools'.

Diff Detail

Repository
rB Blender
Branch
soc-2017-normal-tools
Build Status
Buildable 1384
Build 1384: arc lint + arc unit
Rohan Rathi (RohanRathi) retitled this revision from This is a revision of the work done on Normal Editing in GSoC 2017. to Normal Editing Tools - GSoC 2017.Oct 24 2017, 4:27 PM
Rohan Rathi (RohanRathi) edited the summary of this revision. (Show Details)
Bastien Montagne (mont29) requested changes to this revision.Nov 9 2017, 11:13 PM

Started more indepth review & some cleanup/refactor in the branch… So far looks good, aside from lots of tiny details, main changes I did was to store BMLoop pointers themselves in lnorspace array, instead of loop indices, when generated in BMesh context. Referring to items from their indices really is not the “BMesh way to go”, even worse in case of loops since we do not have the mapping for those…

Changes so far are less huge than I was afraid, but still, would be cool if you could double-check I did not break anything on the run.

Also, there is an issue currently, when starting e.g. normal rotate on a mesh which clnors are partly merged, partly split, depending on angle between faces (autosmooth feature, try e.g. newly added Monkey, smooth shading, with autosmooth set to 30°), All normals are merged when starting rotate normals operation. I think you do not always properly check/pass that angle when generating lnor_spacearr? Just found the issue, so no time to investigate on it yet, would be cool if you could solve it and update the diff before I keep on my cleaning rampage. ;)

release/scripts/startup/bl_ui/properties_data_modifier.py
1552–1566

We can make better (more compact) UI here… will commit a new proposal.

release/scripts/startup/bl_ui/space_view3d_toolbar.py
454–462

Think those should also go in the new Normals Tools panel, will commit.

465–512

Please use same (spaces) indentation as everywhere else in file. ;)
(good trick is to make space-like chars visible in your editor).

source/blender/blenkernel/BKE_mesh.h
198

Since it becomes 'public' API, should be renamed to something like BKE_mesh_loop_manifold_fan_around_vert_next. Will commit.

source/blender/bmesh/intern/bmesh_mesh.c
1086–1110

That kind of double-looping is really horrible… and realized we can easily get rid of those by storing BMLoop pointers instead of loopindices for BMesh lnorspace arrays... Will commit the change. ;)

source/blender/bmesh/intern/bmesh_mesh.h
55

Should be something like const bool do_rebuild (we try to always ue either use_ or do_ prefixes for booleans). Will commit.

56

Should also be renamed to something like BM_loop_check_cyclic_smooth_fan I guess… Will commit.

source/blender/editors/mesh/editmesh_tools.c
6364

Why ??? Where are you ever allocating memory for this? Afaict, you only even assign to it some vertex' coordinates, am pretty sure you never want to free that here?

6366–6367

That’s really the kind of stuff to be factorized in a function... will commit.

6775

Uuuhhhhh… modifying external data you do not really know whether it might be used again or not… rather use loop temp flag here!

6776

That’s another horrible loop-inside-loop, we can avoid it by storing a mapping lidx->transdata… But at the very least there should have been a break here! ;)

source/blender/editors/transform/transform.c
1473–1488

Missing break here (fixed in branch).

This revision now requires changes to proceed.Nov 9 2017, 11:13 PM

Cleanup
Fixed the memory allocation in point_normals, a few bugs and made custom normals work with auto smooth.

Finished heavy refactor of the 'keep sharp' case for weighted modifier now, it will now take all 'sharp edge' factors into account like every where else in code (i.e. sharp edges themselves, but also flat faces and split angle setting of mesh is needed).

I am still a bit on the fence regarding the 'weight' stuff. As far as I understand that code, it is doing some kind of exponential (or inverted exponential) correction, using the sorted list of 'values' to increase or decrease correction factor in relation with the values importance.

I may be wrong, but I have a strong feeling that this is a rather doggy process, not very predictable what importance which face will get in the end (when weight setting is not at its neutral 50 value). I even think that a same (square, so same value for all its loops) face can get different importances for each of its loops depending on surrounding geometry, correct?

Why not simply normalize the face/loop values (for angle it’s dead simple, for face area we'd need to store max area and do another loop to divide by it), and apply basic exponential correction? Or maybe even not normalize them at all, just apply powf(val, corrective_factor) ?

Bastien Montagne (mont29) requested changes to this revision.Mar 13 2018, 10:56 AM

OK, so besides concerns noted in previous comment regarding how weighting is handled in weighted normals modifier, and the ones below regarding rotation of normals in transform code, think I’m done here. Once those two are addressed, we'll ask Campbell to check on bmesh changes (and maybe transform ones too), but think we are now quite close to merge point now. :)

Regarding transform, I sort of have the feeling you copied code handling various center points for rotation - but in case of normals, I don’t think any other option than 'individual center' (i.e. normal’s vertex) would make sense at all? So since our normals are already normalized, we can just transform them with the rotation matrix and we are done.

This should also simplify a bit the BMLoopNorEditData struct, since we can get rid of its mtx and smtx matrices?

source/blender/editors/transform/transform.c
4329–4342

Afaict (I don’t like matrices that much :P), all this is useless, and can be replaced by this single line?

		mul_v3_m3v3(lnor_ed->nloc, mat, lnor_ed->niloc);
4333–4334

Mere copy, smat ends up being same as mat (unless am mistaken)…

4336

Mere copy, center is zero vec!

4339

Mere copy, center is zero vec!

4341–4342

A nice, plain NOP operation :P

vec = nloc - niloc;
nloc = niloc + vec = niloc + nloc - niloc = nloc;
This revision now requires changes to proceed.Mar 13 2018, 10:56 AM

Changed rotate normal computation

I understand that the current exponential weighting system is incessantly computation intensive. Efficient calculating the corrective factor also needs to be taken into account.

I may be wrong, but I have a strong feeling that this is a rather doggy process, not very predictable what importance which face will get in the end (when weight setting is not at its neutral 50 value). I even think that a same (square, so same value for all its loops) face can get different importances for each of its loops depending on surrounding geometry, correct?

Yes, the face can have different importance for loops dependent on adjacent faces. The current method works very similar to YAVNE.

Why not simply normalize the face/loop values (for angle it’s dead simple, for face area we'd need to store max area and do another loop to divide by it), and apply basic exponential correction? Or maybe even not normalize them at all, just apply powf(val, corrective_factor) ?

One thing I have in mind is to find out avg area of faces around each vert and use the current (poly area / avg area) as corrective factor. With final normal being: Summation (normal[3] * powf(corrective factor, weight))

Hi

I want to comment a few bugs added since @Bastien Montagne (mont29) refactor, these problems don't appear in 9d3ad0eb5af7ab4f3fe28820f1e419e2f69c7cb9

  • When you add a weighted normal modifier and activate the sharp edges, in some conditions, Blender automatically add sharp edges in edges that have an angle superior to the autosmooth value. This is unpredictable behavior and the correct behaviour was the original, ignore the angle value from autosmooth and don't add sharp edges to the mesh.

    To reproduce the problem: Create a suzanne object > shade smooth > activate autosmooth > enter edit mode > show sharp edges > add weighted normal modifier > activate sharp edges in modifier > go to object mode > enter edit mode

  • Face influence have bad behaviours, at least in some triangles. And activate sharp edges give unexpected results in border normals, even when the mesh does not have sharp edges.

In this case the border ngons have weak value and center triangles are medium value (left object). The actual modifier give a strange solution when in triangles, when if you compare with right object with correct behaviour. Also you can compare the difference in the normal of the top vertex of the model when you activate sharp edges, if you compare with righ object (correct behaviour)

And here the behaviour of the modifier before the refactor, with all cases working well.

Thanks for the feedback, fixed the first issue already (easy stupid one, rB3b01edd52e8e).

Still checking on the second one, was introduced by rB93ef006e3c.

OK, second issue is caused by addition of check for too small normal vectors (which typically indicate degenerated cases, and are hence 'reset' to default vertex normal). Think 1e-4f is a bit of an aggressive threshold actually, reducing it to more common 1e-6 value seems to fix reported issue and should probably still be OK. Will commit the change (rB2da3949e26f), thanks again for the feedback.