blender
/
blender
136
411
Fork 595
Code Issues 6.3k Pull Requests 776 Projects 19 Wiki Activity

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

Merged
Nathan Vegdahl merged 13 commits from himisa/blender:main into main 2023-06-06 12:32:00 +02:00
Conversation 15 Commits 13 Files Changed 1 +1 -4
1 changed files with 1 additions and 4 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 5c79c03aa9 - Show all commits

5
source/blender/blenkernel/intern/armature_deform.c
View File

@ -71,14 +71,11 @@ static void pchan_deform_accumulate(const DualQuat *deform_dq,
mul_v3_m4v3(dst, mdq.scale, co_in);
sub_v3_v3(dst, co_in);
float w = mdq.quat[0], x = mdq.quat[1], y = mdq.quat[2], z = mdq.quat[3];
himisa marked this conversation as resolved Outdated
Nathan Vegdahl commented 2023-06-05 11:54:23 +02:00
Outdated
Review
Copy Link

I think it's already fairly obvious that mdq.quat[i] are the wxyz components of the quaternion, so I'd say get rid of the temporary variables and just use mdq.quat directly.

I think it's already fairly obvious that `mdq.quat[i]` are the wxyz components of the quaternion, so I'd say get rid of the temporary variables and just use `mdq.quat` directly.
himisa commented 2023-06-05 13:12:17 +02:00
Outdated
Review
Copy Link

I just want to avoid repeating mdq.quad[ for 12 times.

I just want to avoid repeating `mdq.quad[` for 12 times.
float dstx = mdq.scale[3][0], dsty = mdq.scale[3][1], dstz = mdq.scale[3][2];
mdq.scale[3][0] -= dst[0];
mdq.scale[3][1] -= dst[1];
mdq.scale[3][2] -= dst[2];
mdq.trans[0] -= .5f * (x * dst[0] + y * dst[1] + z * dst[2]);
mdq.trans[1] += .5f * (w * dst[0] + y * dst[2] - z * dst[1]);
mdq.trans[2] += .5f * (w * dst[1] + z * dst[0] - x * dst[2]);
mdq.trans[3] += .5f * (w * dst[2] + x * dst[1] - y * dst[0]);
mdq.scale_weight = 0.f;
deform_dq = &mdq;
himisa marked this conversation as resolved Outdated
Nathan Vegdahl commented 2023-06-05 11:58:41 +02:00
Outdated
Review
Copy Link

deform_dq is declared const, so it feels like bad form to modify its pointer. I think it would be better to get rid of this assignment entirely, and just use mdq directly below.

`deform_dq` is declared `const`, so it feels like bad form to modify its pointer. I think it would be better to get rid of this assignment entirely, and just use `mdq` directly below.
himisa commented 2023-06-05 13:11:00 +02:00
Outdated
Review
Copy Link

const Type *ptr = &value means the content of value shall not be changed via ptr. It is well defined to modify ptr itself.

If ptr is declared as Type * const ptr, ptr itself is a const, modifying ptr is bad formed.

`const Type *ptr = &value` means the content of `value` shall not be changed via `ptr`. It is well defined to modify `ptr` itself. If ptr is declared as `Type * const ptr`, `ptr` itself is a `const`, modifying `ptr` is bad formed.
himisa commented 2023-06-05 13:23:04 +02:00
Outdated
Review
Copy Link

I updated code.

But If we use mdq directly, then a else statement repeating add_weighted_dq_dq(... will be necessary for no scaling case.

I strongly recommand to use deform_dq = &mdq;, so that one unified call of add_weighted_dq_dq is sufficient.

I updated code. But If we use `mdq` directly, then a `else` statement repeating `add_weighted_dq_dq(...` will be necessary for no scaling case. I strongly recommand to use `deform_dq = &mdq;`, so that one unified call of `add_weighted_dq_dq` is sufficient.
Nathan Vegdahl commented 2023-06-06 10:04:21 +02:00
Outdated
Review
Copy Link

Ah, yeah. I'm well aware of the semantics of const *, but I wasn't following the control flow correctly: I forgot about the case where the if branch is skipped.

All other things being equal, I think I'd prefer an else branch that calls add_weighted_dq_dq(... with different arguments. By convention, I think of const input parameters as sort of indication to not modify them, even though the semantics allow it. But I don't really feel too strongly one way or the other. So let's just leave it as-is.

Ah, yeah. I'm well aware of the semantics of `const *`, but I wasn't following the control flow correctly: I forgot about the case where the `if` branch is skipped. All other things being equal, I think I'd prefer an `else` branch that calls `add_weighted_dq_dq(...` with different arguments. By convention, I think of const input parameters as sort of indication to not modify them, even though the semantics allow it. But I don't really feel too strongly one way or the other. So let's just leave it as-is.
Nathan Vegdahl commented 2023-06-06 10:06:26 +02:00
Outdated
Review
Copy Link

Ah! I should have looked at the changes first. Yeah, I think I prefer the else branch, like you've written it now. Thanks so much!

Ah! I should have looked at the changes first. Yeah, I think I prefer the else branch, like you've written it now. Thanks so much!
}