Page MenuHome

Implement the delta mush modifier.
ClosedPublic

Authored by Campbell Barton (campbellbarton) on Mar 14 2015, 4:34 AM.

Diff Detail

Repository
rB Blender
Branch
deltamush

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

I know this isn't an area for design comments, but I thought I'd bring up the fact that "Laplacian smoothing" as it's mentioned in the original R&H paper and video is almost completely unrelated to what we have known as the Laplacian smooth modifier in Blender. The original paper calls for a smoothing function much closer to Blender's standard smoothing modifier, and indeed after getting this branch built the modifier as it stands doesn't work at all as expected in its smoothing functions. Additionally (and I may be mistaken here) I believe that all of the info needed for the delta mush function is accessible without the need to bind to UV coordinates.

In any case, glad to see this being tackled! This will be a huge get for animators when it's completed.

Thanks Matt Heimlich,
Interesting about the smoothing mode. I'll take a look tonight when I get home from work. I did notice that the smoothing effect did seem to end up being pretty localised.

The uv map is necessary for consistently oriented tangents, any uv map should do though. I guess I could remove the field and just report an error if one doesn't exist, but I don't think that is particularly user friendly. I'm all ears if there is another solution though

source/blender/modifiers/intern/MOD_deltamush.c
165

Yep, the comma should be a *. Funny that it still worked, I guess the tangents don't need to be super accurate.

282

Looks like I forgot to free this, will fix this along with whatever else in next diff

Jack Simpson (sazerac) updated this revision to Diff 3755.
  • Campbell Barton's warning fixup + a couple of changes
  • Change smoothing method to use normal smooth modifier

Added an updated diff with warning stuff + smoothing method change

source/blender/modifiers/intern/MOD_deltamush.c
163

abs converts to an int, you probably want fabsf(acosf( ... ))

Massive rework of modifier.

  • Changed to use first face/loop tangent to calculate tangent space
  • Now directly calculates tangent space
  • Now uses new edge weighted smoothing method
  • Now stores rest positions of vertices and re-calculates the deltas when smoothing params change
  • Vertex groups now only apply to the smoothing, and deltas are re-calculated when weights change (this should lead to a more intuitive effect)
  • smoothing is now always displayed when not bound
  • when bound, there is now a toggle to display only the effects of smoothing

didn't test the new patch, just some notes from checking over the code.

source/blender/modifiers/intern/MOD_deltamush.c
121–139

dmmd->old_weights is allocated & checked every time - even when there aren't any vertex groups.
Think its reasonable not to allocate it if no vgroups are used. (can be freed in that case)

167

better rename temp -> something useful eg, edge_dir

173

just negate_v3(temp), no need to recalculate the length.

or just reuse, eg:

	 {
		float edge_dir[3], *s;
		sub_v3_v3v3(edge_dir, vertexCos[edges[i].v2], vertexCos[edges[i].v1]);
		distance = len_v3(edge_dir);
		mul_v3_fl(edge_dir, distance);
	
		s = smoothVec[edges[i].v1];
		add_v3_v3(s, edge_dir);
		s[3] += distance;
		s[4] += 1.0f;
	
		s = smoothVec[edges[i].v2];
		sub_v3_v3(s, edge_dir);
		s[3] += distance;
		s[4] += 1.0f;
	 }
190

just malloc if its being cleared below every time anyway

244

values from getVertNo don't have to be re-normalized.
just copy_v3_v3 is fine.

383

(size_t)(numVerts * 3 * 3) * sizeof(float) can be written as (size_t)numVerts * sizeof(float[3][3]) or (size_t)numVerts * sizeof(*tangentSpaces)

384–387

for these kinds of allocations we just assume they suceed in modifier code. No need to error check

  • Address some review comments and add in pinning for boundary verts.
source/blender/blenloader/intern/readfile.c
4961

missing NULL check, also in some other places.

source/blender/makesdna/DNA_modifier_types.h
1298

name is a bit misleading, this is really current_weights - since its kept up to date (and compares against new weights)

1311

Realize we cant always avoid this - but storing previous values & checking if something changed this way is something to avoid.

If some action should run when a value is changed - better run it in an RNA update callback, where possible (goes for old_repeat too)

Editing some RNA values could just free the bind data.

source/blender/modifiers/intern/MOD_deltamush.c
148

This works but is a bit odd using (bit mask) > 0, this reads a bit more clearly - ((a & flag) == 0) != ((b & flag) == 0)

226

To be a little more clear, smoothVec could use a struct, eg struct { float co[3], dist_accum, dist_num; } VertSmoothData

244

better pass by reference - const MLoop *

322

This code doesn't ensure a CDDM is passed in, so better not call CDDM functions direct. dm->getVertArray(dm) works fine.

  • Address some more review comments, including moving invalidation of deltas to rna callbacks

@Campbell Barton (campbellbarton), I have addressed you comments so far, and I was wondering, is there a simpler way of setting up the bind button in the UI that defining a whole operator just to essentially flip a bit?

source/blender/makesdna/DNA_modifier_types.h
1311

Cheers, I completely missed that you could do this. That does make it a lot nicer.

Note that this modifier is logically a *deformation* modifier, yet its making a copy of the entire mesh just to perform modifications and calculations on.

To get functionality review and sort out basic behavior, this is fine,
but before going into master it should be changed to an eModifierTypeType_OnlyDeform modifier.

source/blender/modifiers/intern/MOD_deltamush.c
273

This shouldn't be needed, and you can iterate the 3 adjacent loops without modulo.

pre = p->loopstart + numLoops - 2;
loop = pre + 1;
post = p->loopstart;
for (l = 0; l < numLoops; l++) {
    ....

    /* no need for modulo*/
    pre = loop;
    loop = post;
    post++;
}
296

All axis are already normalized.

355–356

needs to be (dmmd->dm_flags & MOD_DELTAMUSH_SHOWSMOOTH) != 0, MSVC can have issues otherwise.

  • More fixes, including removing the DerivedMesh copy

Much better, calculating normals is fairly expensive, (and storing in the MVert meant it had to do short/float conversions every time too).

source/blender/makesrna/intern/rna_modifier.c
2142–2143

Is a negative value useful? - I tried to use it but it just made a scrambled mess.

2165

Probably should call use_bind, The prefix *show* is typically used for display, showing extra information - but not changing the calculation.

source/blender/modifiers/intern/MOD_deltamush.c
246–256

can save some sqrt calls and use the length of the cross product as the weight.

if (compare_v3v3(v_prev, v_next, FLT_EPSILON * 10.0f) == false) {
	float nor[3]; 
	float weight;

	cross_v3_v3v3(nor, v_prev, v_next);
	weight = normalize_v3(nor);
	if (weight != 0.0f) {
		weight = fabsf(acosf(dot_v3v3(v_next, v_prev) / weight));
	}
	cross_v3_v3v3(r_tspace[0], r_tspace[1], nor);

	mul_v3_fl(nor, weight);
	/* accumulate weighted normals */
	add_v3_v3(r_tspace[2], nor);
}
282

This isn't needed, could add BLI_assert(pre != post); - but it will never happen.

  • Addressed a few more review comments - turns out I already know how long the vectors I am using form normal weighting are - I normalised them already.
  • Change ui range of smoothing factor to positive only

Seems rather fine and useful modifier. Some minor feedback, but it might also be done by as when applying the modifier.

source/blender/editors/object/object_modifier.c
1830

Doesn't follow our code style: { is to be on the same line.

1835

Is this operator really intended to be used for both binding and unbinding?

source/blender/makesdna/DNA_modifier_types.h
87

Alignment is broken here.

1300

Call it just flags, dm_flags means flags of DerivedMesh, which is confusing.

source/blender/makesrna/intern/rna_modifier.c
1045

Space between keyword and parentheses.

2160

Should be called use_bind.

2165

Boolean properties should have use_ prefix.

source/blender/modifiers/intern/MOD_deltamush.c
46

(DeltaMushModifierData *)md

203

Think it makes sense to include boundaries into bound weights and skip boundaries calculation on every modifier run.

218

Suggestion: make this option taken into account during binding and skip all extra heavy calculations on runtime. You don't really need to toggle it on runtime anyway i bet.

270

can we call them numPolys?

source/blender/editors/object/object_modifier.c
1835

Odd syntax. just use mmd->dm_flags |= MOD_DELTAMUSH_BIND;

source/blender/makesdna/DNA_modifier_types.h
1300

dm_flags -> flag

source/blender/modifiers/intern/MOD_deltamush.c
219–220

Its rather inefficient to calculate boundary array every time - can't the result of boundaries be included in the bind data?

Not as another array, just multiply and store with the bind data.

This will mean pinning can only done before binding, but I think its worth doing for the speedup.

Then the rule will be - if pin or weights are used - there will have to be a smooth_weights array created on bind.

323

can use sizeof(float[3]) for all cases like this.

source/blender/editors/object/object_modifier.c
1835

it is for both bind and unbind, so needs to be XOR.

I was thinking though, it probably isn't necessary to have unbind at all, just do a rebind. So I may change this to just free the bind data and then have the modifier recalculate it

source/blender/modifiers/intern/MOD_deltamush.c
218

See reply on campbell's comment, certainly this isnt something people will want to toggle at runtime, but interactively editing the weights might be.

219–220

I could do this, but the problem I see is that it will break the way I track changes to the weights, which would mean that you could no longer interactively paint weights, unless there is a better way to achieve this?

If I use yet another array, then this could work fine, but is using more and more memory.

What do you think? from a user workflow perspective I would like to keep the interactive weight painting, but more speed is good too.

Campbell Barton (campbellbarton) reopened this revision.
Campbell Barton (campbellbarton) commandeered this revision.

auto-closed, updating this diff, you can commandeer back afterwards

Updated the patch, no real functional changes, minor edits and optimizations (approx ~30-40% faster in own tests of an optimized build with 100 iterations).


Summary of changes:

Each of these is a commit to a temp repo (temp-modifier-deltamush-experimental branch)

Campbell Barton (campbellbarton) planned changes to this revision.EditedMar 29 2015, 1:00 PM

@Jack Simpson (sazerac)

The smoothing algorithm seems rather strange (multiplying the edge by its length & accumulating),
is this based on some known good/working smooth method? or is it something you came up with yourself?

I tried some different methods, but found the most simple smoothing method works quite well. rB46f83e7391b637d16d81dc12a2d21bab912c5afd

Its stronger then the one is this patch (2x - 3x),
From my tests with 10 iterations, a factor of 0.5, it gets a good result which requires more iterations with the current method used in this patch.

Its committed to the branch but I didn't include in this diff since its changing behavior.

Committed if/def's to temp-modifier-deltamush-experimental to check on different smoothing methods.

The currently currently active one is unchanged from the original patch.

rBabe001bcfbf1a4dd3df6bb5ff936f5c705798a4d

This adds angle-weighted smoothing, so face-fans wont skew the vertex to one side.
rB8ec8cfe8b2fc7db1625b26da1782ba4263348970

However in my tests I really can't see much noticeable difference between them positive or negative (tested with make-human figure, so its possible the geometry is already too evenly spaces to show up problems in any of the methods).


Updated this patch.

  • Use simple smoothing function
  • pre-divide vertex-count, avoid zero checks in smooth loop
  • Add in defines for different smoothing methods
  • Add loop-angle-weighted smoothing

Updates from temp-modifier-deltamush-experimental branch.

  • minor rename RNA/flags
  • don't store vertex weights in the modifier
  • Add vertex-group invert option
  • rename boundverts -> positions_num
  • Don't write deltas to disk (keep as runtime cache)
  • Use the same weight buffer for boundary as vertex weights

@Jack Simpson (sazerac)
The smoothing algorithm seems rather strange (multiplying the edge by its length & accumulating),
is this based on some known good/working smooth method? or is it something you came up with yourself?
I tried some different methods, but found the most simple smoothing method works quite well. rB46f83e7391b637d16d81dc12a2d21bab912c5afd
Its stronger then the one is this patch (2x - 3x),
From my tests with 10 iterations, a factor of 0.5, it gets a good result which requires more iterations with the current method used in this patch.
Its committed to the branch but I didn't include in this diff since its changing behavior.

Interesting,

I based the smoothing on some random code here: http://www.chris-g.net/2014/09/05/delta-mush-in-splice/
Supposedly the distance weighting should reduce the number of iterations required. However, looking over it, the distance is effectively part of the original delta anyway, so its unlikely to make very much difference at all.

I did make a few attempts to implement the 1/edgelen weighted smoothing (scale dependent laplacian smoothing), which is an option on the voodoo version in the video, but I haven't been able to get it to work properly.

  • minor cleanup, comments
  • Add smoothing type as an option
  • Rename DeltaMush -> CorrectiveSmooth
  • Edit modifier defaults

@Jack Simpson (sazerac), made some examples with less even geometry and it does indeed give better results in some cases, though I found using 2.0 instead of 1.0 often works better. (gives nice smoothing with less iterations)

I've committed an option so you can select the smooth-type, simple or length weight.

This revision was automatically updated to reflect the committed changes.

Committed to master rBc16a8983efba9ecacd8da408d03c37a55483e528

Some notes on the final patch:

  • Optional smoothing method [simple/length-weight]
  • Binding is optional, defaults to using *original* coordinages. (The modifier no longer needs to write any data to disk)
  • Some UI changes, make layout follow laplacian-deform and armature a little.

Thanks for your contribution, artists will be very happy to have this in our next release!

\- Campbell

Thanks for all your help with review and fixes.