This patch add offset/individual scaling XYZ/rotation to custom shape. and remove uniform scaling from custom shape.
It will be more easily set up custom shapes instead of append many objects that adjusted offset/scaling/rotation:
Details
Details
- Reviewers
Sybren A. Stüvel (sybren) - Group Reviewers
Animation & Rigging - Commits
- rBfc5bf09fd88c: Rigging: Add transform for custom bone shapes
Diff Detail
Diff Detail
- Repository
- rB Blender
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
I found custom_scale (custom_shape_scale) uses as invisible (scale = 0.0) on some addons.
so I added custom_shape_hide.
| source/blender/blenkernel/intern/action.c | ||
|---|---|---|
| 659 | Is there any reason you still set this variable? | |
| 1244 | I'm guessing custom_shape_hide should be copied here as well? | |
| source/blender/blenloader/intern/versioning_290.c | ||
| 2012 ↗ | (On Diff #36208) | This should be without any version check and inside the Versioning code until next subversion bump goes here. code block below. We will move it into a MAIN_VERSION_ATLEAST check later when we bump the file version later. |
| 2021 ↗ | (On Diff #36208) | Is is possible to detect the case where the new PCHAN_DRAW_NO_CUSTOM_SHAPE enum should be set? Also as mentioned above, you shouldn't need to set the custom scale to 1.0f here right` |
| source/blender/makesdna/DNA_action_types.h | ||
| 268 | You should probably add a /* Deprecated */ comment here. | |
| source/blender/blenkernel/intern/action.c | ||
|---|---|---|
| 1244 | I added custom_shape_hide (Python) on the drawflag. | |
Comment Actions
You can mark the comments that are handled as "Done" by checking the checkmark on them and submitting an empty comment.
This way it is easier to keep track of if there are anything that is yet to be fixed.
All my comments seems to be fixed now for me :)
| source/blender/blenkernel/intern/action.c | ||
|---|---|---|
| 1244 | You are totally correct! | |
Comment Actions
The patch needs to be updated to the latest master version.
Otherwise the versioning_290.c changes gets put in the wrong place.
Can you also generate the diff with git diff -U1000 so we can that we get rid of the "Context not available" here in the patch review?
I also noticed that if I enter edit mode on a single bone added to the default cube scene, blender crashes.
Can you reproduce this crash as well?
If you can't, you can try to see if building with ASAN exposes this for you.
| source/blender/draw/engines/overlay/overlay_armature.c | ||
|---|---|---|
| 1266 | I think you should invert this check. So you write instead: if ((pchan->drawflag & PCHAN_DRAW_NO_CUSTOM_SHAPE) ) {
return;
}
float length;At the start of the function. This way you know that if the flag is set, this function does nothing. | |
Comment Actions
I fixed some lines by review, fixed accesses to null pointer on draw non-custom shape bones.
Comment Actions
The patch needs to be updated to the latest master version.
Otherwise the versioning_290.c changes gets put in the wrong place.
I updated to the latest master.
I also noticed that if I enter edit mode on a single bone added to the default cube scene, blender crashes.
Can you reproduce this crash as well?
I can reproduce it and I fixed it.
Comment Actions
There is a use of PCHAN_CUSTOM_DRAW_SIZE in armature.c that hasn't been updated for the fact that the macro now no longer takes custom bone scale into account.
| source/blender/blenkernel/intern/action.c | ||
|---|---|---|
| 659–661 | This is just copy_v3_fl(chan->custom_scale_xyz, 1.0f) | |
| 660 | This is just zero_v3(chan->custom_offset_loc); same below. | |
| source/blender/blenloader/intern/versioning_290.c | ||
| 2058–2074 ↗ | (On Diff #36285) | This should be moved into the versioning_30.c file once that has been added. This is notoriously tricky to get right in the review patch, though, so it'll have to be moved by the committer. |
| 2061 ↗ | (On Diff #36285) | Use LISTBASE_FOREACH (Object *, ob, &bmain->objects) { instead. |
| 2062 ↗ | (On Diff #36285) | Reduce cognitive complexity by just doing if (ob->pose == NULL) { continue; }. That'll un-indent the rest of the for-loop. |
| 2063–2064 ↗ | (On Diff #36285) | Use LISTBASE_FOREACH |
| 2065 ↗ | (On Diff #36285) | Same as above: use the copy_v3_fl function. |
| 2066–2067 ↗ | (On Diff #36285) | New fields are automatically zeroed out, so no need to do this here. |
| 2068–2070 ↗ | (On Diff #36285) | I don't understand this code. If pchan->drawflag & PCHAN_DRAW_NO_CUSTOM_SHAPE is true, that means that the bit is set, and the xor operator will always clear it. This means that you don't need the condition to begin with, and can just write something like this instead: pchan->drawflag &= ~PCHAN_DRAW_NO_CUSTOM_SHAPE; However, that bit was previously unused in the drawflag, so it should be zero already. |
| source/blender/draw/engines/overlay/overlay_armature.c | ||
| 1034 | There is no such thing as a 3D bone length, as "length" is inherently 1D. bone_scale is better. | |
| 1046 | length can be const | |
| 1048 | This is just mul_v3_fl(bone_length, length). | |
| 1054 | Use the copy_v3_fl function. | |
| 1284–1285 | It's faster to first multiply pchan->custom_scale_xyz with length, and then apply the result to the matrix. | |
| source/blender/makesdna/DNA_action_types.h | ||
| 270 | change offset_loc to translation | |
| 271 | change eul to rotation_euler. There is no need to overly shorten names. | |
| 420–421 | This now no longer takes bone scale into account, so it's no longer a single macro that provides the correct draw size. This should be documented in a comment. | |
| source/blender/makesrna/intern/rna_pose.c | ||
| 1369 | I think "Offset" is ambiguous. After all, the rotation is also offsetting the shape rotationally. Use either "Location" (which is technically incorrect, but used in many places in Blender) or "Translation" (which is technically correct but used less often). That way you don't have to suffix it with _loc to remove the ambiguity. | |
| 1372 | The scale isn't overridable, but the rotation and translation properties are. I'd say this ought to be consistent. | |
| 1380 | "in XYZ Eulers" can be removed from the description. | |
| 1380 | "XYZ Euler" can be removed from the label. | |
| 1385 | "The hide of" means something like "the skin of", like the hide of an animal. The description should also make it clear what happens when the custom shape is hidden. Does that mean the original bone shape is shown? Or is the entire bone invisible? | |
Comment Actions
Not all feedback has been taken into account (even including a note that was marked as "done").
| source/blender/makesrna/intern/rna_pose.c | ||
|---|---|---|
| 1366 | "XYZ" can be removed, as the fact that this is a float[3] property already is clear enough. | |
| 1385 | "Hide the custom shape" doesn't convey anything beyond what the property label already tells you. As I said, the description should also make it clear what happens when the custom shape is hidden. | |
| source/blender/makesrna/intern/rna_pose.c | ||
|---|---|---|
| 1369 | Replace "Offset Location" with "Location". | |
| 1385 | "Temporarily hide the custom shape" still isn't explaining enough. As I said before: The description should make it clear what happens when the custom shape is hidden. Does that mean the original bone shape is shown? Or is the entire bone invisible? | |
Comment Actions
I fixed some lines by review, moved if-block for PCHAN_DRAW_NO_CUSTOM_SHAPE to hide custom shape.
| source/blender/blenloader/intern/versioning_290.c | ||
|---|---|---|
| 2068–2070 ↗ | (On Diff #36285) | I removed this code. |
Comment Actions
"Custom Shape is not visible when it is not in Edit Mode"
If the custom shape is not visible, what happens to the bone? Does it reverts to showing the default shape (like it would without custom shape)? Or is assigning a custom shape + hiding the custom shape the same as hiding the bone altogether (so nothing is drawn)?
Comment Actions
I think adding a second "Hide" property for bones would be AMAZING, but this is the wrong way to do that, and it's maybe out of scope for this patch. The issue with a custom_shape_hide is simply that it relies on a custom shape being assigned. It could instead just be called enabled and not be related to custom shapes, and I think that would be a stronger design. But that raises other questions that make it be out of the scope of this patch.
Comment Actions
If the custom shape is not visible, what happens to the bone? Does it reverts to showing the default shape (like it would without custom shape)? Or is assigning a custom shape + hiding the custom shape the same as hiding the bone altogether (so nothing is drawn)?
"custom_shape_hide" will be invisible custom shape in viewport. not happens to the bone.
I think adding a second "Hide" property for bones would be AMAZING, but this is the wrong way to do that, and it's maybe out of scope for this patch. The issue with a custom_shape_hide is simply that it relies on a custom shape being assigned. It could instead just be called enabled and not be related to custom shapes, and I think that would be a stronger design. But that raises other questions that make it be out of the scope of this patch.
I added custom_shape_hide, requested by animator. because they uses it (uniform custom_shape_scale = 0.0) with driver for rig. and in some addons also.
I removed custom_shape_hide in currently patch.
Comment Actions
I tested the patch with a Sprite Fright character and it seems to work fine! 👍
I wonder if the precision when sliding the Translation values can be made more precise. Currently the increment is quite large on a human sized character, even when holding Shift. This is also another reason I was advocating for a tool/gizmo for this. ;)
Comment Actions
It seems like this hasn't been addressed yet.
In source/blender/blenkernel/intern/armature.c line 2884, this macro is used as well.
So that line needs to be updated to take into account the changes to the macro.
Also as Sybren pointed out, the macro no longer returns the the final custom bone size/scale.
However instead of a comment I was thinking that perhaps we could rename the macro.
What do you guys think about PCHAN_CUSTOM_BONE_LENGTH ?
To me this makes sense as it returns the bone length and the length is then set to 1.0f if the "scale custom object to bone length" option is off.
Comment Actions
👍
This would still need a comment that it doesn't take custom bone scaling into account, though, as it otherwise would be ambiguous whether it's the pre-scale or post-scale length.