Page MenuHome

Loop Normals - Stage I
AbandonedPublic

Authored by Bastien Montagne (mont29) on Nov 18 2013, 9:59 PM.

Details

Summary

Basic loop normals support in Blender, and feature TSpace in py API for export addons.

This is current state of loop normals work. It is functionnal, but not yet polished - I'd rather get a first quick review over general design before spending more time to support all draw modes, polish the UI, etc.

What it does currently:

  • Allow py scripts to compute tspace (then available in each loops, the same way as for normals).
  • Add an (object-level) option to compute loop normals as a final step of DM creation, and use those both to create TSpace (when baking) and to draw sharp edges in viewport. The later only works with "modern" code currently (VBO enabled).

Diff Detail

Branch
loop_normals

Event Timeline

Bastien Montagne (mont29) updated this revision to Unknown Object (????).Dec 4 2013, 5:55 PM

Just updated the patch against latest master…

Bastien Montagne (mont29) updated this revision to Unknown Object (????).Dec 12 2013, 1:49 PM

Updated against current master...

Bastien Montagne (mont29) updated this revision to Unknown Object (????).Dec 29 2013, 10:04 PM

Updated patch against latest master again...

Checked over the patch, notes:

  • In general, seems like this is heading in the right direction...
  • Code lists TODO's - so this patch is WIP (ok, just noting)

User Point of View:

  • The term Loops does not show in the user interface currently and I think its best it stays an internal name (exactly what name am not so fussed...).
  • Having the option to use Loop Normals seems a bit strange to me, especially since we have another smoothing angle for the mesh (It seems silly to store 2 different smoothing angles and store one in the object, another in the mesh and put them in different places in the interface). I really don't think the ability to set smoothing angle per object is all that useful. How about group this option with Autosmooth? , eg Display Autosmooth / Viewport Autosmooth / ...or have an EYE icon and RENDER icon next to autosmooth number button (for viewport and render autosmootrh - matching what we have for modifiers and outliner). ... Exactly how to access smoothing we could leave open but think a good first step would be to store the toggle in the mesh and use the mesh smooth angle.

First pass code review:

Regarding moving forward with this patch:

Suggest following steps...

  • Split out tangent space RNA access into its own patch, This is fairly isolated and can be reviewed and approved separately, Exporters can use this irrespective of the ability to display loop normals.
  • It gets hard to keep track of customdata, when it should/shouldn't exist, to avoid having loop-normals allocated without anyone noticing - it would be good to add some asserts that make sure loop-normal/tangent layers are not there when the option is disabled. It should be possible to validate that no loop data exists when the option is disabled. Also ensure this data doesnt end up in the Mesh DNA, or EditMode data.
  • Some profiling, check how much more memory is used, how much slower object updates are, how much slower drawing is. - Good to be aware of any larger regressions in performance and see if they can be identified & addressed early on.

Other todo's probably are obvious but listing...

  • Get non-vbo display working.
  • Address XXX/TODO hacks in code.
  • Add BMesh access to loop normals. (Customdata layers and Py BMesh API).
  • Investigate having these normals replace render autosmooth (maybe nice if viewport and render shares same normal calculation method)... can be postponed of course.
source/blender/makesrna/intern/rna_mesh.c
1935

only computed on demand!, Is this right? - looks like its just copying from the CD layer.

source/blender/makesrna/intern/rna_mesh_api.c
106

This could be moved into a BKE_mesh_*** function.

Bastien Montagne (mont29) updated this revision to Unknown Object (????).Jan 7 2014, 8:00 PM

This is mostly an update to clarify patch, as tangent space stuff has been split off in its own diff.
So this one now depends on D185.

Appart from that, just renamed UI-wise "Loop Normal" to "Split Normal".

@Campbell Barton (campbellbarton) thanks a lot for that first review!

  • Add checks and do profiling: good point! :D
  • About render autosmooth: it is indeed my intention to remove this and use computed loop normals in renderers instead. However, I’d like to keep the setting at the object level, since later work on custom normals (modifiers at least) should also work for NURBS surfaces…
  • About BMesh: I’d like to keep this for later, if possible… I’m not yet quite sure how to expose/layout loop normals in edit world, so i’d rather first finalize Object-level code.

@Bastien Montagne (mont29) Thanks for the update & splitting diff,

Regarding auto-smooth being an object property, currently all smoothing settings are object-data so moving this into the object needs some very good reasoning/advantage - this kind of change should be well considered and not just a side-effect from other changes.

Not sure why custom normals / modifiers or nurbs surfaces would have anything to do with this.

And the overhead of having to manage smoothing between linked objects is a drawback too IMHO.

Its also worth considering exporters and rendering engines,

Currently you can write a mesh to a render engine - and as long as its got no modifiers or object materials (which is simple to check), you can assume that linked objects can be instances of the same mesh-data. With per object smoothing this means you would need to write a different mesh for each smoothing value and keep track of each mesh configuration. one _could_ argue that this is needed anyway, but this further complicates things for small/no benefit IMHO

I also think its just logical that this be an object data setting since it controls how the geometry is evaluate/treated.

Nurbs can just have a field added for smoothing angle as we have for meshes - if thats needed.

Bastien Montagne (mont29) updated this revision to Unknown Object (????).Jan 17 2014, 10:42 AM
  • Rename "Loop Normal" to "Split Normal" UI wise (could also use "Corner Normal", but it'd introduce a new, longer name, not worth it imho).
  • Fix some merging/rebase mess...
  • Remove split_normals from Object level, and use existing Mesh's autosmooth instead.
  • Some cleanup...
  • Add loopnormal support to legacy 3Dview OpenGL code as well (ie now works without the VBO option).
  • Add (limited) support for split normals in Blender Internal

Notes about last commit (BI): I'm only half-happy with this commit, not sure we'll keep this in the end... BI re-computes way too much things, esp. if it does (texture-based) displacement. In this case, there is no way for us to use loopnormals, and autosmooth falls back to previous default behavior. Not sure we want such half-way behavior, and I don't think it's worth double-implementing again the split normals code into BI, especially as we do not have edge sharp flag here. :/

@Campbell Barton (campbellbarton): yeah, looks like I missed quite a bit of points about that splinormals at object level… Moved it back to reuse autosmooth settings for now (may have to add a new flag to control it, though, if we decide to not keep last commit about BI support for split normals).

@Brecht Van Lommel (brecht): I’d like very much to get your advice about that "BI supports loopnormals" part of the patch… ;)

source/blender/render/intern/source/convertblender.c
612

Woops, forgot to add the check here (returned value may be -1 in invalid cases)… sorry.

3556

Should be ||, not && :/

source/blender/render/intern/source/convertblender.c
3560–3562

…and I should free loop_nors, too… tsst.

Brecht Van Lommel (brecht) requested changes to this revision.Jan 17 2014, 3:22 PM

Checked the render code. I think we should get rid of the old autosmooth code, that might also simplify the loop normal code here.

source/blender/blenloader/intern/versioning_260.c
2732

This needs a subversion bump to avoid breaking existing files on repeated saves.

source/blender/render/intern/source/convertblender.c
3195–3202

do_disp => do_displace

3552–3555

Can we get rid of the old autosmooth code somehow? Seems a bit silly to have two implementations of the same thing. I'm guessing no one actually uses displacement + autosmooth, I've never seen it anyway, it likely only generates problems.

source/blender/render/intern/source/convertblender.c
3195–3202

Ehhh… do_displace is already the name of the function… we could rename the func displace then?

3552–3555

If you think this is OK, I’ll do it gladly! ;)

Bastien Montagne (mont29) updated this revision to Unknown Object (????).Jan 18 2014, 10:41 AM

Simplified displace/autosmooth interactions in BI (now, when doing displace, there is no autosmooth!), added version bump, and some profiling prints in final DM normals/tesselation computation.

  • Rename "Loop Normal" to "Split Normal" UI wise (could also use "Corner Normal", but it'd introduce a new, longer name, not worth it imho).
  • Fix some merging/rebase mess...
  • Remove split_normals from Object level, and use existing Mesh's autosmooth instead.
  • Some cleanup...
  • Add loopnormal support to legacy 3Dview OpenGL code as well (ie now works without the VBO option).
  • Add (limited) support for split normals in Blender Internal
  • Add some timing prints (for dev only, to be removed in final patch!).
  • BI displace/autosmooth simplification: now we only do autosmooth if we do not have any displace texture.
  • Bump subversion...
  • Attempt to speed up loopnormals computing. Main bottleneck is tessdata update, see D226.
release/scripts/startup/bl_ui/space_view3d.py
2769

Since this is for shading I can see why its here but this also effects renders... think it could be better to keep these settings in properties window.

source/blender/blenkernel/BKE_cdderivedmesh.h
98

these kinds of changes you could just commit to master, they don't need to be reviewed and increase chance patch conflicts later.

source/blender/blenkernel/intern/DerivedMesh.c
2019

*picky*, (Mesh *) use space before pointer.

2261

Are you sure this is ok? - the derivedFinal isn't just used for drawing. (vertex paint for example uses normals from the derived mesh), Need to be careful if v->no is going to be left unset. Checked and looks like DM_calc_loop_normals isnt setting them.

source/blender/blenkernel/intern/customdata.c
1174

Think it could be worth storing these as short[3] (24bytes rather then 48). Since they aren't ever manipulated and are only for display.

source/blender/editors/space_view3d/drawobject.c
3392

*picky* extra ()

release/scripts/startup/bl_ui/space_view3d.py
2769

I do not have strong opinion on this for final (master-ready) patch, but for now I prefer to keep them here (easier for tests to have it in 3DView ;) ).

source/blender/blenkernel/BKE_cdderivedmesh.h
98

Done.

source/blender/blenkernel/intern/DerivedMesh.c
2261

Well, it should at least (DM_calc_loop_normals calls BKE_mesh_calc_normals_poly, which will compute also vertex normals unless DM_DIRTY_NORMALS is not set).

Bastien Montagne (mont29) updated this revision to Unknown Object (????).Jan 23 2014, 4:53 PM
  • Fixes & updates for recent changes in master (mostly regarding tessellation).
  • Minor cleanup from campbell's review
  • Switch tessellated loop normals to shorts instead of floats (suggested by Campbell, thx).
  • Add loop normals support to subsurf OGL draw code...

Note since tessellated code has been optimized, final DM code is about two times faster when using loop normals (yet it remains about three to four times slower than without loop normals, as tessellation gain benefits to both cases).

I still have to make some checks in this area, but appart from those performances issues, I think "Object mode" part of the patch is becoming rather good. Main work remains to have a better/cleaner support of edit mode (and add asserts where needed to check this temp data does not leak into saved stuff, too).

I was just poking around the Blender patch tracker and am taking potshots at variable names. Don' be a hater :)

source/blender/blenkernel/intern/cdderivedmesh.c
947–964

At the risk of starting a war. Surely there is a better name than "lnors"?

source/blender/gpu/intern/gpu_buffers.c
714

tlnor?

How about

tessellated_loop_normals?

/me ducks

tlnors is obscure...

but we already have tessface as common name. how about... tessface_lnors, tessloopnors ?

source/blender/blenkernel/intern/DerivedMesh.c
2030

This really should be resolved before merging to master I think. is there much problem with adding support for this?

@Campbell Barton (campbellbarton): This definitively has to be resolved in master-ready patch, for sure! In fact, EditMode is the main last remaining WIP part.

In fact, I’m still thinking about how to handle (store) lnors in editmesh, as ultimately I'd like to get real custom ones. My current idea would be to only store one lnor per smoothgroup of faces and per vertex (smoothgroups = groups of faces defined by sharp edges and/or flat faces, for each vertex). Then you add an index mapping (loop_idx -> lnors).

The benefit would be double:

  • less mem usage in most common cases (as esp. in big meshes, most vertex will be in the middle of smoothed faces, so for quad grid, that would make 1 lnor + 4 uints for 4 loops, 28 bytes instead of 64 bytes per vertex...).
  • Easier to handle nicely custom lnors (has a diff from default lnors, in a tangent-like space generated from the whole smoothgroup) - and less mem usage here as well.

Tbh, I’m even thinking to switch the whole lnor CDLayer to this system, even for object mode (and only keep one lnor per loop for tessellated data)...

Not sure yet about the performance/complexity tradeoff this would involve, have to make some tests.

Bastien Montagne (mont29) updated this revision to Unknown Object (????).Feb 19 2014, 4:56 PM

Mostly added basic split normals support to BMesh and EditDerivedBMesh...

Main issue that bothers me about it is the need to have valid indices for BM_LOOP as well, added a basic code to to so, until I get agreement this is OK to implement.

Bastien Montagne (mont29) updated this revision to Unknown Object (????).Feb 27 2014, 5:18 PM
  • Fix BMesh dirty loop indices hack.
  • Add access to tessellated loop normals (for renderers like cycles...).
  • Add support of split (loop) normals to Cycles.

Ok, so this patch was becoming insane, too big, too much different areas affected... So I split it in five pieces:

  • loop_normals_basis (D365): Basic split normals handling in non-edit mesh/DM code.
  • loop_normals_bmesh (D366): Handle split normals in BMesh/EDBM code (depends on D365).
  • loop_normals_3dview (D367): Handle split normals in 3DView drawing (depends on D365 and D366).
  • loop_normals_renderer_bi (D368): Handle split normals in Blender Internal rendering (depends on D365 and D366).
  • loop_normals_renderer_cycles (D369): Handle split normals in Cycles rendering (depends on D365 and D366).

Note I keep this diff, as it allows to apply everything at once (code is not 100% the same as in branches, but differences should not affect behavior of new features).

Committed sub-patches.