Armature: apply new DualQuat scale handling to constraints and crazyspace. #111759

Alexander Gavrilov · 2023-08-31T20:35:48+02:00

2023-08-31 20:35:48 +02:00

The f12e9f32b5 patch introduced a new improved method of blending
dual quaternion transformations to handle combined scale and rotation
better. However, the changes were not complete:

The new math ignored crazyspace computations, which need to compute
a complete transformation matrix. As an optimization, the new method
avoided fully computing the scale component, so the matrix would
have no scale or shear.
The Armature constraint is supposed to behave identically to the
modifier, and it was not updated. The constraint also requires
computing a complete matrix.

This change extracts the new math into a utility function, change
the optimization to be controlled by a parameter, and use the new
function in the constraint.

Attached are files from the original #108134 PR with added empties using the Armature constraint.

The f12e9f32b56ec7 patch introduced a new improved method of blending dual quaternion transformations to handle combined scale and rotation better. However, the changes were not complete: * The new math ignored crazyspace computations, which need to compute a complete transformation matrix. As an optimization, the new method avoided fully computing the scale component, so the matrix would have no scale or shear. * The Armature constraint is supposed to behave identically to the modifier, and it was not updated. The constraint also requires computing a complete matrix. This change extracts the new math into a utility function, change the optimization to be controlled by a parameter, and use the new function in the constraint. ------ Attached are files from the original #108134 PR with added empties using the Armature constraint.

fix_asymmetry1.blend

118 KiB

fix_shearing1.blend

166 KiB

fix_nonuniform1.blend

166 KiB

👍 2

Alexander Gavrilov added this to the Animation & Rigging project 2023-08-31 20:36:35 +02:00

Alexander Gavrilov added this to the 4.0 milestone 2023-08-31 20:36:44 +02:00

Alexander Gavrilov added the

Module

Animation & Rigging

label 2023-08-31 20:37:03 +02:00

Alexander Gavrilov requested review from Nathan Vegdahl 2023-08-31 20:37:50 +02:00

Alexander Gavrilov referenced this pull request

2023-08-31 20:48:22 +02:00

Fix #32022, #43188, #100373, Armature modifier - Preserve volume + Scale #108134

Nathan Vegdahl requested changes 2023-09-04 13:02:47 +02:00

Nathan Vegdahl left a comment

Thanks for putting the work in on this! Everything works great in my testing, and the code generally looks good. Just some nits I'd like addressed and some questions.

source/blender/blenkernel/intern/armature_deform.cc Outdated

						
				@ -82,3 +69,1 @@

				    else {

				      add_weighted_dq_dq(dq_accum, deform_dq, weight);

				    }

				    add_weighted_dq_dq_pivot(dq_accum, deform_dq, co_in, weight, mat_accum != nullptr);

Nathan Vegdahl commented

2023-09-04 13:00:08 +02:00

It's not clear to me why the last parameter here is based on whether mat_accum is null or not, especially since mat_accum doesn't seem to be used at all if we're in this branch. Can you add a comment documenting that?

It's not clear to me why the last parameter here is based on whether `mat_accum` is null or not, especially since `mat_accum` doesn't seem to be used at all if we're in this branch. Can you add a comment documenting that?

Nathan Vegdahl commented

2023-09-04 14:09:50 +02:00

From blender.chat:

AlexanderGavrilov
smat is mat_accum - the code for non-preserve volume is using the pointer as a flag, so I did the same, even though it's a bit weird in this case
A more clear alternative may be to introduce an explicit boolean parameter to all the functions
So do I make it an explicit bool parameter instead of the weird implicit use of non-null pointer?

Nathan Vegdahl
I think it makes sense to change it to be a separate parameter, yeah. It made sense as a possibly-null pointer to the matrix when the thing it was controlling was whether or not that matrix got filled in, but now that it's affecting more than that I think a separate bool is a lot clearer.

From blender.chat: > **AlexanderGavrilov** > smat is mat_accum - the code for non-preserve volume is using the pointer as a flag, so I did the same, even though it's a bit weird in this case > A more clear alternative may be to introduce an explicit boolean parameter to all the functions > So do I make it an explicit bool parameter instead of the weird implicit use of non-null pointer? > **Nathan Vegdahl** > I think it makes sense to change it to be a separate parameter, yeah. It made sense as a possibly-null pointer to the matrix when the thing it was controlling was whether or not that matrix got filled in, but now that it's affecting more than that I think a separate bool is a lot clearer.

nathanvegdahl marked this conversation as resolved

source/blender/blenkernel/intern/armature_deform.cc Outdated

						
				@ -292,0 +279,4 @@

				      /* The matrix itself is only computed in mul_v3m3_dq below, but

				       * smat not being null is important for pchan_deform_accumulate. */

				      smat = summat;

				    }

Nathan Vegdahl commented

2023-09-04 12:52:56 +02:00

This was easier for me to follow when it was rolled into the code around lines 370-380 ish. Is moving the code up here a bug fix, or is it just a stylistic change? If it's just stylistic, please revert.

Alexander Gavrilov commented

2023-09-04 14:09:56 +02:00

smat == mat_accum, it was using the pointer as an implicit boolean parameter. Now it is an explicit bool flag.

`smat` == `mat_accum`, it was using the pointer as an implicit boolean parameter. Now it is an explicit bool flag.

nathanvegdahl marked this conversation as resolved

source/blender/blenkernel/intern/constraint.cc Outdated

						
				@ -2648,3 +2648,3 @@

				    armdef_accumulate_matrix(ct->tar->object_to_world,

				                             iobmat,

				                             ct->tar->world_to_object,

Nathan Vegdahl commented

2023-09-04 12:43:19 +02:00

How sure are you that world_to_object is valid here? The previous code was computing this from object_to_world, which makes me wonder if world_to_object isn't always guaranteed to be up-to-date when this function is called.

How sure are you that `world_to_object` is valid here? The previous code was computing this from `object_to_world`, which makes me wonder if `world_to_object` isn't always guaranteed to be up-to-date when this function is called.

Alexander Gavrilov commented

2023-09-04 14:09:14 +02:00

Reverted since this is unrelated to the main problem.

nathanvegdahl marked this conversation as resolved

Alexander Gavrilov force-pushed armature-dualquat-pivot from 05582b25b0 to 7b2961f47c

2023-09-04 14:08:14 +02:00

Compare

Nathan Vegdahl requested changes 2023-09-04 14:17:29 +02:00

source/blender/blenkernel/intern/armature_deform.cc Outdated

						
				@ -116,3 +103,3 @@

				  pchan_deform_accumulate(

				      &quats[index], mats[index + 1].mat, co, weight * (1.0f - blend), vec, dq, defmat);

				      &quats[index], mats[index + 1].mat, co, weight * (1 - blend), vec, dq, defmat, full_deform);

Nathan Vegdahl commented

2023-09-04 14:17:23 +02:00

According to the style guide, float literals are supposed to be written 1.0f as it was before. I don't feel strongly about this myself, but since it was already that way, and that adheres to the style guide, I think it's best to leave it as it was.

According to the style guide, float literals are supposed to be written `1.0f` as it was before. I don't feel strongly about this myself, but since it was already that way, and that adheres to the style guide, I think it's best to leave it as it was.

nathanvegdahl marked this conversation as resolved

Alexander Gavrilov force-pushed armature-dualquat-pivot from 7b2961f47c to ddb85d3d9a

2023-09-04 14:44:07 +02:00

Compare

Nathan Vegdahl approved these changes 2023-09-04 14:48:42 +02:00

Nathan Vegdahl left a comment

Looks good to me!

Thanks so much for both catching and fixing this!

Looks good to me! Thanks so much for both catching and fixing this!

Alexander Gavrilov merged commit dee29f4e81 into main

2023-09-04 14:49:36 +02:00

Alexander Gavrilov deleted branch armature-dualquat-pivot

2023-09-04 14:49:37 +02:00

Alexander Gavrilov referenced this issue from a commit

2023-09-04 14:49:38 +02:00

Armature: apply new DualQuat scale handling to constraints and crazyspace.

Stefan Werner referenced this issue from a commit

2023-09-07 15:02:38 +02:00

Armature: apply new DualQuat scale handling to constraints and crazyspace.

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Armature: apply new DualQuat scale handling to constraints and crazyspace. #111759