blender
/
blender
136
411
Fork 595
Code Issues 6.3k Pull Requests 774 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 +4 -2
1 changed files with 4 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 245ce97d12 - Show all commits

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

@ -62,8 +62,10 @@ 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);
DualQuat dq;
memcpy(&dq, deform_dq, sizeof(DualQuat));
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.
set_scale_pivot_dq(&dq, co_in);
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!
add_weighted_dq_dq(dq_accum, &dq, weight);
}
else {
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.)
float tmp[3];