WIP: Fix #97049: Auto-keyframe Status Message Overlaps Navigation Gizmo #105565

Abhinav Chennubhotla · 2023-03-08T15:05:11+01:00

2023-03-08 15:05:11 +01:00

First-time contributor

Adjust the position of the Auto-keying status message so that it no
longer displays on top of the Navigation Gizmo.

In Preferences we can turn on the "Auto-Keyframing" "Warning". With that enabled we see a status message while transforming objects and bones while auto-keyframing is on. Unfortunately that status message overlaps awkwardly with the Navigation Gizmo at the top-right:

This re-opens this old diff - https://archive.blender.org/developer/D14614

Fixes #97049.
Partially fixed by moving the message to the left by a length equal to the length of the gizmo.

Alternate solutions: Remove the message entirely, and change the recording icon to red when auto-keying is turned on. (#105574)

TODO:

Fix bug for manually altered sizes of the gizmo
Fix bug for the USER_MINI_AXIS_TYPE_MINIMAL gizmo.

Adjust the position of the Auto-keying status message so that it no longer displays on top of the Navigation Gizmo. --- In Preferences we can turn on the "Auto-Keyframing" "Warning". With that enabled we see a status message while transforming objects and bones while auto-keyframing is on. Unfortunately that status message overlaps awkwardly with the Navigation Gizmo at the top-right: ![autokeywarning.png](/attachments/0fa0d78a-875e-4a3b-9413-81713873d3d7) This re-opens this old diff - https://archive.blender.org/developer/D14614 Fixes #97049. Partially fixed by moving the message to the left by a length equal to the length of the gizmo. Alternate solutions: Remove the message entirely, and change the recording icon to red when auto-keying is turned on. (#105574) TODO: - Fix bug for manually altered sizes of the gizmo - Fix bug for the `USER_MINI_AXIS_TYPE_MINIMAL` gizmo.

auto-keyframe-before.mp4

1.7 MiB

auto-keyframe-fix.mp4

654 KiB

autokeywarning.png

44 KiB

Abhinav Chennubhotla commented

2023-03-08 15:11:26 +01:00

First-time contributor

To reiterate what was mentioned in the older diff,

The problem here seems to be that printable_size (the size of the text it seems) is changing with the size of the gizmo. At size 80, printable_size[0] is 87 and at size 200, it is 235. Even the y value is changing, as you can see the record icon is slightly below the text.

The offset value for USER_MINI_AXIS_TYPE_MINIMAL also seems to be missing from UserDef attributes. In release/scripts/startup/bl_ui/space_userpref.py mini_axis_size is mentioned but I could only find it in an rna file, not as an attribute. Any help with this? I put in gizmo_size_navigate_v3d as a placeholder so it's at least somewhat functional.

Also the offset value for USER_MINI_AXIS_TYPE_NONE is set to 25, since that seems to be a value that works and the text doesn't overlap with the zoom and pan buttons.

I mainly need help figuring out why the values of printable_size are changing with the gizmo size.

@harley (tagging because you were involved in the conversation in the older diff)

To reiterate what was mentioned in the older diff, ![image](/attachments/fc39642f-45bf-4f30-9435-6469f4e6f60d) > The problem here seems to be that `printable_size` (the size of the text it seems) is changing with the size of the gizmo. At size 80, `printable_size[0]` is 87 and at size 200, it is 235. Even the y value is changing, as you can see the record icon is slightly below the text. > The `offset` value for `USER_MINI_AXIS_TYPE_MINIMAL` also seems to be missing from `UserDef` attributes. In `release/scripts/startup/bl_ui/space_userpref.py` `mini_axis_size` is mentioned but I could only find it in an rna file, not as an attribute. Any help with this? I put in `gizmo_size_navigate_v3d` as a placeholder so it's at least somewhat functional. > Also the offset value for `USER_MINI_AXIS_TYPE_NONE` is set to 25, since that seems to be a value that works and the text doesn't overlap with the zoom and pan buttons. I mainly need help figuring out why the values of `printable_size` are changing with the gizmo size. @harley (tagging because you were involved in the conversation in the older diff)

image.png

26 KiB

Abhinav Chennubhotla requested review from Harley Acheson 2023-03-08 15:50:13 +01:00

Abhinav Chennubhotla force-pushed 97049-fix from c5d58cdd6a to da0b07dd07

2023-03-08 16:20:28 +01:00

Compare

Abhinav Chennubhotla force-pushed 97049-fix from da0b07dd07 to db532b4b09

2023-03-08 16:42:46 +01:00

Compare

Abhinav Chennubhotla force-pushed 97049-fix from db532b4b09 to eef2ee0a71

2023-03-08 17:21:06 +01:00

Compare

Harley Acheson commented

2023-03-08 18:58:43 +01:00

Hi!

I've sent a quick note to the Animation Module in case they want that warning changed, moved elsewhere, etc.

As for the complications in this PR I haven't looked at it much, but would guess that the differences in printable_size you are seeing is that a font size is not explicitly set when the string is measured. So the size of the default font will be whatever it is when that function is called, but when it is drawn with BLF_draw_default later it is using a specific size.

If you follow BLF_default to see what it is doing you will see that BLF_draw_default is setting a size. But you will also see a BLF_set_default in between them that I think you can just swap for BLF_default as they are the same but it also sets that same size.

In case the above is unclear I mean changing this line, along with an other changes you want to make:

diff --git a/source/blender/editors/transform/transform.c b/source/blender/editors/transform/transform.c
index 7f5b23f0489..96107d40d1f 100644
--- a/source/blender/editors/transform/transform.c
+++ b/source/blender/editors/transform/transform.c
@@ -1484,17 +1484,17 @@ static void drawTransformView(const struct bContext *C, ARegion *region, void *a
 static void drawAutoKeyWarning(TransInfo *UNUSED(t), ARegion *region)
 {
   const char *printable = IFACE_("Auto Keying On");
   float printable_size[2];
   int xco, yco;
 
   const rcti *rect = ED_region_visible_rect(region);
 
-  const int font_id = BLF_default();
+  const int font_id = BLF_set_default();
   BLF_width_and_height(
       font_id, printable, BLF_DRAW_STR_DUMMY_MAX, &printable_size[0], &printable_size[1]);
 
   xco = (rect->xmax - U.widget_unit) - (int)printable_size[0];
   yco = (rect->ymax - U.widget_unit);
 
   /* warning text (to clarify meaning of overlays)
    * - original color was red to match the icon, but that clashes badly with a less nasty border

Note that some of this bad overlap will be reduced as that warning is raised, not just as it is moved left. Just because that gizmo is round so it might tuck in nicely into its top-left corner at bit.

Hi! I've sent a quick note to the Animation Module in case they want that warning changed, moved elsewhere, etc. As for the complications in this PR I haven't looked at it much, but would _guess_ that the differences in `printable_size` you are seeing is that a font size is not explicitly set when the string is measured. So the size of the default font will be whatever it is when that function is called, but when it is drawn with `BLF_draw_default` later it is using a specific size. If you follow `BLF_default` to see what it is doing you will see that `BLF_draw_default` is setting a size. But you will also see a `BLF_set_default` in between them that I think you can just swap for `BLF_default` as they are the same but it also sets that same size. In case the above is unclear I mean changing this line, along with an other changes you want to make: ```Diff diff --git a/source/blender/editors/transform/transform.c b/source/blender/editors/transform/transform.c index 7f5b23f0489..96107d40d1f 100644 --- a/source/blender/editors/transform/transform.c +++ b/source/blender/editors/transform/transform.c @@ -1484,17 +1484,17 @@ static void drawTransformView(const struct bContext *C, ARegion *region, void *a static void drawAutoKeyWarning(TransInfo *UNUSED(t), ARegion *region) { const char *printable = IFACE_("Auto Keying On"); float printable_size[2]; int xco, yco; const rcti *rect = ED_region_visible_rect(region); - const int font_id = BLF_default(); + const int font_id = BLF_set_default(); BLF_width_and_height( font_id, printable, BLF_DRAW_STR_DUMMY_MAX, &printable_size[0], &printable_size[1]); xco = (rect->xmax - U.widget_unit) - (int)printable_size[0]; yco = (rect->ymax - U.widget_unit); /* warning text (to clarify meaning of overlays) * - original color was red to match the icon, but that clashes badly with a less nasty border ``` Note that some of this bad overlap will be reduced as that warning is *raised*, not just as it is moved left. Just because that gizmo is round so it might tuck in nicely into its top-left corner at bit.

Sybren A. Stüvel added the

Module

Animation & Rigging

label 2023-03-09 10:15:19 +01:00

Sybren A. Stüvel commented

2023-03-09 10:21:25 +01:00

@PhoenixFlame101 thanks for revitalizing the old diff!

Could you update the PR description so that it follows the ingredients of a pull request? That way it's easier for others to jump in. Unfortunately it's not yet possible to embed videos here, but you can attach them.

@PhoenixFlame101 thanks for revitalizing the old diff! Could you update the PR description so that it follows the [ingredients of a pull request](https://wiki.blender.org/wiki/Process/Contributing_Code#Ingredients_of_a_Pull_Request)? That way it's easier for others to jump in. Unfortunately it's not yet possible to embed videos here, but you can attach them.

👍 1

Harley Acheson changed title from ~~WIP: Fix #97049: Auto-keyframing warning message shows on top of navigator~~ to WIP: Fix #97049: Auto-keyframe Status Message Overlaps Navigation Gizmo

2023-03-10 22:02:00 +01:00

Harley Acheson commented

2023-03-28 22:59:51 +02:00

@PhoenixFlame101 - If you need further help or advice, don't hesitate to ask.

Abhinav Chennubhotla force-pushed 97049-fix from eef2ee0a71 to 5c822f77f4

2023-03-29 09:57:51 +02:00

Compare

Abhinav Chennubhotla commented

2023-03-29 10:02:18 +02:00

First-time contributor

Sorry about the delay, I was busy with some other stuff.

Simply changing the font_id to BLF_set_default() from BLF_default() seems to fix the issue!

I have updated the commit to reflect this (as well as using a switch case instead of the if statements).

The fix is still somewhat broken for USER_MINI_AXIS_TYPE_MINIMAL, I couldn't find the size of this axis in code. Not sure how many people actually use this one, but it's worth mentioning nonetheless.

Sorry about the delay, I was busy with some other stuff. Simply changing the `font_id` to `BLF_set_default()` from `BLF_default()` seems to fix the issue! I have updated the commit to reflect this (as well as using a switch case instead of the if statements). The fix is still somewhat broken for `USER_MINI_AXIS_TYPE_MINIMAL`, I couldn't find the size of this axis in code. Not sure how many people actually use this one, but it's worth mentioning nonetheless.

Harley Acheson commented

2023-03-29 18:29:32 +02:00

Sorry about the delay, I was busy with some other stuff.

No worries! I was just worried that you were stuck.

Simply changing the font_id to BLF_set_default() from BLF_default() seems to fix the issue!

Awesome!

The fix is still somewhat broken for USER_MINI_AXIS_TYPE_MINIMAL, I couldn't find the size of this axis in code. Not sure how many people actually use this one, but it's worth mentioning nonetheless.

Fairly certain that is U.rvisize * U.pixelsize, drawn in draw_view_axis in blender\source\blender\editors\space_view3d\view3d_draw.cc. If not, let me know and I can dig a bit deeper.

> Sorry about the delay, I was busy with some other stuff. No worries! I was just worried that you were stuck. > Simply changing the `font_id` to `BLF_set_default()` from `BLF_default()` seems to fix the issue! Awesome! > The fix is still somewhat broken for `USER_MINI_AXIS_TYPE_MINIMAL`, I couldn't find the size of this axis in code. Not sure how many people actually use this one, but it's worth mentioning nonetheless. Fairly certain that is `U.rvisize * U.pixelsize`, drawn in `draw_view_axis` in `blender\source\blender\editors\space_view3d\view3d_draw.cc`. If not, let me know and I can dig a bit deeper.

Abhinav Chennubhotla commented

2023-03-29 18:50:00 +02:00

First-time contributor

Weirdly enough, U.pixelsize seems to be 0 for me. U.rvisize works though!

It's a little too exact though, should I add an extra offset on top of the rvisize?

Weirdly enough, `U.pixelsize` seems to be 0 for me. `U.rvisize` works though! It's a little *too* exact though, should I add an extra offset on top of the rvisize? ![image](/attachments/4f3b0c9f-ec75-4fd1-9a99-cc65ce6e5e80)

image.png

21 KiB

Harley Acheson commented

2023-03-29 19:36:57 +02:00

Weirdly enough, U.pixelsize seems to be 0 for me. U.rvisize works though!

Unless I need more coffee that U.pixelsize should never be less than 1.0f. And if it were zero then that draw_view_axis would break. Is it possible that the code you are in is called often and early in that you can just not do work if that value is zero? I mean if that value is zero it would be before the userdef was even populated in WM_window_set_dpi and all sorts would be odd in there.

FYI, We have a few things that change with the UI scale.

U.pixelsize is best thought of as our "minimum feature size" and indicates the how many pixels wide to draw lines. The best way to see the effect of that is to be showing the mini axis then go to Edit / Preferences / Interface / "Line Width" and change to "Thick". You will see many lines get twice as wide, including those of the mini-axis which obviously changes its size.

UI_SCALE_FAC is more directly related to the user's resolution scale (and DPI scaling of each monitor). Best thought of as our final multiplier for more UI items.

U.widget_unit is UI-sized value that combines a bit of both of the above and can be useful for setting some sizes. Basically the size of a single square button in the UI, so it gets larger with the resolution scale, but also gets a bit bigger with "wide lines".

It's a little too exact though, should I add an extra offset on top of the rvisize?

Yes, you should be able to multiply some factor, like 1.2f or something, to that U.rvisize * U.pixelsize and your text should stay a consistant distance away. But just make sure to check changing both the resolution scale and the "line width".

> Weirdly enough, `U.pixelsize` seems to be 0 for me. `U.rvisize` works though! Unless I need more coffee that `U.pixelsize` should never be less than 1.0f. And if it were zero then that `draw_view_axis` would break. Is it possible that the code you are in is called often and early in that you can just not do work if that value is zero? I mean if that value is zero it would be before the userdef was even populated in `WM_window_set_dpi` and all sorts would be odd in there. FYI, We have a few things that change with the UI scale. U.pixelsize is best thought of as our "minimum feature size" and indicates the how many pixels wide to draw lines. The best way to see the effect of that is to be showing the mini axis then go to Edit / Preferences / Interface / "Line Width" and change to "Thick". You will see many lines get twice as wide, including those of the mini-axis which obviously changes its size. UI_SCALE_FAC is more directly related to the user's resolution scale (and DPI scaling of each monitor). Best thought of as our final multiplier for more UI items. U.widget_unit is UI-sized value that combines a bit of both of the above and can be useful for setting some sizes. Basically the size of a single square button in the UI, so it gets larger with the resolution scale, but also gets a bit bigger with "wide lines". > It's a little *too* exact though, should I add an extra offset on top of the rvisize? Yes, you should be able to multiply some factor, like 1.2f or something, to that `U.rvisize * U.pixelsize` and your text *should* stay a consistant distance away. But just make sure to check changing both the resolution scale and the "line width".

Abhinav Chennubhotla added 1 commit 2023-03-30 05:21:53 +02:00

3fab8dea45

Fix offset value for USER_MINI_AXIS_TYPE_MINIMAL

Abhinav Chennubhotla commented

2023-03-30 05:29:23 +02:00

First-time contributor

I did U.rvisize * 1.2f, and it works! Still don't know what to do with the U.pixelsize though, since it's 0 and multiplying it would mean the product becomes 0.

I did `U.rvisize * 1.2f`, and it works! Still don't know what to do with the U.pixelsize though, since it's 0 and multiplying it would mean the product becomes 0. ![image](/attachments/78a19b22-0b58-4cf5-81a1-da90168bdb1f)

image.png

22 KiB

Harley Acheson commented

2023-03-30 07:04:18 +02:00

I did U.rvisize * 1.2f, and it works! Still don't know what to do with the U.pixelsize though, since it's 0 and multiplying it would mean the product becomes 0.

No worries, will apply your patch and take a look at that in the morning.

> I did `U.rvisize * 1.2f`, and it works! Still don't know what to do with the U.pixelsize though, since it's 0 and multiplying it would mean the product becomes 0. No worries, will apply your patch and take a look at that in the morning.

Sybren A. Stüvel commented

2023-03-30 16:12:29 +02:00

I'm sorry, I completely forgot to post a comment here. This PR was discussed in an Animation & Rigging module meeting. The consensus was that having the message ("Auto Keying on") overlapping the 3D Viewport is distracting.

@PhoenixFlame101 could you adjust the patch so that the message is drawn next to the 'numbers' in the header? Something like this:

Alternatively it could be drawn more to the left, so that it is closer to the numerical values. It will be a challenge to avoid it jumping left & right when the numbers change, which is why I put it on the right-hand side in my mockup.

I'm sorry, I completely forgot to post a comment here. This PR was discussed in [an Animation & Rigging module meeting](https://devtalk.blender.org/t/2023-03-09-animation-rigging-module-meeting/28204). The consensus was that having the message ("Auto Keying on") overlapping the 3D Viewport is distracting. @PhoenixFlame101 could you adjust the patch so that the message is drawn next to the 'numbers' in the header? Something like this: ![mockup showing the message in the top bar, instead of overlapping the 3D viewport](/attachments/9d64e047-8904-40ae-bd81-f1a10a4faf3f) Alternatively it could be drawn more to the left, so that it is closer to the numerical values. It will be a challenge to avoid it jumping left & right when the numbers change, which is why I put it on the right-hand side in my mockup.

image.png

139 KiB

Sybren A. Stüvel requested review from Sybren A. Stüvel 2023-03-30 16:13:09 +02:00

Sybren A. Stüvel added this to the Animation & Rigging project 2023-03-30 16:13:15 +02:00

Abhinav Chennubhotla commented

2023-04-03 16:02:07 +02:00

First-time contributor

After doing some research, I found this code responsible for writing the text on the left side of the header while grabbing, rotating etc. : headerRotation() and similarly headerTranslation() and headerResize().

These use the BLI_snprintf() function to write the text (to a buffer?), but I am unable to understand where the buffer is flushed and the text is written. Modifying these functions displays the code, but trying to make my own call to BLI_snprintf() doesn't.

Any guidance on this?

After doing some research, I found this code responsible for writing the text on the left side of the header while grabbing, rotating etc. : [`headerRotation()`](https://projects.blender.org/blender/blender/src/commit/da764ee3573782187b11479ea880d10a7cdccc7e/source/blender/editors/transform/transform_mode.c#L518) and similarly `headerTranslation()` and `headerResize()`. These use the `BLI_snprintf()` function to write the text (to a buffer?), but I am unable to understand where the buffer is flushed and the text is written. Modifying these functions displays the code, but trying to make my own call to `BLI_snprintf()` doesn't. Any guidance on this?

Harley Acheson commented

2023-04-03 17:29:06 +02:00

@PhoenixFlame101 - U.pixelsize seems to be 0 for me.

My guess here is that you examined what changes when a user selects "wide lines" and were actually looking at U.ui_line_width, which is zero for normal lines. This can't really be used in our calculations, instead we use that and other settings to calculate something usable: U.pixelsize. I've got some of that written down here: https://wiki.blender.org/wiki/Source/Interface/Preferences_and_Defaults

When I look at that section of code one thing to note is that I think the simple axes changes size and position with wide lines while the regular gizmo does not. So I get a result that seems to work in various settings (although not tested exhaustively) like this:

...
  switch ((eUserpref_MiniAxisType)U.mini_axis_type) {
    case USER_MINI_AXIS_TYPE_GIZMO:
      offset = U.gizmo_size_navigate_v3d * U.scale_factor;
      break;
    case USER_MINI_AXIS_TYPE_MINIMAL:
      offset = U.rvisize * U.pixelsize * 2.0f;
      break;
    case USER_MINI_AXIS_TYPE_NONE:
      offset = 25 * U.scale_factor;
      break;
  }

  xco = (rect->xmax - U.widget_unit) - (int)printable_size[0] - offset;
...

but I am unable to understand where the buffer is flushed and the text is written

I think what you are looking for is ED_area_status_text. If you search for uses of that you will see you just call it with any text and it shows up in that area. Called with NULL it is cleared.

> @PhoenixFlame101 - `U.pixelsize` seems to be 0 for me. My guess here is that you examined what changes when a user selects "wide lines" and were actually looking at U.ui_line_width, which is zero for normal lines. This can't really be used in our calculations, instead we use that and other settings to calculate something usable: U.pixelsize. I've got some of that written down here: https://wiki.blender.org/wiki/Source/Interface/Preferences_and_Defaults When I look at that section of code one thing to note is that I think the simple axes changes size and position with wide lines while the regular gizmo does not. So I get a result that seems to work in various settings (although not tested exhaustively) like this: ```C ... switch ((eUserpref_MiniAxisType)U.mini_axis_type) { case USER_MINI_AXIS_TYPE_GIZMO: offset = U.gizmo_size_navigate_v3d * U.scale_factor; break; case USER_MINI_AXIS_TYPE_MINIMAL: offset = U.rvisize * U.pixelsize * 2.0f; break; case USER_MINI_AXIS_TYPE_NONE: offset = 25 * U.scale_factor; break; } xco = (rect->xmax - U.widget_unit) - (int)printable_size[0] - offset; ... ``` > but I am unable to understand where the buffer is flushed and the text is written I think what you are looking for is `ED_area_status_text`. If you search for uses of that you will see you just call it with any text and it shows up in that area. Called with NULL it is cleared.

Sybren A. Stüvel commented

2023-04-17 16:53:22 +02:00

@PhoenixFlame101 Is the WIP status of this pull request still relevant? If you want someone to re-review your patch, please remove the WIP prefix.

Abhinav Chennubhotla commented

2023-04-23 10:49:42 +02:00

First-time contributor

The code works and is mergeable, but it doesn't do what you outlined here.

I tried using ED_area_status_text, but it simply replaced the original text that displayed position, rotation etc. In your mockup you displayed the message on the right, which requires a custom function I believe. I still haven't gotten that working so I kept the WIP prefix.

The code works and is mergeable, but it doesn't do what you outlined [here](https://projects.blender.org/blender/blender/pulls/105565#issuecomment-911269). I tried using `ED_area_status_text`, but it simply replaced the original text that displayed position, rotation etc. In your mockup you displayed the message on the right, which requires a custom function I believe. I still haven't gotten that working so I kept the WIP prefix.

Sybren A. Stüvel commented

2023-05-01 16:13:10 +02:00

👍 that's all fine then, thanks for the update!

:+1: that's all fine then, thanks for the update!

Sybren A. Stüvel referenced this pull request

2023-07-21 10:26:11 +02:00

UI: Updated Auto Keying Button Icon #105574

Nate Rupsis referenced this pull request

2023-08-21 19:20:30 +02:00

Fix #97049: Auto-keyframe Status Message Overlaps Navigation Gizmo #111356

Nate Rupsis commented

2023-08-24 15:27:14 +02:00