Normal Editing Tools - GSoC 2017
Needs RevisionPublic

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



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

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

Diff Detail

rB Blender
Build Status
Buildable 923
Build 923: 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. ;)


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


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


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


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


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. ;)


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


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


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?


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


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


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! ;)


Missing break here (fixed in branch).

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