Fix #111841: Prevent node title text overlap with icon #112940

Semen Komissarov · 2023-09-27T09:34:29+02:00

Semen Komissarov commented

2023-09-27 09:34:29 +02:00

First-time contributor

In Geometry Nodes editor, some nodes have title text overlap with icon, which can be observed via rescaling, e.g. in Node Context Menu:

Add -> Group -> "other node group name"
Add -> Hair -> some menus have overlapping title icon

Tested on two monitors, both running Windows: 4k and laptop 1080p. Probably requires approve of mac / linux user.

In Geometry Nodes editor, some nodes have title text overlap with icon, which can be observed via rescaling, e.g. in Node Context Menu: - Add -> Group -> "other node group name" - Add -> Hair -> some menus have overlapping title icon Tested on two monitors, both running Windows: 4k and laptop 1080p. Probably requires approve of mac / linux user.

Semen Komissarov changed title from ~~Fix #111841: prevent node title text overlap with icon (e.g. add hair, add link to other node group)~~ to Draft: Fix #111841: prevent node title text overlap with icon (e.g. add hair, add link to other node group)

2023-09-27 09:39:12 +02:00

Semen Komissarov changed title from ~~Draft: Fix #111841: prevent node title text overlap with icon (e.g. add hair, add link to other node group)~~ to Fix #111841: prevent node title text overlap with icon (e.g. add hair, add link to other node group)

2023-09-27 09:42:58 +02:00

Semen Komissarov referenced this pull request

2023-09-27 09:44:28 +02:00

Nodes: Node Name are overlap Icons in header #111841

Iliya Katushenock added this to the Nodes & Physics project 2023-09-27 13:33:05 +02:00

Iliya Katushenock added the

Module

User Interface

label 2023-09-27 13:33:09 +02:00

Iliya Katushenock reviewed 2023-09-27 13:37:17 +02:00

Iliya Katushenock left a comment

Thanks for fix. I haven't gone deep into this, but i believe that text size and border should have a more formal definition.

source/blender/editors/space_node/node_draw.cc Outdated

						
				@ -2655,6 +2655,9 @@ static void node_draw_extra_info_panel(const Scene *scene,

				  }

				}

				// prevents node title text overlap with icon. (e.g. add hair, add link to other node group)

Iliya Katushenock commented

2023-09-27 13:34:46 +02:00

https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Comments

Semen Komissarov commented

2023-09-27 14:46:06 +02:00

First-time contributor

adjusted

Semen Komissarov commented

2023-09-27 15:49:36 +02:00

First-time contributor

removed in last commit

mod_moder marked this conversation as resolved

source/blender/editors/space_node/node_draw.cc Outdated

						
				@ -2656,2 +2656,4 @@

				}

				// prevents node title text overlap with icon. (e.g. add hair, add link to other node group)

				#define NODE_TITLE_ICON_OFFSET_MAGIC (18.0f + 12.0f)

Iliya Katushenock commented

2023-09-27 13:34:04 +02:00

Please, use static constexpr float instead of macros.

Please, use `static constexpr float` instead of macros.

Semen Komissarov commented

2023-09-27 14:46:19 +02:00

First-time contributor

fixed

Semen Komissarov commented

2023-09-27 15:49:44 +02:00

First-time contributor

removed in last commit

mod_moder marked this conversation as resolved

Semen Komissarov commented

2023-09-27 15:41:49 +02:00

First-time contributor

Thanks for fix. I haven't gone deep into this, but i believe that text size and border should have a more formal definition.

Thanks for heads up. I think I got perfect alignment for group nodes, replacing magic with icon size and reasonable constants.

first attempt looked impresize, briefing text prematurely, before touching icon edge

> Thanks for fix. I haven't gone deep into this, but i believe that text size and border should have a more formal definition. Thanks for heads up. I think I got perfect alignment for group nodes, replacing magic with icon size and reasonable constants. first attempt looked impresize, briefing text prematurely, before touching icon edge

Semen Komissarov force-pushed geom-nodes-title-text-icon-overlap from 610a37e49f to 5964ff7acb

2023-09-27 21:56:22 +02:00

Compare

Iliya Katushenock changed title from ~~Fix #111841: prevent node title text overlap with icon (e.g. add hair, add link to other node group)~~ to Fix #111841: Prevent node title text overlap with icon

2023-10-07 15:38:10 +02:00

Iliya Katushenock requested changes 2023-10-07 15:49:24 +02:00

Iliya Katushenock left a comment

Sorry for delay, i just checked this:

Bug still occur.

Sorry for delay, i just checked this: ![image](/attachments/5f7ad1e3-4dff-41e2-a042-eb43cbd96c42) Bug still occur.

image.png

19 KiB

Semen Komissarov commented

2023-10-07 20:43:18 +02:00

First-time contributor

@mod_moder
please tell menu name and how to access it.
I've enumerated 2 fixed cases:

Add -> Group -> "other node group name"
Add -> Hair -> some menus have overlapping title icon

and they were using same method for offset
but your menu on screenshot is different, I cant find it

@mod_moder please tell menu name and how to access it. I've enumerated 2 fixed cases: ``` Add -> Group -> "other node group name" Add -> Hair -> some menus have overlapping title icon ``` and they were using same method for offset\ but your menu on screenshot is different, I cant find it

Iliya Katushenock commented

2023-10-07 20:47:05 +02:00

No description provided.

2023-10-07 21-46-19.mp4

8.8 MiB

Semen Komissarov commented

2023-10-07 23:33:52 +02:00

First-time contributor

I have a feeling you're not on my branch, could you double check you pulled last changes please?

_ (Unsaved) - Blender 4.1 Alpha 2023-10-07 23-19-21.mp4

4.5 MiB

Iliya Katushenock commented

2023-10-08 00:03:19 +02:00

I just did recheck and i pretty sure what that is not worked. It might depend on screen size thought.

Semen Komissarov commented

2023-10-08 15:12:57 +02:00

First-time contributor

figured out it depends on desktop resolution scale. it works on desktop scale 150% but doesnt work for 100%. (checked on 4k monitor where scale was 150% and then reproduced bug on laptop 1080p with scale 100%. when changed laptop to 150% - no bug, perfect fit. 125% has a little overlap).
which is a bit weird. previous solution with hardcoded constants worked on both machines.
now need to find some other convenient variable representing desktop scale, or check other size scale dependent variables. in current approach, looks like UI_SCALE_FAC is not enough

figured out it depends on desktop resolution scale. it works on desktop scale 150% but doesnt work for 100%. (checked on 4k monitor where scale was 150% and then reproduced bug on laptop 1080p with scale 100%. when changed laptop to 150% - no bug, perfect fit. 125% has a little overlap). which is a bit weird. previous solution with hardcoded constants worked on both machines. now need to find some other convenient variable representing desktop scale, or check other size scale dependent variables. in current approach, looks like UI_SCALE_FAC is not enough

Iliya Katushenock commented

2023-10-08 15:14:47 +02:00

U.widget_unit?

`U.widget_unit`?

Semen Komissarov commented

2023-10-08 15:33:29 +02:00

First-time contributor

replaced UI_SCALE_FAC for constant 1.5 (150%), which fits perfectly scales 100 to 300 %. Sometimes empirical magic beats reasoning).
thanks for reproducing issue again

please review

replaced UI_SCALE_FAC for constant 1.5 (150%), which fits perfectly scales 100 to 300 %. Sometimes empirical magic beats reasoning).\ thanks for reproducing issue again please review

Iliya Katushenock approved these changes 2023-10-08 15:48:25 +02:00

Iliya Katushenock left a comment

Now that works for me.

source/blender/editors/space_node/node_draw.cc Outdated

						
				@ -2744,6 +2744,14 @@ static void node_draw_extra_info_panel(const Scene *scene,

				  }

				}

				BLI_INLINE float node_draw_get_node_group_width(const float iconofs, const rctf &rct)

Iliya Katushenock commented

2023-10-08 15:47:23 +02:00

I not sure if BLI_INLINE is really needed here.

I not sure if `BLI_INLINE` is really needed here.

Semen Komissarov commented

2023-10-08 15:59:11 +02:00

First-time contributor

it changes to __forceinline, reducing compilation time in some cases? to not rely on compiler judgement. I just saw it is used everywhere and documented https://wiki.blender.org/wiki/Style_Guide/C_Cpp as

BLI_INLINE: Portable prefix for inline functions.

it changes to __forceinline, reducing compilation time in some cases? to not rely on compiler judgement. I just saw it is used everywhere and documented https://wiki.blender.org/wiki/Style_Guide/C_Cpp as > BLI_INLINE: Portable prefix for inline functions.

source/blender/editors/space_node/node_draw.cc Outdated

						
				@ -2746,1 +2746,4 @@

				BLI_INLINE float node_draw_get_node_group_width(const float iconofs, const rctf &rct)

				{

				  const float iconbutw = NODE_HEADER_ICON_SIZE;

Iliya Katushenock commented

2023-10-08 15:46:51 +02:00

It is the necessary variable?

Semen Komissarov commented

2023-10-08 16:03:51 +02:00

First-time contributor

to be consistent with naming in other functions. in case of any change to iconbutw variable in other functions, this hints "consider also changing it here"

to be consistent with naming in other functions. in case of any change to `iconbutw` variable in other functions, this hints "consider also changing it here"

source/blender/editors/space_node/node_draw.cc Outdated

						
				@ -2747,0 +2747,4 @@

				BLI_INLINE float node_draw_get_node_group_width(const float iconofs, const rctf &rct)

				{

				  const float iconbutw = NODE_HEADER_ICON_SIZE;

				  // it was iconbutw * UI_SCALE_FAC, where UI_SCALE_FAC == 1.5 fits perfectly (150% resolution

Iliya Katushenock commented

2023-10-08 15:45:36 +02:00

Comment style

Semen Komissarov commented

2023-10-08 16:08:09 +02:00

First-time contributor

sorry, adjusted

Leon Schittek commented

2023-10-08 16:51:44 +02:00

Hi, thanks for working on this.

I haven't looked too deeply into the problem, but to me it feels like the current PR is a bit more complicated than it needs to be.
This small diff seems to do the trick for me, but maybe I'm missing something:

diff --git a/source/blender/editors/space_node/node_draw.cc b/source/blender/editors/space_node/node_draw.cc
index 1e7c0b8edb8..3dada938251 100644
--- a/source/blender/editors/space_node/node_draw.cc
+++ b/source/blender/editors/space_node/node_draw.cc
@@ -2977,13 +2977,14 @@ static void node_draw_basis(const bContext &C,
   char showname[128];
   bke::nodeLabel(&ntree, &node, showname, sizeof(showname));

+  const int labelbut_x = rct.xmin + NODE_MARGIN_X + 0.4f;
   uiBut *but = uiDefBut(&block,
                         UI_BTYPE_LABEL,
                         0,
                         showname,
-                        int(rct.xmin + NODE_MARGIN_X + 0.4f),
+                        labelbut_x,
                         int(rct.ymax - NODE_DY),
-                        short(iconofs - rct.xmin - (18.0f * UI_SCALE_FAC)),
+                        short(iconofs - labelbut_x),
                         short(NODE_DY),
                         nullptr,
                         0,

To elaborate: iconofs is representing the x location of the drawing icons (which is offset to the left with each icon that is drawn). So to properly calculate the available space for the label we just check the distance between where our label start (labelbut_x ) and where it's supposed to end (iconofs).

I think the current diff works because iconbutw * 1.5 works out to be roughly the same as NODE_MARGIN_X + 0.4f.

Hi, thanks for working on this. I haven't looked too deeply into the problem, but to me it feels like the current PR is a bit more complicated than it needs to be. This small diff seems to do the trick for me, but maybe I'm missing something: ```Diff diff --git a/source/blender/editors/space_node/node_draw.cc b/source/blender/editors/space_node/node_draw.cc index 1e7c0b8edb8..3dada938251 100644 --- a/source/blender/editors/space_node/node_draw.cc +++ b/source/blender/editors/space_node/node_draw.cc @@ -2977,13 +2977,14 @@ static void node_draw_basis(const bContext &C, char showname[128]; bke::nodeLabel(&ntree, &node, showname, sizeof(showname)); + const int labelbut_x = rct.xmin + NODE_MARGIN_X + 0.4f; uiBut *but = uiDefBut(&block, UI_BTYPE_LABEL, 0, showname, - int(rct.xmin + NODE_MARGIN_X + 0.4f), + labelbut_x, int(rct.ymax - NODE_DY), - short(iconofs - rct.xmin - (18.0f * UI_SCALE_FAC)), + short(iconofs - labelbut_x), short(NODE_DY), nullptr, 0, ``` To elaborate: `iconofs` is representing the x location of the drawing icons (which is offset to the left with each icon that is drawn). So to properly calculate the available space for the label we just check the distance between where our label start (`labelbut_x `) and where it's supposed to end (`iconofs`). I think the current diff works because `iconbutw * 1.5` works out to be roughly the same as `NODE_MARGIN_X + 0.4f`.

Semen Komissarov commented

2023-10-08 20:19:57 +02:00

First-time contributor

@lone_noel your solution looks better, will you prepare PR?
also makes sense replace hardcoded magic in node_draw_hidden ? next function in file, similar hardcoded offsets

@lone_noel your solution looks better, will you prepare PR? also makes sense replace hardcoded magic in `node_draw_hidden` ? next function in file, similar hardcoded offsets

Leon Schittek commented

2023-10-09 07:42:01 +02:00

@lone_noel your solution looks better, will you prepare PR?

I probably won't have time before the end of the week. So if you confirmed that diff works for you, as well, feel free to just update this PR.

also makes sense replace hardcoded magic in node_draw_hidden ? next function in file, similar hardcoded offsets

node_draw_hidden is a bit different, since these icons aren't shown on hidden nodes.
On hidden nodes there's that scale widget on the right that can conflict with the label. Especially since that widget scales a bit oddly currently (the width is constant in screen space).

If you want you could take a stab at fixing that in a similar fashion, too, and maybe even improve the width scaling of the widget while you're at it.

> @lone_noel your solution looks better, will you prepare PR? I probably won't have time before the end of the week. So if you confirmed that diff works for you, as well, feel free to just update this PR. > also makes sense replace hardcoded magic in `node_draw_hidden` ? next function in file, similar hardcoded offsets `node_draw_hidden` is a bit different, since these icons aren't shown on hidden nodes. On hidden nodes there's that scale widget on the right that can conflict with the label. Especially since that widget scales a bit oddly currently (the width is constant in screen space). If you want you could take a stab at fixing that in a similar fashion, too, and maybe even improve the width scaling of the widget while you're at it.

👍 1

Semen Komissarov force-pushed geom-nodes-title-text-icon-overlap from 979a4eeec6 to 5f49efc084

2023-10-09 10:23:36 +02:00

Compare

Semen Komissarov added 1 commit 2023-10-09 10:25:51 +02:00

a117d70272 Fix #111841 : Prevent node title text overlap with icon

Semen Komissarov commented

2023-10-09 10:41:49 +02:00

First-time contributor

@mod_moder switched to Leon's solution, please review again

Iliya Katushenock commented

2023-10-09 15:10:20 +02:00

Yep, that still works
Thanks all for that

Yep, that still works Thanks all for that

Leon Schittek approved these changes 2024-04-05 21:09:20 +02:00

Leon Schittek left a comment

It seems like this fell through the cracks. Sorry about that, @Semen-Komissarov!

I just tested it and the change still applies cleanly and fixes the issue.

It seems like this fell through the cracks. Sorry about that, @Semen-Komissarov! I just tested it and the change still applies cleanly and fixes the issue.

❤️ 1

Merge conflict checking is in progress. Try again in few moments.

View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u geom-nodes-title-text-icon-overlap:Semen-Komissarov-geom-nodes-title-text-icon-overlap

git checkout Semen-Komissarov-geom-nodes-title-text-icon-overlap

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Fix #111841: Prevent node title text overlap with icon #112940

Checkout