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

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

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

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)

better rename temp -> something useful eg, edge_dir

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;
	 }

just malloc if its being cleared below every time anyway

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

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

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

missing NULL check, also in some other places.

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

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.

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

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

better pass by reference - const MLoop *

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

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

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++;
}

All axis are already normalized.

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

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

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

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

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

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

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

Alignment is broken here.

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

Space between keyword and parentheses.

Should be called use_bind.

Boolean properties should have use_ prefix.

(DeltaMushModifierData *)md

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

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.

can we call them numPolys?

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

dm_flags -> flag

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.

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

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

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

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.