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

Closed
Abhinav Chennubhotla wants to merge 2 commits from PhoenixFlame101/blender:97049-fix into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
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:

autokeywarning.png

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.
Author
First-time contributor

To reiterate what was mentioned in the older diff,

image

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)
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
Member

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

@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.
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
Member

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

@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
Author
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.
Member

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.
Author
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?

image

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)
Member

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
Author
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.

image

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)
Member

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.

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:

mockup showing the message in the top bar, instead of overlapping the 3D viewport

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.
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
Author
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?
Member

@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.

@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.

@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.
Author
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.

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

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

Landed in #111356

Landed in [#111356](https://projects.blender.org/blender/blender/pulls/111356)
Nate Rupsis closed this pull request 2023-08-24 15:27:16 +02:00

Pull request closed

Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Code Documentation
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
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
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
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
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
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
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 Milestone
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#105565
No description provided.