blender
/
blender
129
397
Fork 571
Code Issues 6.2k Pull Requests 750 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
himisa commented 2023-05-22 08:42:58 +02:00
Contributor
Copy Link

Fixed armature deformation bug when in Preserve Volume mode and both scale & rotation is present.

The problem:

  • When a mesh has an armature modifier on it with Preserve Volume mode enabled, and the bones of the modifier are both scaled and rotated, strange deformations occur.

Why this happens:

  • This happens because the Preserve Volume mode internally uses Quaternion to interpolate rotations.

    A bone has 3 parts of data describing its deformation: Quaternion(w,x,y,z) rotation quat, Translation(w,x,y,z) trans, and Scaling(4x4 matrix) scale. To calculate deformed position of a vertex r, it will be firstly scaled by scale, then rotated and translated by quat & trans.

    The 4x4 scaling matrix scale has a 3x3 part S33 about scaling and shearing along 3 axes, and a vector part ST3 that further translate the scaled position. i.e. scale@r = S33@r + ST3. This enables scaling about an arbitrary "pivot" (a point r0 satisfies scale@r0 = r0).

    However, when blending influence of multiple bones, different bones have different scaling pivot (their head position). Since quaternion rotation and translation/scaling are not mutable operations, this is what I believe causing this bug.

How this is fixed:

  • Note that the translational part ST3 of the scaling matrix is redundant in functionality with Translational part trans in deformation data. There exists an equivalence transformation that simultaneously change trans and ST3, while keeping the deformation unchanged.

    I applied this equivalence transformation to move the pivot to the vertex that the bones are deforming, before blending multiple bone transformations. Note that now the vertex is the pivot, so scaling transformations will not change its position. Further blending/applying of scaling matrices can be avoided.

Fixed armature deformation bug when in Preserve Volume mode and both scale & rotation is present. ---- The problem: - When a mesh has an armature modifier on it with Preserve Volume mode enabled, and the bones of the modifier are both scaled and rotated, strange deformations occur. Why this happens: - This happens because the Preserve Volume mode internally uses Quaternion to interpolate rotations. A bone has 3 parts of data describing its deformation: Quaternion(w,x,y,z) rotation `quat`, Translation(w,x,y,z) `trans`, and Scaling(4x4 matrix) `scale`. To calculate deformed position of a vertex `r`, it will be firstly scaled by `scale`, then rotated and translated by `quat & trans`. The 4x4 scaling matrix `scale` has a 3x3 part `S33` about scaling and shearing along 3 axes, and a vector part `ST3` that further translate the scaled position. i.e. `scale@r = S33@r + ST3`. This enables scaling about an arbitrary "pivot" (a point `r0` satisfies `scale@r0 = r0`). However, when blending influence of multiple bones, different bones have different scaling pivot (their head position). Since quaternion rotation and translation/scaling are not mutable operations, this is what I believe causing this bug. How this is fixed: - Note that the translational part `ST3` of the scaling matrix is redundant in functionality with Translational part `trans` in deformation data. There exists an equivalence transformation that simultaneously change `trans` and `ST3`, while keeping the deformation unchanged. I applied this equivalence transformation to move the pivot to the vertex that the bones are deforming, before blending multiple bone transformations. Note that now the vertex is the pivot, so scaling transformations will not change its position. Further blending/applying of scaling matrices can be avoided.
showcase(#32022).blend
1.1 MiB
showcase(#32022).png
188 KiB
showcase(#32022).png
himisa added 3 commits 2023-05-22 08:43:04 +02:00
2652043889 revert 3982f2ddf9 
revert try to fix 10-yr old bug #32022
himisa changed title from Fix #32022, #43188, #100373 to Fix #32022, #43188, #100373, Armature modifier - Preserve volume + Scale 2023-05-22 08:49:19 +02:00
Philipp Oeser added this to the Animation & Rigging project 2023-05-22 08:54:25 +02:00
Nathan Vegdahl requested review from Nathan Vegdahl 2023-05-23 11:10:41 +02:00
Nathan Vegdahl added the
Module
Animation & Rigging
 label 2023-05-23 11:10:59 +02:00
Nathan Vegdahl commented 2023-05-23 12:21:04 +02:00
Member
Copy Link

I tested this PR out, and although it does indeed solve the uniformly-scaling-all-bones problem, I don't think it's the "proper" fix that we ideally want.

For example, with this PR when two bones are scaled differently, there's an asymmetry in how that affects the deformations depending on the direction of the bones. Here's an example (made with this PR) demonstrating this, with a comparison to linear blend skinning in the same situation:


scaling_asymmetry.blend

In this image the upper bone in all four examples has been scaled down. The linear blend skinning doesn't care about the direction of the bones. And ideally that's what we want from dual quaternion skinning as well.

Nevertheless, maybe this is still worth merging as a stop-gap solution...? Because it at least resolves the most egregious case. But it would be nice to have a full, proper solution that makes scaling with dual quaternion skinning actually fully well behaved.

I tested this PR out, and although it does indeed solve the uniformly-scaling-all-bones problem, I don't think it's the "proper" fix that we ideally want. For example, with this PR when two bones are scaled differently, there's an asymmetry in how that affects the deformations depending on the direction of the bones. Here's an example (made with this PR) demonstrating this, with a comparison to linear blend skinning in the same situation: <a href="/attachments/d0c1bdf0-9fdc-4aab-a8e7-1e33474fd6e8"><img src="/attachments/d0c1bdf0-9fdc-4aab-a8e7-1e33474fd6e8" width="500" /></a> [scaling_asymmetry.blend](/attachments/f17543c5-4dd5-4b86-9956-6d7d1a5672d6) In this image the upper bone in all four examples has been scaled down. The linear blend skinning doesn't care about the direction of the bones. And ideally that's what we want from dual quaternion skinning as well. Nevertheless, maybe this is still worth merging as a stop-gap solution...? Because it at least resolves the most egregious case. But it would be nice to have a full, proper solution that makes scaling with dual quaternion skinning actually fully well behaved.
scaling_asymmetry.png
180 KiB
scaling_asymmetry.blend
964 KiB
Nathan Vegdahl commented 2023-05-23 12:58:59 +02:00
Member
Copy Link

A couple more things I noticed with additional testing of the PR:

  • It doesn't address problems that manifest when doing non-uniform scaling (i.e. different scales on different axes).
  • It doesn't address problems that manifest when shear is involved (which can happen due to parents with non-uniform scales).

So all-in-all, it looks to me like this approach is perhaps a bit of a workaround that only narrowly addresses one specific case: all bones being uniformly scaled by the same factor.

And again, maybe that's worth merging as a stop-gap. But, also again, having a proper solution that makes all scaling/shearing situations well behaved is ultimately what we want.

@himisa Also, if we do decide to merge this, the PR description probably needs to be fleshed out first. The attached image and blend file are definitely helpful and appreciated, but those wouldn't make it into the final commit log, which also needs to clearly communicate what is being fixed, etc.

A couple more things I noticed with additional testing of the PR: - It doesn't address problems that manifest when doing non-uniform scaling (i.e. different scales on different axes). - It doesn't address problems that manifest when shear is involved (which can happen due to parents with non-uniform scales). So all-in-all, it looks to me like this approach is perhaps a bit of a workaround that only narrowly addresses one specific case: all bones being uniformly scaled by the same factor. And again, maybe that's worth merging as a stop-gap. But, also again, having a proper solution that makes all scaling/shearing situations well behaved is ultimately what we want. @himisa Also, if we do decide to merge this, the PR description probably needs [to be fleshed out](https://wiki.blender.org/wiki/Process/Contributing_Code#Ingredients_of_a_Pull_Request) first. The attached image and blend file are definitely helpful and appreciated, but those wouldn't make it into the final commit log, which also needs to clearly communicate what is being fixed, etc.
himisa commented 2023-05-23 14:59:17 +02:00
Author
Contributor
Copy Link

@nathanvegdahl
( This is a quick fix that solved my problem when I'm rigging my character... )
I will further investigate whether the non-uniform scaling/shearing can also be fixed. It may take some time.

@nathanvegdahl ( This is a quick fix that solved my problem when I'm rigging my character... ) I will further investigate whether the non-uniform scaling/shearing can also be fixed. It may take some time.
himisa changed title from Fix #32022, #43188, #100373, Armature modifier - Preserve volume + Scale to WIP: Fix #32022, #43188, #100373, Armature modifier - Preserve volume + Scale 2023-05-23 15:04:28 +02:00
himisa changed title from WIP: Fix #32022, #43188, #100373, Armature modifier - Preserve volume + Scale to Fix #32022, #43188, #100373, Armature modifier - Preserve volume + Scale 2023-05-23 19:59:11 +02:00
himisa commented 2023-05-23 20:33:55 +02:00
Author
Contributor
Copy Link

@nathanvegdahl
( This is a quick fix that solved my problem when I'm rigging my character... )
I will further investigate whether the non-uniform scaling/shearing can also be fixed. It may take some time.

I examined the non-uniform scaling/shearing cases and found no clue about how they can be fixed.

The idea of this fix is explained in PR description, and is very simple and intuitive: align the pivot of all scaling transforms before blending. This solution does not care about whether the scaling is uniform or not. So, it also does not provide any clue for me to find what's going wrong in non-uniform scaling case. Thus, I conclude that the remaining absurdity may be the indeed limitation of quaternion interpolation, and an 'ideally proper fix' is beyond the scope of this PR. This statement is also true for asymmetries about bone direction.

I think this PR is ready for merge.

> @nathanvegdahl > ( This is a quick fix that solved my problem when I'm rigging my character... ) > I will further investigate whether the non-uniform scaling/shearing can also be fixed. It may take some time. I examined the non-uniform scaling/shearing cases and found no clue about how they can be fixed. The idea of this fix is explained in PR description, and is very simple and intuitive: align the pivot of all scaling transforms before blending. This solution does not care about whether the scaling is uniform or not. So, it also does not provide any clue for me to find what's going wrong in non-uniform scaling case. Thus, I conclude that the remaining absurdity may be the indeed limitation of quaternion interpolation, and an 'ideally proper fix' is beyond the scope of this PR. This statement is also true for asymmetries about bone direction. I think this PR is ready for merge.
himisa added 1 commit 2023-05-29 17:17:14 +02:00
himisa added 1 commit 2023-05-29 17:18:24 +02:00
f6a94de0eb revert a3aea691bf 
revert try to fix #101877
himisa changed title from Fix #32022, #43188, #100373, Armature modifier - Preserve volume + Scale to WIP: Fix #32022, #43188, #100373, Armature modifier - Preserve volume + Scale 2023-06-03 14:22:58 +02:00
himisa commented 2023-06-03 15:45:12 +02:00
Author
Contributor
Copy Link

The worst problem of this PR is it make some cases worse when both the bone and object vertices are offset from origin of their objects...

This make it unacceptable to merge at this point, and must be fixed...

The worst problem of this PR is it make some cases worse when both the bone and object vertices are offset from origin of their objects... This make it unacceptable to merge at this point, and must be fixed...
himisa added 4 commits 2023-06-03 19:15:07 +02:00
himisa commented 2023-06-03 19:28:17 +02:00
Author
Contributor
Copy Link

@nathanvegdahl

I found a new way to reset pivot (set it to the vertices that bones are deforming), such that all the issues we have found are addressed.

This must be the ideal perfect fix.

Happy.

@nathanvegdahl I found a new way to reset pivot (set it to the vertices that bones are deforming), such that all the issues we have found are addressed. This must be the ideal perfect fix. Happy.
fix_shearing.blend
1.1 MiB
fix_asymmetry.blend
964 KiB
fix_nonuniform.blend
1.1 MiB
real_fix.png
408 KiB
real_fix.png
himisa changed title from WIP: Fix #32022, #43188, #100373, Armature modifier - Preserve volume + Scale to Fix #32022, #43188, #100373, Armature modifier - Preserve volume + Scale 2023-06-03 19:29:18 +02:00
Nathan Vegdahl requested changes 2023-06-05 11:59:47 +02:00
source/blender/blenkernel/intern/armature_deform.c
@ -63,2 +63,4 @@
if (dq_accum) {
BLI_assert(!co_accum);
/* FIX https://projects.blender.org/blender/blender/issues/32022 */
Nathan Vegdahl commented 2023-06-05 11:36:07 +02:00
Member
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.
nathanvegdahl marked this conversation as resolved
source/blender/blenkernel/intern/armature_deform.c
@ -65,0 +67,4 @@
DualQuat mdq;
if (deform_dq->scale_weight) {
float dst[3];
memcpy(&mdq, deform_dq, sizeof(DualQuat));
Nathan Vegdahl commented 2023-06-05 11:42:26 +02:00
Member
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.)
himisa marked this conversation as resolved
source/blender/blenkernel/intern/armature_deform.c Outdated
@ -65,0 +70,4 @@
memcpy(&mdq, deform_dq, sizeof(DualQuat));
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];
Nathan Vegdahl commented 2023-06-05 11:54:23 +02:00
Member
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
Author
Contributor
Copy Link

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

I just want to avoid repeating `mdq.quad[` for 12 times.
himisa marked this conversation as resolved
source/blender/blenkernel/intern/armature_deform.c Outdated
@ -65,0 +76,4 @@
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;
Nathan Vegdahl commented 2023-06-05 11:58:41 +02:00
Member
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
Author
Contributor
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
Author
Contributor
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
Member
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
Member
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!
himisa marked this conversation as resolved
Nathan Vegdahl commented 2023-06-05 12:02:02 +02:00
Member
Copy Link

I brought this to the animation module meeting, and we agreed that we'd like to merge this once the code is in shape. I did another code review, with some change requests.

I brought this to the animation module meeting, and we agreed that we'd like to merge this once the code is in shape. I did another code review, with some change requests.
Nathan Vegdahl commented 2023-06-05 12:45:10 +02:00
Member
Copy Link

I did some testing with your new solution, and it works great! Awesome work!

I did find some extreme situations with shear that could still be improved, but this is already such a massive improvement (it addresses the vast majority of issues) that I think it makes sense to merge this latest solution. Further improvements can be done in a future PR if desired/needed.

Thank you so much for your work on this! Once you've addressed the change requests, I think this is ready to merge.

I did some testing with your new solution, and it works *great!* Awesome work! I did find some extreme situations with shear that could still be improved, but this is already such a *massive* improvement (it addresses the vast majority of issues) that I think it makes sense to merge this latest solution. Further improvements can be done in a future PR if desired/needed. Thank you so much for your work on this! Once you've addressed the change requests, I think this is ready to merge.
Nathan Vegdahl added this to the 4.0 milestone 2023-06-05 12:45:50 +02:00
himisa added 2 commits 2023-06-05 13:19:19 +02:00
himisa requested review from Nathan Vegdahl 2023-06-05 13:49:55 +02:00
Nathan Vegdahl requested changes 2023-06-06 10:15:38 +02:00
source/blender/blenkernel/intern/armature_deform.c
@ -66,1 +65,3 @@
add_weighted_dq_dq(dq_accum, deform_dq, weight);
if (deform_dq->scale_weight) {
/* FIX https://projects.blender.org/blender/blender/issues/32022 */
Nathan Vegdahl commented 2023-06-06 10:15:35 +02:00
Member
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
Member
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
Author
Contributor
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
Author
Contributor
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
Author
Contributor
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
Member
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!
himisa marked this conversation as resolved
Nathan Vegdahl merged commit f12e9f32b5 into main 2023-06-06 12:32:00 +02:00
Nathan Vegdahl commented 2023-06-06 12:32:21 +02:00
Member
Copy Link

Thanks again for working on this!

Thanks again for working on this!
Nathan Vegdahl approved these changes 2023-06-08 10:24:24 +02:00
Nathan Vegdahl commented 2023-06-08 10:26:02 +02:00
Member
Copy Link

(Forgot to approve before merging. This week is not my week, ha ha.)

(Forgot to approve before merging. This week is not my week, ha ha.)
Alexander Gavrilov commented 2023-08-31 17:11:44 +02:00
Member
Copy Link

This seems to be ignoring crazyspace, which needs to produce a local transformation matrix, rather than simply transforming a single coordinate.

It also completely ignores the Armature constraint, which is supposed to produce the same result as the armature modifier, and also needs to produce a local transformation matrix.

This seems to be ignoring crazyspace, which needs to produce a local transformation matrix, rather than simply transforming a single coordinate. It also completely ignores the Armature constraint, which is supposed to produce the same result as the armature modifier, and also needs to produce a local transformation matrix.
Alexander Gavrilov commented 2023-08-31 20:48:22 +02:00
Member
Copy Link

I added a follow up PR #111759 that extends this to constraints and crazyspace (computing the full scale matrix when necessary).

I added a follow up PR #111759 that extends this to constraints and crazyspace (computing the full scale matrix when necessary).
👍 1 ❤️ 1
lucas veber commented 2023-09-05 15:06:58 +02:00
First-time contributor
Copy Link

Just wanted to say a big "THANK YOU", was waiting for that to be fixed for years. @angavrilov Great work! (hope i don't spam/don't pollute the thread with this comment... if this is perceived as is, feel free to remove it)

Just wanted to say a big "THANK YOU", was waiting for that to be fixed for years. @angavrilov Great work! (hope i don't spam/don't pollute the thread with this comment... if this is perceived as is, feel free to remove it)
Alexander Gavrilov commented 2023-09-05 16:07:37 +02:00
Member
Copy Link

@angavrilov Great work!

Eh, I just extended the fix to crazyspace (sculpting) and constraints, the main fix is by @himisa.

> @angavrilov Great work! Eh, I just extended the fix to crazyspace (sculpting) and constraints, the main fix is by @himisa.
lucas veber commented 2023-09-06 10:27:51 +02:00
First-time contributor
Copy Link

Eh, I just extended the fix to crazyspace (sculpting) and constraints, the main fix is by @himisa.

Oh ok! Thank you @himisa then!
I even tried to solve it myself, but did not succeed, probably lacking maths knowledge. You made my day.
This is even better than the Maya implementation, that uses an external global scale value to handle scale over dual quats.

> Eh, I just extended the fix to crazyspace (sculpting) and constraints, the main fix is by @himisa. Oh ok! Thank you @himisa then! I even tried to solve it myself, but did not succeed, probably lacking maths knowledge. You made my day. This is even better than the Maya implementation, that uses an external global scale value to handle scale over dual quats.
❤️ 1
Nathan Vegdahl removed this from the Animation & Rigging project 2023-09-28 12:43:09 +02:00
Sign in to join this conversation.
Reviewers
No reviewers
Nathan Vegdahl
Labels
Clear labels   
Interest
Alembic

  
Interest
Animation & Rigging

  
Interest
Asset Browser

  
Interest
Asset Browser Project Overview

  
Interest
Audio

  
Interest
Automated Testing

  
Interest
Blender Asset Bundle

  
Interest
BlendFile

  
Interest
Collada

  
Interest
Compatibility
This issue affects/is about backward or forward 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
Issues relating to Wayland windowing support.

  
Interest
Workbench

  
Interest: X11

Issues relating to Xorg/X11 support.

  
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
Issues relating to security: https://wiki.blender.org/wiki/Process/Vulnerability_Reports

  
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 Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
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
Milestone
Clear milestone
No items
No Milestone
4.0
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
4 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#108134
No description provided.
Delete Branch "himisa/blender:main"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?