Page MenuHome

Armature: Add Display Axis Offset
ClosedPublic

Authored by Sybren A. Stüvel (sybren) on May 11 2020, 2:47 AM.
Tokens
"Love" token, awarded by Yuro."Love" token, awarded by gilberto_rodrigues."Love" token, awarded by Xury46."Love" token, awarded by AquaticNightmare."Love" token, awarded by jwvd."Like" token, awarded by Fracture128."Love" token, awarded by morritzio."Love" token, awarded by Draise."Love" token, awarded by zNight."Love" token, awarded by carlosmu."Like" token, awarded by AnityEx."Love" token, awarded by Vaquero."Love" token, awarded by verbal007.

Details

Summary

The added functionality was from a request from a rigger friend of mine. As a rigger, he and most other riggers he works with find that usually when you display axis, you expect them to be at the root of the bone (like you'd expect the axis to be at the origin of an object) vs the tip. However, there was a concern that having the axis at the root of the bone would get hidden in areas where there are plenty of other bones in the same area. So, the proposed fix is to have the axis be able to be moved from the root to tip of the bone and leave the positioning up to the rigger.

Right now, the axis position is in the same UI as the show axis property. From my understanding, it sounds like the UI team wants to keep those two elements close together. Also, the slider is a factor, since that seems to be the design that most people agree is better. Lastly, the UI is borrowing from the camera passepartout, and generally I'm trying to follow Blender UI conventions as closely as I can.

Screenshot:

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Please stop coding, and discuss this proposal properly first. As I said, post on Right Click Select, and include screenshots of the "before" and "after". Also be sure to discuss this change with the UI and Animation module teams first.

Please stop coding, and discuss this proposal properly first. As I said, post on Right Click Select, and include screenshots of the "before" and "after". Also be sure to discuss this change with the UI and Animation module teams first.

Sorry, should have clarified. I will still set up a proposal on RCS, and I'll happily talk with UI and Animation (what's the best way to communicate with them?). The code work to be done will be to help demo the proposal and to help me learn the back end of Blender. I'm okay with having the code completely thrown out and possibly rewritten. Either way, I don't expect the patch to move forward until I've made the proposal.

Sorry, should have clarified. I will still set up a proposal on RCS, and I'll happily talk with UI and Animation (what's the best way to communicate with them?). The code work to be done will be to help demo the proposal and to help me learn the back end of Blender. I'm okay with having the code completely thrown out and possibly rewritten. Either way, I don't expect the patch to move forward until I've made the proposal.

👍

Drop by on https://blender.chat/, you'll find the teams in the #ui and #animation-module channel. You can also always drop by in #blender-coders, most Blender developers hang out there.

Note that I just renamed the UI channel to #user-interface-module (to follow our naming convention, reading this here reminded me of this).

Jer Bot (verbal007) added a comment.EditedDec 22 2020, 8:09 PM

@Sybren A. Stüvel (sybren) - As requested, I created a proposal on Right-Click Select.


Options for Bone Axis Display
https://blender.community/c/rightclickselect/dQgbbc/

Jer Bot (verbal007) added a comment.EditedDec 23 2020, 4:13 AM

@Sybren A. Stüvel (sybren) , Despite the timestamp shown on Right-Click Select, the proposal has only been up for about 7 hours and has already gotten significant attention, with 39 retweets, 168+ likes and 31 upvotes on R-Click Select. No one has really added suggestions, besides general comments of support of this being added as an option (Dare I say... default? Naw, I won't push it).


The questions that remain are:

  1. Would this be a tip / base switch or a slider, to allow the axis display to be positioned more strategically?
  2. Should this be seen as a "set it and forget it" option (placed in the Preferences) -OR- be put in one of the Viewport Display menus. @Fin O'Riordan (fin.eskimo) might have some thoughts on this.
  3. *While this option is being considered, there is something related that I'm seeing that will affect riggers* - There is an automatic scaling being applied to the axis display, which is based on the bone length. This is pretty slick for most (60-70% of bones & views), but this not-so-great for situations where the bones are short. This impacts the visibility of hands, feet and spine with more than 3-4 bones. When rigging, I find it important to be able to view all major points of articulation at a glance, but this feature can hide critical pieces for getting the overall feeling of alignments, and how they relate.

All that being said...
What is the next logical step?
Is there additional information needed or checkboxes that need to get ticked?

P.S. I hope that someone noticed that I avoided saying, "*This is how they do it in XXX software.*" ;)

Nice to see this is being worked on! As a rigger this little issue has always been disturbing. My 2 cents about it:

  • Bones axis should be drawn by default at the root position (as proposed)
  • A slider to adjust the head-tail axis position is a nice option. Should be definitely a per-bone setting, not a global preference, to workaround the issue only when multiple bones have the same root location, leading to display multiple axes at the same location. This issue already exists with the current axis position, when multiple bones have the same tail position.
  • More drawing option for axis scaling would be nice (and I'd go further, for bones draw scale as well) , but maybe it should be treated in another patch in order to better separate the tasks and optimize chances for inclusion in master.

As discussed a bit in the Rigging & Animation meeting.

We all seemed to agree to have this as a per Armature attribute.

This is simpler than having to manage per bone.
This also makes it easier to find the option/slide, as it keeps it next the Axis display checkbox.

How is this layout?

Or should the slider be squeezed onto the same line as the checkbox?

So, this should be the finalish patch.

Here's the features:

  • Control axis on a per-armature basis.
  • The UI property gets hidden if the show_axes is False.

TODO:

  • Not sure why, but it looks like RNA_def_property_float_default(prop, 1.0f); doesn't get respected.
Scott Wilson (propersquid) retitled this revision from [WIP] Armature: Add Display Axis Offset to Armature: Add Display Axis Offset.Feb 19 2021, 5:16 AM
Sybren A. Stüvel (sybren) requested changes to this revision.Mar 1 2021, 11:15 AM

So, this should be the finalish patch.

Before this patch, the "Show" label only needed to be there once because it applied to all the following UI elements. This is now no longer the case.
How about making the Axes checkbox and the slider a single row, something like this:

  • Axes Position: [------ 0.25 ------]

(Of course it would need some more separation between "Axes" and "Position" so that it doesn't read like "Axes Position", but that's hard to do with ASCII art here).

  • Control axis on a per-armature basis.
  • The UI property gets hidden if the show_axes is False.

👍

TODO:

  • Not sure why, but it looks like RNA_def_property_float_default(prop, 1.0f); doesn't get respected.

Are you saying we shouldn't review yet, because you're still looking into it? Or are you asking for help with this?

Please also update the patch description so that it reflects the current state of the patch. People interested should be able to just read that, and understand what the patch is doing, without having to read all the comments.

This revision now requires changes to proceed.Mar 1 2021, 11:15 AM

Hey, thanks for the feedback.

  • Checkbox and slider on same line. Got it! Would it still make sense to hide the slider when checkbox is off, or disable it? From my memory, Blender usually disables the element.
  • The default value. Yeah, I need help here. I don't know where to set the default value, and as a side question, what do we want the default to be (root or tip)?
  • Ticket title and description: I'll update it tonight.

Thanks again!

I think this is a feature that should go in the animation preferences, as its something you'd normally always prefer on either the tip or the head of the bone and only change it when for some reason the current place is cluttered or so, but that's corner cases.
For those I'd have it in the display of the overlays as a sort of "override" of the default set in the animation settings.

Scott Wilson (propersquid) edited the summary of this revision. (Show Details)

Changed the axis position UI to be on the same row as the show axis checkbox. Also, the position property is enabled/disabled depending on the show axis checkbox.

I think this is a feature that should go in the animation preferences, as its something you'd normally always prefer on either the tip or the head of the bone and only change it when for some reason the current place is cluttered or so, but that's corner cases.
For those I'd have it in the display of the overlays as a sort of "override" of the default set in the animation settings.

I honestly have no thoughts on this either way. From what I was told by my rigger friend, the anim and UI team wanted to keep those options close together. If we do want it moved elsewhere, then I'll need some help to learn where to put it and how to have it accessible in the function I've modified.

I think this is a feature that should go in the animation preferences, as its something you'd normally always prefer on either the tip or the head of the bone and only change it when for some reason the current place is cluttered or so, but that's corner cases.
For those I'd have it in the display of the overlays as a sort of "override" of the default set in the animation settings.

In the biweekly animation/rigging meeting, it was suggested that it be placed here to keep it next to the visibility switch, which would prevent people from having to hunt for the option.

I think this is a feature that should go in the animation preferences, as its something you'd normally always prefer on either the tip or the head of the bone and only change it when for some reason the current place is cluttered or so, but that's corner cases.
For those I'd have it in the display of the overlays as a sort of "override" of the default set in the animation settings.

In the biweekly animation/rigging meeting, it was suggested that it be placed here to keep it next to the visibility switch, which would prevent people from having to hunt for the option.

I agree that hunting for options isnt great, but when an option like this you basically want to set it up once and never thing about again the settings make more sense.

Scott Wilson (propersquid) edited the summary of this revision. (Show Details)

Updates:

  • Change the position slider to a percentage. NOTE: This converts the value frame from 0 - 1 to 0 - 100, so it needs to be scaled down in the draw function.
  • Move the property to its own column. Borrowing from the camera passepartout as a reference.
Scott Wilson (propersquid) edited the summary of this revision. (Show Details)

Updates from Animation Module meeting:

  • Axis position defaults to 1 (tip) to match current Blender way of doing things. We decided to go for the least controversial settings for now, and can set the default to 0 (root) if requested (in the next Blender version?).
  • Changed the percentage back to factor. A few people pointed out they prefer factor, and I'm okay with that.
  • Fixed custom bone shapes not being affected by the position slider
  • The placement in the UI is still in the armature panel. The reasoning is that we want the turn on/off the axis and the axis position to be in the same area. We could move it to an overlay position, but that opens a can of worms (Should all armatures have their axes visible at once, or is that too noisy? What other overlay options that are currently on a per node type basis should also be moved to a global overlay?)
Sybren A. Stüvel (sybren) requested changes to this revision.Mar 19 2021, 10:51 AM

As a tip for future patches: please either use Arcanist to upload/update the patch, or use git diff -U10000. Both approaches will prevent the "Context not available." message you see below between your changes, and will make it possible to see more of the surrounding code. This makes it easier for reviewers to quickly see more of the context, without having to switch to their editors.

When building I get these messages:

ERROR (rna.define): rna_define.c:2051 RNA_def_property_float_default: "ArmatureEditBones.axes_position", set from DNA.
ERROR (rna.define): rna_define.c:2051 RNA_def_property_float_default: "ArmatureEditBones.axes_position", set from DNA.

So I guess that once you've set the default value in DNA, you don't need to repeat it in RNA.

The patch works fine when there are no custom shapes involved. However, on the Spring rig, the result is a bit dubious:

I've talked with @Ton Roosendaal (ton) about the bone axes being shown on the tail. He said it was more or less a random choice, happened to just stay that way. He's fine if we flip it to the other side. If we're indeed going for "0 factor" as the new default, then the custom bones issue certainly has to be solved. Or at least explained why it behaves the way it behaves (maybe it makes total sense and it turns out to be acceptable, but I can't say that now).

source/blender/draw/engines/overlay/overlay_armature.c
1281–1283

Don't include non-functional (in this case, formatting) changes with functional changes. It makes the functional changes harder to find, and the non-functional changes harder to improve.

Having said that, although it's more readable, it's not Blender's standard formatting. This means that anyone saving the file (with auto-formatting enabled) or running make format will revert this change.

1294

No need to put arm->axes_position in parentheses (also below).

source/blender/makesdna/DNA_armature_types.h
150

Add a comment that explains what this means. For example:

/** Relative position of the axes on the bone, from head (0.0f) to tail (1.0f). */
This revision now requires changes to proceed.Mar 19 2021, 10:51 AM
Scott Wilson (propersquid) edited the summary of this revision. (Show Details)

Updates:

  • Remove the defaults from RNA, so ERROR (rna.define): rna_define.c:2051 RNA_def_property_float_default: "ArmatureEditBones.axes_position", set from DNA. shouldn't pop up again.
  • Fix the axis position not going to the root with custom shapes. The spring rig looks fine to me after the fix.
  • Removed the formatting (need to investigate why my vscode formatter and Blender's formatting rules don't agree), and removed the unneeded parentheses.
  • Added missing comment
  • Changed the default of the axis position to the root.
Sybren A. Stüvel (sybren) requested changes to this revision.Mar 25 2021, 11:05 AM

Changed the percentage back to factor. A few people pointed out they prefer factor, and I'm okay with that.

I missed this. Who were those "few people"? I don't feel strongly either way (I prefer percentages, but constraint influence and all kind of shader node properties use 0-1 factors).

Changed the default of the axis position to the root.

A nice touch would be to add some versioning code, to set the value for existing files to 1. That way backward compatibility is maintained, and opening an existing file doesn't show a different result all of a sudden.

This revision now requires changes to proceed.Mar 25 2021, 11:05 AM

Updates:

  • Make Blender versions pre 2.93 (assuming this is when this patch will get in) axes position 1.0 (tip) by default instead of 0.0 (root).

Changed the percentage back to factor. A few people pointed out they prefer factor, and I'm okay with that.

I missed this. Who were those "few people"? I don't feel strongly either way (I prefer percentages, but constraint influence and all kind of shader node properties use 0-1 factors).

Honestly, can't remember. I've talked with a few people about the patch.

Changed the default of the axis position to the root.

A nice touch would be to add some versioning code, to set the value for existing files to 1. That way backward compatibility is maintained, and opening an existing file doesn't show a different result all of a sudden.

Added!

Sybren A. Stüvel (sybren) added inline comments.
source/blender/blenloader/intern/versioning_290.c
643–648

New versioning code should go into the "empty" block below, it'll be moved into a MAIN_VERSION_ATLEAST condition when the file version is bumped. This is notoriously tricky to get right in a patch, so don't worry about it, I'll do that when I commit.

Campbell Barton (campbellbarton) added inline comments.
source/blender/makesdna/DNA_armature_types.h
137

Should be 3 bytes.

source/blender/makesrna/intern/rna_armature.c
1547

Descriptions shouldn't end with a full-stop as the UI code adds this (warns in debug mode).

Julian Eisel (Severin) requested changes to this revision.Mar 26 2021, 11:23 AM

Pretty much ready, just two little things.

Note that for people preferring the use of percentages, there is an option for that in the Preferences: InterfaceEditorsFactor Display Type.

source/blender/makesdna/DNA_armature_types.h
137

Padding shouldn't be more than 7 bytes. Think this can be 3 bytes.

source/blender/makesrna/intern/rna_armature.c
1547

The numbers here are wrong, should be 0-1 (well, or 0-100 if the percentage preference is enabled). Plus, there shouldn't be a ending period, RNA gives a warning about it in debug builds.

Suggestion: "The position for the axes on the bone. Increasing the value moves it closer to the tip; decreasing moves it closer to the root"

This revision now requires changes to proceed.Mar 26 2021, 11:23 AM

Updates:

  • Made the armature padding 3 bytes instead of 11.
  • Change the tooltip for the axis (thanks for the suggested text)

Commandeering for some final tweaks before committing.

source/blender/makesrna/intern/rna_armature.c
1544–1547

This is not formatted properly. I'll do that before committing the patch. For future patches, enable auto-formatting with Clang-Format in your IDE or run make format before submitting a patch.

  • Cleanup: formatting
  • Versioning: move to blo_do_versions_290() function and only run the versioning code if the DNA has no float axes_position in the bArmature struct. This way it will only set the old default once, even when the subversion isn't bumped yet.
  • Remove the default value for axes_position. Since it's zero now, it doesn't have to be mentioned in DNA_armature_defaults.h (zero is the default default).
This revision is now accepted and ready to land.Mar 30 2021, 11:14 AM
This revision was automatically updated to reflect the committed changes.