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

Merged
Alexander Gavrilov merged 1 commits from angavrilov/blender:armature-dualquat-pivot into main 2023-09-04 14:49:36 +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.
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
Nathan Vegdahl requested changes 2023-09-04 13:02:47 +02:00
Nathan Vegdahl left a comment
Member

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.

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.
@ -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);
Member

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?
Member

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

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.

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.
Author
Member

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
@ -2648,3 +2648,3 @@
armdef_accumulate_matrix(ct->tar->object_to_world,
iobmat,
ct->tar->world_to_object,
Member

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.
Author
Member

Reverted since this is unrelated to the main problem.

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
@ -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);
Member

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
Member

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
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser Project (Legacy)
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#111759
No description provided.