Page MenuHome

Solidify Modifier Extension
Needs ReviewPublic

Authored by Henrik Dick (weasel) on Wed, Sep 11, 9:17 PM.
Tokens
"Love" token, awarded by bnzs."Love" token, awarded by michaelknubben."Love" token, awarded by MetinSeven."Love" token, awarded by Frozen_Death_Knight."Like" token, awarded by capnm."Love" token, awarded by brilliant_ape."Love" token, awarded by amonpaike."Like" token, awarded by duarteframos."Love" token, awarded by semaphore."Love" token, awarded by juang3d."Love" token, awarded by wo262.

Details

Reviewers
Campbell Barton (campbellbarton)
Group Reviewers
Modifiers
Summary

I added new features to the solidify modifier.

  1. Clamp with respect to angles
  2. New Manifold Solidify mode to solidify non manifold geometry into manifold geometry

The C code is roughly based on my temporary addon script at:
https://github.com/redweasel/manifold-solidify
It is rewritten though and a lot of bugs from the script are fixed.
Also the fix intersections feature isn't implemented yet, but I will
likely implement it sometime and add a new revision then.

Diff Detail

Repository
rB Blender
Branch
arcpatch-D5766 (branched from master)
Build Status
Buildable 5054
Build 5054: arc lint + arc unit

Event Timeline

Henrik Dick (weasel) edited the summary of this revision. (Show Details)Wed, Sep 11, 9:42 PM

changed the obsolete designated initializers to standard syntax

fixed normalized vertex normals for curves

fixed a bug where very complicated geometry would create wrong loops resulting in all sorts of failure

Is it possible to split up the functions and add comment of what they are doing to make it easier to review the code and its logic (and, in the future, make it more maintainable) ?

release/scripts/startup/bl_ui/properties_data_modifier.py
992–993

Dead code is highly discouraged.

Overall functionality is very nice, I think users will find this option useful.

I didn't do an in-depth code review, some notes from testing.

  • Works well with low poly shapes, for complex meshes it can give spikes.
  • The limitation: "No duplicates allowed for manifold solidify" would be nice to remove. For animation it's possible there will be degenerate geometry at some moment in time and it would be a shame if solidify breaks someones animation because it couldn't skip this geometry or use a fallback. It may be difficult to track this error down with more complex modifier stacks too.
  • Shouldn't it should be possible to support the "Offset" option?
  • Crashes with isolated vertices .
source/blender/modifiers/intern/MOD_solidify.c
273

This isn't needed, empty input can silently do nothing, errors should only be used in the case the output fails to do what the user would expect.

Campbell Barton (campbellbarton) requested changes to this revision.Sun, Sep 15, 10:03 AM
This revision now requires changes to proceed.Sun, Sep 15, 10:03 AM

Ok I have fixed all the bugs I found and added a lot of code to support duplicates. Also I added the offset property that was missing. I have a file with my test objects that I will add to this comment.

Campbell Barton (campbellbarton) requested changes to this revision.Tue, Sep 17, 8:43 AM
  • The "Only Rim" option works differently to the current extrude method (doesn't keep the original shell).b
  • This is asserting on the attached file (play the animation, crashes on the last frame)

  • Naming of variables is a bit messy: num / tot / count / len / lens are used in a confusing way, suggest:
    • _len is used for array lengths.
    • _length / _lengths is used for single, plural distances.
source/blender/modifiers/intern/MOD_solidify.c
350

No need for bit-shifting here (less readable), use multiply by two instead. Applies to other uses of bit-shifting too.

450

This epsilon is most likely too small, for 32bit floats we normally use epsilons around 1e-6 to 1e-10 - see: https://stackoverflow.com/a/50885022/432509

Also, this would be better defined at the top of the file, eg:

const float edge_length_sq_epsilon = 1e-8f;

496

Best set this to NULL, make it clear if freed memory is accessed later on.

1221

use float (*normals_queue)[3] for readability.

1243

Use face_nors[3][3], makes the code below more readable.

1386

Is there a chance this could be 4 or more? If this should never happen, add: default: BLI_assert(0);

1410–1422

There are macros ME_POLY_LOOP_NEXT / ME_POLY_LOOP_PREV, although - it's more efficient to make these part of the for loop.

See: BKE_crazyspace_set_quats_mesh.

1517

Again, best declare epsilons at the top of the function and re-use, this one seems fairly large.

1651–1653

Mixing of bool and short here looks incorrect.

2604

Would prefer use int pairs for readability unsigned int (*edge_user_pairs)[2]

This revision now requires changes to proceed.Tue, Sep 17, 8:43 AM
source/blender/makesrna/intern/rna_modifier.c
4101

use use_ prefix for booleans.

4161

use use_ prefix for booleans.

Henrik Dick (weasel) updated this revision to Diff 18294.EditedTue, Sep 17, 3:58 PM

made naming more understandable, applied all the other suggestions from above.

Also your provided blend file does not cause an assertion error. If I set the first modifier to constraints though it does strangly cause an assertion error with wrong loop len. I can't seem to see the problem in the code yet though... (Its obviously a problem in the mesh result from the first modifier and the second modifier produces the error)

As you pointed out Only Rim does something different in Manifold Mode than in the default Extrude Mode. That is so because in Manifold Mode no original geometry is kept anyway. I think it is correct to leave it like this, because it also does exactly what it says. In cases where you want the other behaviour you will most certainly not have non manifold geometry and you would use Extrude Mode anyway.

I found another bug with even method not working correctly anymore. I am investigating that right now.

Henrik Dick (weasel) marked 13 inline comments as done.Tue, Sep 17, 11:38 PM
Henrik Dick (weasel) updated this revision to Diff 18311.

improved naming further, used a few more makros, fixed constraints calculation for offset != 0. Need to fix even and simple method for offset != 0 as well but had no time for that yet. The error in solidify_crash.blend is still unresolved.

Henrik Dick (weasel) updated this revision to Diff 18347.EditedWed, Sep 18, 8:22 PM

fixed all sort of linear algebra mistakes and cleaned up the angle calculation.

As I was fixing the even and simple method I noticed, that they will always give worse results than the default constraints method. I think we should still include them, because they are faster to compute (for animation) and they can in some cases look more like what the artist wants, but thats open for discussion here: https://devtalk.blender.org/t/new-solidify-modifier/9192/21

Cleanup: use 'uint' instead of 'unsigned int', wrap struct declarations using trailing comma

Minor cleanup:

  • Use 'uint' instead of 'unsigned int'
  • Wrap struct declarations (adding trailing comma)

Declare iterators in for loops where possible

Think this is close to completion.

  • I'm still getting an assert in (frame 10).

    BLI_assert failed: /src/blender/source/blender/modifiers/intern/MOD_solidify.c:1977, manifold_solidify(), at 'new_edge->old_edge == orig_mloop[loopstart + j].e'

As you pointed out Only Rim does something different in Manifold Mode than in the default Extrude Mode. That is so because in Manifold Mode no original geometry is kept anyway. I think it is correct to leave it like this, because it also does exactly what it says. In cases where you want the other behaviour you will most certainly not have non manifold geometry and you would use Extrude Mode anyway.

While technically this is true, from a user perspective we have the use case of not needing to create an entire duplicate shell, yet still warning rims the the appearance of being solid the way this works now isn't very useful (Only making rims with no inner/outer surface).

source/blender/modifiers/intern/MOD_solidify.c
1617–1618

The clamping could be checked before calling cosf.

Henrik Dick (weasel) updated this revision to Diff 18376.EditedThu, Sep 19, 9:03 PM
  • fixed assertion error (had an outdated condition)
  • fixed size of vert_adj_edges (was numEdges instead of numVerts) and fix for isolated verts
  • added clamping for cosf everywhere where I used it
Henrik Dick (weasel) marked an inline comment as done.Thu, Sep 19, 9:59 PM
Henrik Dick (weasel) updated this revision to Diff 18377.
  • removed clamps for cosf in confusion why I ever added them (cosf is defined for all finite values of float)
  • cleaned up code a little
  • fixed isolated verts in result for only rim
  • check for offset == 0 for extrude mode
  • there is a segmentation fault that occurs sometimes when using extrude, even thickness, angle clamp. It seems to appear at no specific point, just after using it a while (like for animation)
Henrik Dick (weasel) updated this revision to Diff 18397.EditedSat, Sep 21, 12:23 PM
  • clamped mat_nr (could go out of range before)
  • fixed clamp angle segmentation fault

I think the revision is finished now. I didn't find any more bugs.

Running into a crash with the current patch:

Open this file and play it, frame 1 crashes (other frames crash too, jump to any frame and play).

This is the asan output.

1==24947==ERROR: AddressSanitizer: heap-use-after-free on address 0x61100012b288 at pc 0x5561cda00e42 bp 0x7f4ab63e5a00 sp 0x7f4ab63e59f0
2READ of size 8 at 0x61100012b288 thread T15
3 #0 0x5561cda00e41 in manifold_solidify /src/blender/source/blender/modifiers/intern/MOD_solidify.c:1137
4 #1 0x5561cda300b8 in applyModifier /src/blender/source/blender/modifiers/intern/MOD_solidify.c:3044
5 #2 0x5561cb1a5ad4 in modwrap_applyModifier /src/blender/source/blender/blenkernel/intern/modifier.c:945
6 #3 0x5561cb7f22b8 in mesh_calc_modifiers /src/blender/source/blender/blenkernel/intern/DerivedMesh.c:1208
7 #4 0x5561cb7faa40 in mesh_build_data /src/blender/source/blender/blenkernel/intern/DerivedMesh.c:1844
8 #5 0x5561cb7fc379 in makeDerivedMesh /src/blender/source/blender/blenkernel/intern/DerivedMesh.c:1973
9 #6 0x5561cb2da70a in BKE_object_handle_data_update /src/blender/source/blender/blenkernel/intern/object_update.c:187
10 #7 0x5561cb2dd852 in BKE_object_eval_uber_data /src/blender/source/blender/blenkernel/intern/object_update.c:358
11 #8 0x5561cbd33b31 in void std::__invoke_impl<void, void (*&)(Depsgraph*, Scene*, Object*), Depsgraph*, Scene*&, Object*&>(std::__invoke_other, void (*&)(Depsgraph*, Scene*, Object*), Depsgraph*&&, Scene*&, Object*&) /usr/include/c++/9.1.0/bits/invoke.h:60
12 #9 0x5561cbd2db59 in std::__invoke_result<void (*&)(Depsgraph*, Scene*, Object*), Depsgraph*, Scene*&, Object*&>::type std::__invoke<void (*&)(Depsgraph*, Scene*, Object*), Depsgraph*, Scene*&, Object*&>(void (*&)(Depsgraph*, Scene*, Object*), Depsgraph*&&, Scene*&, Object*&) /usr/include/c++/9.1.0/bits/invoke.h:95
13 #10 0x5561cbd2538c in void std::_Bind<void (*(std::_Placeholder<1>, Scene*, Object*))(Depsgraph*, Scene*, Object*)>::__call<void, Depsgraph*&&, 0ul, 1ul, 2ul>(std::tuple<Depsgraph*&&>&&, std::_Index_tuple<0ul, 1ul, 2ul>) /usr/include/c++/9.1.0/functional:400
14 #11 0x5561cbd1aa06 in void std::_Bind<void (*(std::_Placeholder<1>, Scene*, Object*))(Depsgraph*, Scene*, Object*)>::operator()<Depsgraph*, void>(Depsgraph*&&) /usr/include/c++/9.1.0/functional:484
15 #12 0x5561cbd0f956 in std::_Function_handler<void (Depsgraph*), std::_Bind<void (*(std::_Placeholder<1>, Scene*, Object*))(Depsgraph*, Scene*, Object*)> >::_M_invoke(std::_Any_data const&, Depsgraph*&&) /usr/include/c++/9.1.0/bits/std_function.h:300
16 #13 0x5561cbd8cba2 in std::function<void (Depsgraph*)>::operator()(Depsgraph*) const /usr/include/c++/9.1.0/bits/std_function.h:690
17 #14 0x5561cbd88e4d in deg_task_run_func /src/blender/source/blender/depsgraph/intern/eval/deg_eval.cc:86
18 #15 0x5561cbc80918 in task_scheduler_thread_run /src/blender/source/blender/blenlib/intern/task.c:450
19 #16 0x7f4ada5b157e in start_thread (/usr/lib/libpthread.so.0+0x957e)
20 #17 0x7f4ad99c40e2 in __clone (/usr/lib/libc.so.6+0xfc0e2)
21
220x61100012b288 is located 8 bytes inside of 208-byte region [0x61100012b280,0x61100012b350)
23freed by thread T15 here:
24 #0 0x7f4adb6386c0 in __interceptor_free /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:122
25 #1 0x5561cbf1795e in MEM_lockfree_freeN /src/blender/intern/guardedalloc/intern/mallocn_lockfree_impl.c:157
26 #2 0x5561cda00c1c in manifold_solidify /src/blender/source/blender/modifiers/intern/MOD_solidify.c:1117
27 #3 0x5561cda300b8 in applyModifier /src/blender/source/blender/modifiers/intern/MOD_solidify.c:3044
28 #4 0x5561cb1a5ad4 in modwrap_applyModifier /src/blender/source/blender/blenkernel/intern/modifier.c:945
29 #5 0x5561cb7f22b8 in mesh_calc_modifiers /src/blender/source/blender/blenkernel/intern/DerivedMesh.c:1208
30 #6 0x5561cb7faa40 in mesh_build_data /src/blender/source/blender/blenkernel/intern/DerivedMesh.c:1844
31 #7 0x5561cb7fc379 in makeDerivedMesh /src/blender/source/blender/blenkernel/intern/DerivedMesh.c:1973
32 #8 0x5561cb2da70a in BKE_object_handle_data_update /src/blender/source/blender/blenkernel/intern/object_update.c:187
33 #9 0x5561cb2dd852 in BKE_object_eval_uber_data /src/blender/source/blender/blenkernel/intern/object_update.c:358
34 #10 0x5561cbd33b31 in void std::__invoke_impl<void, void (*&)(Depsgraph*, Scene*, Object*), Depsgraph*, Scene*&, Object*&>(std::__invoke_other, void (*&)(Depsgraph*, Scene*, Object*), Depsgraph*&&, Scene*&, Object*&) /usr/include/c++/9.1.0/bits/invoke.h:60
35 #11 0x5561cbd2db59 in std::__invoke_result<void (*&)(Depsgraph*, Scene*, Object*), Depsgraph*, Scene*&, Object*&>::type std::__invoke<void (*&)(Depsgraph*, Scene*, Object*), Depsgraph*, Scene*&, Object*&>(void (*&)(Depsgraph*, Scene*, Object*), Depsgraph*&&, Scene*&, Object*&) /usr/include/c++/9.1.0/bits/invoke.h:95
36 #12 0x5561cbd2538c in void std::_Bind<void (*(std::_Placeholder<1>, Scene*, Object*))(Depsgraph*, Scene*, Object*)>::__call<void, Depsgraph*&&, 0ul, 1ul, 2ul>(std::tuple<Depsgraph*&&>&&, std::_Index_tuple<0ul, 1ul, 2ul>) /usr/include/c++/9.1.0/functional:400
37 #13 0x5561cbd1aa06 in void std::_Bind<void (*(std::_Placeholder<1>, Scene*, Object*))(Depsgraph*, Scene*, Object*)>::operator()<Depsgraph*, void>(Depsgraph*&&) /usr/include/c++/9.1.0/functional:484
38 #14 0x5561cbd0f956 in std::_Function_handler<void (Depsgraph*), std::_Bind<void (*(std::_Placeholder<1>, Scene*, Object*))(Depsgraph*, Scene*, Object*)> >::_M_invoke(std::_Any_data const&, Depsgraph*&&) /usr/include/c++/9.1.0/bits/std_function.h:300
39 #15 0x5561cbd8cba2 in std::function<void (Depsgraph*)>::operator()(Depsgraph*) const /usr/include/c++/9.1.0/bits/std_function.h:690
40 #16 0x5561cbd88e4d in deg_task_run_func /src/blender/source/blender/depsgraph/intern/eval/deg_eval.cc:86
41 #17 0x5561cbc80918 in task_scheduler_thread_run /src/blender/source/blender/blenlib/intern/task.c:450
42 #18 0x7f4ada5b157e in start_thread (/usr/lib/libpthread.so.0+0x957e)
43
44previously allocated by thread T15 here:
45 #0 0x7f4adb638ce8 in __interceptor_calloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:153
46 #1 0x5561cbf18108 in MEM_lockfree_callocN /src/blender/intern/guardedalloc/intern/mallocn_lockfree_impl.c:267
47 #2 0x5561cbf183f3 in MEM_lockfree_calloc_arrayN /src/blender/intern/guardedalloc/intern/mallocn_lockfree_impl.c:299
48 #3 0x5561cd9fbe81 in manifold_solidify /src/blender/source/blender/modifiers/intern/MOD_solidify.c:892
49 #4 0x5561cda300b8 in applyModifier /src/blender/source/blender/modifiers/intern/MOD_solidify.c:3044
50 #5 0x5561cb1a5ad4 in modwrap_applyModifier /src/blender/source/blender/blenkernel/intern/modifier.c:945
51 #6 0x5561cb7f22b8 in mesh_calc_modifiers /src/blender/source/blender/blenkernel/intern/DerivedMesh.c:1208
52 #7 0x5561cb7faa40 in mesh_build_data /src/blender/source/blender/blenkernel/intern/DerivedMesh.c:1844
53 #8 0x5561cb7fc379 in makeDerivedMesh /src/blender/source/blender/blenkernel/intern/DerivedMesh.c:1973
54 #9 0x5561cb2da70a in BKE_object_handle_data_update /src/blender/source/blender/blenkernel/intern/object_update.c:187
55 #10 0x5561cb2dd852 in BKE_object_eval_uber_data /src/blender/source/blender/blenkernel/intern/object_update.c:358
56 #11 0x5561cbd33b31 in void std::__invoke_impl<void, void (*&)(Depsgraph*, Scene*, Object*), Depsgraph*, Scene*&, Object*&>(std::__invoke_other, void (*&)(Depsgraph*, Scene*, Object*), Depsgraph*&&, Scene*&, Object*&) /usr/include/c++/9.1.0/bits/invoke.h:60
57 #12 0x5561cbd2db59 in std::__invoke_result<void (*&)(Depsgraph*, Scene*, Object*), Depsgraph*, Scene*&, Object*&>::type std::__invoke<void (*&)(Depsgraph*, Scene*, Object*), Depsgraph*, Scene*&, Object*&>(void (*&)(Depsgraph*, Scene*, Object*), Depsgraph*&&, Scene*&, Object*&) /usr/include/c++/9.1.0/bits/invoke.h:95
58 #13 0x5561cbd2538c in void std::_Bind<void (*(std::_Placeholder<1>, Scene*, Object*))(Depsgraph*, Scene*, Object*)>::__call<void, Depsgraph*&&, 0ul, 1ul, 2ul>(std::tuple<Depsgraph*&&>&&, std::_Index_tuple<0ul, 1ul, 2ul>) /usr/include/c++/9.1.0/functional:400
59 #14 0x5561cbd1aa06 in void std::_Bind<void (*(std::_Placeholder<1>, Scene*, Object*))(Depsgraph*, Scene*, Object*)>::operator()<Depsgraph*, void>(Depsgraph*&&) /usr/include/c++/9.1.0/functional:484
60 #15 0x5561cbd0f956 in std::_Function_handler<void (Depsgraph*), std::_Bind<void (*(std::_Placeholder<1>, Scene*, Object*))(Depsgraph*, Scene*, Object*)> >::_M_invoke(std::_Any_data const&, Depsgraph*&&) /usr/include/c++/9.1.0/bits/std_function.h:300
61 #16 0x5561cbd8cba2 in std::function<void (Depsgraph*)>::operator()(Depsgraph*) const /usr/include/c++/9.1.0/bits/std_function.h:690
62 #17 0x5561cbd88e4d in deg_task_run_func /src/blender/source/blender/depsgraph/intern/eval/deg_eval.cc:86
63 #18 0x5561cbc80918 in task_scheduler_thread_run /src/blender/source/blender/blenlib/intern/task.c:450
64 #19 0x7f4ada5b157e in start_thread (/usr/lib/libpthread.so.0+0x957e)
65
66Thread T15 created by T0 here:
67 #0 0x7f4adb562367 in __interceptor_pthread_create /build/gcc/src/gcc/libsanitizer/asan/asan_interceptors.cc:208
68 #1 0x5561cbc81dd0 in BLI_task_scheduler_create /src/blender/source/blender/blenlib/intern/task.c:517
69 #2 0x5561cbc8fbb9 in BLI_task_scheduler_get /src/blender/source/blender/blenlib/intern/threads.c:177
70 #3 0x5561cbc8d41c in BLI_task_parallel_range /src/blender/source/blender/blenlib/intern/task.c:1198
71 #4 0x5561cbdae206 in flush_prepare /src/blender/source/blender/depsgraph/intern/eval/deg_eval_flush.cc:118
72 #5 0x5561cbdae206 in DEG::deg_graph_flush_updates(Main*, DEG::Depsgraph*) /src/blender/source/blender/depsgraph/intern/eval/deg_eval_flush.cc:365
73 #6 0x5561cbcb4c50 in DEG_evaluate_on_refresh /src/blender/source/blender/depsgraph/intern/depsgraph_eval.cc:63
74 #7 0x5561cb4938f0 in scene_graph_update_tagged /src/blender/source/blender/blenkernel/intern/scene.c:1334
75 #8 0x5561cb4939f1 in BKE_scene_graph_update_tagged /src/blender/source/blender/blenkernel/intern/scene.c:1360
76 #9 0x5561cca2be96 in wm_event_do_depsgraph /src/blender/source/blender/windowmanager/intern/wm_event_system.c:370
77 #10 0x5561cca6bfba in wm_file_read_post /src/blender/source/blender/windowmanager/intern/wm_files.c:555
78 #11 0x5561cca6ee69 in wm_homefile_read /src/blender/source/blender/windowmanager/intern/wm_files.c:1059
79 #12 0x5561cca9d2d2 in WM_init /src/blender/source/blender/windowmanager/intern/wm_init_exit.c:293
80 #13 0x5561caaa6548 in main /src/blender/source/creator/creator.c:414
81 #14 0x7f4ad98eeee2 in __libc_start_main (/usr/lib/libc.so.6+0x26ee2)
82