blender
/
blender
134
398
Fork 580
Code Issues 6.3k Pull Requests 757 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 +17 -2
1 changed files with 17 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -62,8 +62,23 @@ static void pchan_deform_accumulate(const DualQuat *deform_dq,
if (dq_accum) {
BLI_assert(!co_accum);
add_weighted_dq_dq(dq_accum, deform_dq, weight);
if (deform_dq->scale_weight) {
nathanvegdahl marked this conversation as resolved
Nathan Vegdahl commented 2023-06-05 11:36:07 +02:00
Review
Copy Link

Instead of FIX I think WORKAROUND is more appropriate here, because it's not a fully general solution, and only addresses certain cases.

EDIT: ah! Just noticed that you updated the solution. (Sorry about that.) I'll do some additional testing. "FIX" might actually be appropriate now. Thanks for your work on this!

EDIT 2: just did some more testing, and although there are some extreme cases that still could be improved, I think "FIX" is fine for this because it does address the large majority of issues.

Instead of `FIX` I think `WORKAROUND` is more appropriate here, because it's not a fully general solution, and only addresses certain cases. EDIT: ah! Just noticed that you updated the solution. (Sorry about that.) I'll do some additional testing. "FIX" might actually be appropriate now. Thanks for your work on this! EDIT 2: just did some more testing, and although there are some extreme cases that still could be improved, I think "FIX" is fine for this because it does address the large majority of issues.
/* FIX https://projects.blender.org/blender/blender/issues/32022 */
himisa marked this conversation as resolved
Nathan Vegdahl commented 2023-06-06 10:15:35 +02:00
Review
Copy Link

Just one last thing:

I would prefer if (deform_dq->scale_weight) to use an explicit comparison, like if (deform_dq->scale_weight != 0.0).

Because, for example, I'm not actually totally sure what the semantics of a float interpreted as a boolean is. I assume it just takes the the bits reinterpreted as an integer, and then 0 is equivalent to false. But I had to think about it. And it doesn't cover e.g. the case -0.0 which can happen with floats, but is equivalent to zero. So using an explicit comparison would, I think, make it both more clear and more correct.

Just one last thing: I would prefer `if (deform_dq->scale_weight)` to use an explicit comparison, like `if (deform_dq->scale_weight != 0.0)`. Because, for example, I'm not actually totally sure what the semantics of a `float` interpreted as a boolean is. I assume it just takes the the bits reinterpreted as an integer, and then 0 is equivalent to false. But I had to think about it. And it doesn't cover e.g. the case `-0.0` which can happen with floats, but is equivalent to zero. So using an explicit comparison would, I think, make it both more clear and more correct.
Nathan Vegdahl commented 2023-06-06 10:33:56 +02:00
Review
Copy Link

Out of curiosity, I looked up the defined behavior: https://en.cppreference.com/w/c/language/conversion

It turns out that using a float as a boolean is equivalent to first casting the float to an int, and then using the int. So, for example, 0.5 converts to 0, which is false.

This wasn't the behavior I expected at all, and also isn't the behavior my coworker here assumed when I asked him. Which I think is an indication that the semantics are non-obvious, and even if correct for the given situation probably shouldn't be used for the sake of code clarity.

Edit: further, it appears that at least clang doesn't follow that specified behavior. Which suggests that in practice it's compiler-specific.

Out of curiosity, I looked up the defined behavior: https://en.cppreference.com/w/c/language/conversion It turns out that using a `float` as a boolean is equivalent to first casting the float to an `int`, and then using the `int`. So, for example, `0.5` converts to `0`, which is false. This wasn't the behavior I expected at all, and also isn't the behavior my coworker here assumed when I asked him. Which I think is an indication that the semantics are non-obvious, and even if correct for the given situation probably shouldn't be used for the sake of code clarity. Edit: further, it appears that at least clang doesn't follow that specified behavior. Which suggests that in practice it's compiler-specific.
himisa commented 2023-06-06 11:23:47 +02:00
Review
Copy Link

This kind of usage (if(float)) is just a copy of existing code. see
150c041e09/source/blender/blenlib/intern/math_rotation.c (L2072)

I assume although scale_weight is float, it should only be integers, so everything works as expected.

I think it is better to leave it as it is now.

This kind of usage (`if(float)`) is just a copy of existing code. see https://projects.blender.org/blender/blender/src/commit/150c041e09a738daba99a28f4fd9b81ec9035ce1/source/blender/blenlib/intern/math_rotation.c#L2072 I assume although `scale_weight` is `float`, it should only be integers, so everything works as expected. I think it is better to leave it as it is now.
himisa commented 2023-06-06 11:30:39 +02:00
Review
Copy Link

see also mat4_to_dquat and dquat_to_mat4 and normalize_dq in same file (math_rotation.c).

see also `mat4_to_dquat` and `dquat_to_mat4` and `normalize_dq` in same file (math_rotation.c).
himisa commented 2023-06-06 11:58:10 +02:00
Review
Copy Link

Out of curiosity, I looked up the defined behavior: https://en.cppreference.com/w/c/language/conversion

I see through the defined behavior but:

Boolean conversion
A value of any scalar type (including nullptr_t) (since C23) can be implicitly converted to _Bool. The values that compare equal to an integer constant expression of value zero are converted to ​0​ (false), all other values are converted to 1 (true).

bool b1 = 0.5;              // b1 == 1 (0.5 converted to int would be zero)
bool b2 = 2.0*_Imaginary_I; // b2 == 1 (but converted to int would be zero)
bool b3 = 0.0 + 3.0*I;      // b3 == 1 (but converted to int would be zero)
bool b4 = 0.0/0.0;          // b4 == 1 (NaN does not compare equal to zero)
bool b5 = nullptr;          // b5 == 0 (since C23: nullptr is converted to false)

So it appears there is no problem for if(float) statement, if the floating point value is non-zero, its boolean value will always be true as expected, even it could be converted to int 0.

> Out of curiosity, I looked up the defined behavior: https://en.cppreference.com/w/c/language/conversion I see through the defined behavior but: ``` Boolean conversion A value of any scalar type (including nullptr_t) (since C23) can be implicitly converted to _Bool. The values that compare equal to an integer constant expression of value zero are converted to ​0​ (false), all other values are converted to 1 (true). bool b1 = 0.5; // b1 == 1 (0.5 converted to int would be zero) bool b2 = 2.0*_Imaginary_I; // b2 == 1 (but converted to int would be zero) bool b3 = 0.0 + 3.0*I; // b3 == 1 (but converted to int would be zero) bool b4 = 0.0/0.0; // b4 == 1 (NaN does not compare equal to zero) bool b5 = nullptr; // b5 == 0 (since C23: nullptr is converted to false) ``` So it appears there is no problem for `if(float)` statement, if the floating point value is non-zero, its boolean value will always be `true` as expected, even it could be converted to `int` 0.
Nathan Vegdahl commented 2023-06-06 12:25:11 +02:00
Review
Copy Link

Ah, indeed, I misread the spec. I also didn't realize that was already being done in the code base(!).

I think that's fine, then. In the future we can migrate these cases to be more clear/explicit.

Thanks!

Ah, indeed, I misread the spec. I also didn't realize that was already being done in the code base(!). I think that's fine, then. In the future we can migrate these cases to be more clear/explicit. Thanks!
DualQuat mdq = *deform_dq;
float dst[3];
mul_v3_m4v3(dst, mdq.scale, co_in);
himisa marked this conversation as resolved
Nathan Vegdahl commented 2023-06-05 11:42:26 +02:00
Review
Copy Link

I think the memcpy() can be replaced with the simpler:

mdq = *deform_dq;

(Unless I'm misunderstanding something.)

I think the `memcpy()` can be replaced with the simpler: ``` mdq = *deform_dq; ``` (Unless I'm misunderstanding something.)
sub_v3_v3(dst, co_in);
mdq.trans[0] -= .5f * (mdq.quat[1] * dst[0] + mdq.quat[2] * dst[1] + mdq.quat[3] * dst[2]);
mdq.trans[1] += .5f * (mdq.quat[0] * dst[0] + mdq.quat[2] * dst[2] - mdq.quat[3] * dst[1]);
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.
mdq.trans[2] += .5f * (mdq.quat[0] * dst[1] + mdq.quat[3] * dst[0] - mdq.quat[1] * dst[2]);
mdq.trans[3] += .5f * (mdq.quat[0] * dst[2] + mdq.quat[1] * dst[1] - mdq.quat[2] * dst[0]);
mdq.scale_weight = 0.f;
add_weighted_dq_dq(dq_accum, &mdq, weight);
}
else {
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!
add_weighted_dq_dq(dq_accum, deform_dq, weight);
}
}
else {
float tmp[3];