UI: Add elbow in hierarchy line and change padding a bit #116200

Sybren A. Stüvel · 2023-12-14T18:29:02+01:00

Sybren A. Stüvel commented

2023-12-14 18:29:02 +01:00

Two small alterations to the layout of Tree View components.

End the vertical hierarchy line in an "elbow" shape. This makes it visually clearer where the children end, and a new parent item begins.
Slightly change the padding rules, so that text aligns regardless of whether the collapse-chevron is drawn or not. This also fixes the indentation of child-less root items in the tree.

Before	After
Geometry Nodes Spreadsheet:

Bone Collections:

The bone collection hierarchy is ~~an upcoming feature, see #115945~~ currently in main and will be included in 4.1. The bone collection tree can be found in the Armature properties panel.

Two small alterations to the layout of Tree View components. 1. End the vertical hierarchy line in an "elbow" shape. This makes it visually clearer where the children end, and a new parent item begins. 2. Slightly change the padding rules, so that text aligns regardless of whether the collapse-chevron is drawn or not. This also fixes the indentation of child-less root items in the tree. | Before | After | | -- | -- | | Geometry Nodes Spreadsheet: | | | ![GN spreadsheet tree before](/attachments/6119e1a8-f6a7-4593-911d-e5c84d5e752b) | ![GN spreadsheet tree after](/attachments/77f3ce64-71c4-44d8-be9b-cdbc3ccd1551) | | Bone Collections: | | | ![Bone collections before](/attachments/eac4aaa0-6593-453f-94f6-cb9bfe04367b) | ![Bone collections after](/attachments/43f226c8-8cad-4388-8bec-2cb15e555e6d) | The bone collection hierarchy is ~~an upcoming feature, see #115945~~ currently in `main` and will be included in 4.1. The bone collection tree can be found in the Armature properties panel.

image.png

64 KiB

image.png

71 KiB

image.png

23 KiB

image.png

22 KiB

image.png

14 KiB

image.png

56 KiB

image.png

56 KiB

image.png

22 KiB

image.png

14 KiB

image.png

23 KiB

❤️ 1

Sybren A. Stüvel added the

Module

User Interface

label 2023-12-14 18:36:37 +01:00

Sybren A. Stüvel requested review from Julian Eisel 2023-12-14 18:36:44 +01:00

Harley Acheson commented

2023-12-14 18:53:05 +01:00

What do you think about extending the elbow down just a bit, to the bottom of the last row? Otherwise I get this sense that it is pointing at the last item and it then looks more special than its siblings before it.

What do you think about extending the elbow down just a bit, to the bottom of the last row? Otherwise I get this sense that it is pointing at the last item and it then looks more special than its siblings before it. ![image](/attachments/a9edf422-ba5b-4354-8675-ff7f9703ee77)

image.png

23 KiB

❤️ 1

Harley Acheson commented

2023-12-14 19:02:27 +01:00

Or... maybe the line could contain all the children? It then separates them from the parent a bit.

Or... maybe the line could _contain_ all the children? It then separates them from the parent a bit. ![image](/attachments/0ab5c34c-2ee7-4f16-aed7-1b32d99c6a19)

image.png

23 KiB

Brecht Van Lommel commented

2023-12-14 19:11:11 +01:00

To me this looks like it's pointing at the last child item, and that there is something special about that item.

I don't remember seeing this kind of elbow in other user interface toolkits.

I found something similar in this GitHub design which is more rounded and lower.
https://user-images.githubusercontent.com/2313998/187719332-3ef9aa9e-476f-4e5a-93fa-5fbd203ece4a.png

But actually looking on the website I see just straight lines. Personally I find only the straight lines clear enough.

To me this looks like it's pointing at the last child item, and that there is something special about that item. I don't remember seeing this kind of elbow in other user interface toolkits. I found something similar in this GitHub design which is more rounded and lower. https://user-images.githubusercontent.com/2313998/187719332-3ef9aa9e-476f-4e5a-93fa-5fbd203ece4a.png But actually looking on the website I see just straight lines. Personally I find only the straight lines clear enough.

👍 2

Harley Acheson commented

2023-12-14 20:11:59 +01:00

I find our indentation rules to be odd with us not indenting children consistently.

In the following "r0_child1" is a clearly a child of "root_0" and it is indented from its parent.

But what is with "r0c1_child0" (highlighted with yellow arrow)? It is child of "r0_child1" above it, yet it is not indented. If we add something into that will it then move to the right? So odd to show parents and children at same indentation level, especially when we do
use indentation.

I find our indentation rules to be odd with us not indenting children consistently. In the following "r0_child1" is a clearly a child of "root_0" and it is indented from its parent. But what is with "r0c1_child0" (highlighted with yellow arrow)? It is child of "r0_child1" above it, yet it is not indented. If we add something into that will it then move to the right? So odd to show parents and children at same indentation level, especially when we do use indentation. ![image](/attachments/03626e4c-6364-46fe-bc9f-72988329f676)

image.png

30 KiB

👍 1

Sybren A. Stüvel commented

2023-12-15 08:39:01 +01:00

@Harley wrote:

What do you think about extending the elbow down just a bit, to the bottom of the last row? Otherwise I get this sense that it is pointing at the last item and it then looks more special than its siblings before it.

That works for me too. That's actually what I implemented earlier, but @nathanvegdahl and @JulianEisel weren't big fans.

In the following "r0_child1" is a clearly a child of "root_0" and it is indented from its parent.

Yeah, but it's indented because it has children of its own. Not because it is a child itself.

But what is with "r0c1_child0" (highlighted with yellow arrow)? It is child of "r0_child1" above it, yet it is not indented. If we add something into that will it then move to the right? So odd to show parents and children at same indentation level, especially when we do
use indentation.

This confusion is indeed the main reason why we tried to improve the situation.

@brecht wrote:

Personally I find only the straight lines clear enough.

For me, the current situation of the tree view is super confusing. The biggest issue IMO is the inconsistent indentation of the text that Harley described. Having a bit of a corner at the bottom of the hierarchy line also helped me to get things visually clearer.

@Harley wrote: > What do you think about extending the elbow down just a bit, to the bottom of the last row? Otherwise I get this sense that it is pointing at the last item and it then looks more special than its siblings before it. That works for me too. That's actually what I implemented earlier, but @nathanvegdahl and @JulianEisel weren't big fans. > In the following "r0_child1" is a clearly a child of "root_0" and it is indented from its parent. Yeah, but it's indented because it has children of its own. Not because it is a child itself. > But what is with "r0c1_child0" (highlighted with yellow arrow)? It is child of "r0_child1" above it, yet it is not indented. If we add something into that will it then move to the right? So odd to show parents and children at same indentation level, especially when we do > use indentation. This confusion is indeed the main reason why we tried to improve the situation. @brecht wrote: > Personally I find only the straight lines clear enough. For me, the current situation of the tree view is super confusing. The biggest issue IMO is the inconsistent indentation of the text that Harley described. Having a bit of a corner at the bottom of the hierarchy line also helped me to get things visually clearer.

❤️ 1

Harley Acheson commented

2023-12-15 20:15:15 +01:00

I find that if there is proper indentation and a change of spacing then I don't feel the need for the top elbow, and the relationship line can even be reduced in color impact.

I find that if there is proper indentation and a change of spacing then I don't feel the need for the top elbow, and the relationship line can even be reduced in color impact. ![image](/attachments/8725d897-2ac9-440a-9c94-a96230e730c3) ![image](/attachments/b7d0615e-a2e2-461f-ae7c-8d946018df88)

image.png

60 KiB

image.png

29 KiB

👍 3

Sybren A. Stüvel commented

2023-12-18 10:36:31 +01:00

Looks good to me, Harley!

Sybren A. Stüvel commented

2023-12-18 15:45:42 +01:00

@brecht @JulianEisel what do you think of Harley's proposal? I think it looks good, and I'd be happy if we can land his changes.

Brecht Van Lommel commented

2023-12-18 16:10:32 +01:00

I still find that the elbow gets in the way more than it helps. At a glance feels like it's pointing at the last item.

👍 1

Harley Acheson commented

2023-12-18 20:55:13 +01:00

I still find that the elbow gets in the way more than it helps. At glance feels like it's pointing at the last item.

I can't really disagree with that.

I think the biggest problem is the indentation and this line is being used to make up for it.

> I still find that the elbow gets in the way more than it helps. At glance feels like it's pointing at the last item. I can't really disagree with that. I think the biggest problem is the indentation and this line is being used to make up for it.

Demeter Dzadik commented

2023-12-19 14:33:00 +01:00

I would advocate to take inspiration from the Outliner. After all, it is the OG tree view.

One of the main reasons I wanted to give feedback on the new nested bone collections UI is the fact that parent and child are at the same indentation level. This is visually super confusing. It doesn't matter much for a UI like the spreadsheet editor where there's only ever one level of nesting, but look at the Outliner. There is always space reserved on the left for a drop-down icon, even if the element has no children yet. When it gets children, its indentation doesn't change.

I agree with Brecht that the elbow is misleading. And like Harley said, once the indentation is correct, the elbows are no longer needed.

I would advocate to take inspiration from the Outliner. After all, it is the OG tree view. One of the main reasons I wanted to give feedback on the new nested bone collections UI is the fact that parent and child are at the same indentation level. This is visually super confusing. It doesn't matter much for a UI like the spreadsheet editor where there's only ever one level of nesting, but look at the Outliner. There is always space reserved on the left for a drop-down icon, even if the element has no children yet. When it gets children, its indentation doesn't change. I agree with Brecht that the elbow is misleading. And like Harley said, once the indentation is correct, the elbows are no longer needed.

👍 1

Sybren A. Stüvel commented

2023-12-19 17:51:16 +01:00

I still find that the elbow gets in the way more than it helps. At a glance feels like it's pointing at the last item.

This seems to be rather subjective, then. The straight line currently in main to me feels like it's pointing to the next sibling, but only if the current item has any children. To me the elbow reads as "children end here", also at a glance. Not sure what's misleading about that.

I think the biggest problem is the indentation and this line is being used to make up for it.

That's definitely true, the indentation is what really got me confused. I still don't quite like the lack of consistency between indentation of siblings with and without children. For me the line + elbow helps, but if that's not going to get approved, we need a better way to indent things.

> I still find that the elbow gets in the way more than it helps. At a glance feels like it's pointing at the last item. This seems to be rather subjective, then. The straight line currently in `main` to me feels like it's pointing to the next sibling, but only if the current item has any children. To me the elbow reads as "children end here", also at a glance. Not sure what's misleading about that. > I think the biggest problem is the indentation and this line is being used to make up for it. That's definitely true, the indentation is what really got me confused. I still don't quite like the lack of consistency between indentation of siblings with and without children. For me the line + elbow helps, but if that's not going to get approved, we need a better way to indent things. ![image](/attachments/8a590e34-e26e-4096-875f-f293287c2960)

image.png

29 KiB

👍 1

Demeter Dzadik commented

2023-12-19 20:10:22 +01:00

In file browser softwares that have indent indicators other than just empty space, they usually have an elbow at the center of every entry, not just the bottom of the last one. That might feel more familiar, if nothing else. But most modern software seems to just use empty space. (Nautilus, Finder, Explorer)

Brecht Van Lommel commented

2023-12-20 13:29:29 +01:00

This is of course subjective and I don't want to nitpicky about this stuff too much. But the elbow is a non-standard UI design, other UI toolkits seem to do fine without it, and I guess that's the objective reasoning.

In some cases it makes sense to deviate from standards, but it comes at a cost, and I don't see the justification here.

This is of course subjective and I don't want to nitpicky about this stuff too much. But the elbow is a non-standard UI design, other UI toolkits seem to do fine without it, and I guess that's the objective reasoning. In some cases it makes sense to deviate from standards, but it comes at a cost, and I don't see the justification here.

Harley Acheson commented

2023-12-20 19:54:07 +01:00

I personally don't need a big change in indentation for it to look a lot better:

diff --git a/source/blender/editors/interface/views/tree_view.cc b/source/blender/editors/interface/views/tree_view.cc
index baefd64d0bb..5327d9aa771 100644
--- a/source/blender/editors/interface/views/tree_view.cc
+++ b/source/blender/editors/interface/views/tree_view.cc
@@ -330,7 +330,7 @@ void AbstractTreeViewItem::add_indent(uiLayout &row) const
   /* Indent items without collapsing icon some more within their parent. Makes it clear that they
    * are actually nested and not just a row at the same level without a chevron. */
   if (!is_collapsible() && parent_) {
-    uiDefBut(block, UI_BTYPE_SEPR, 0, "", 0, 0, 0.2f * UI_UNIT_X, 0, nullptr, 0.0, 0.0, 0, 0, "");
+    uiDefBut(block, UI_BTYPE_SEPR, 0, "", 0, 0, 0.7f * UI_UNIT_X, 0, nullptr, 0.0, 0.0, 0, 0, "");
   }
 
   /* Restore. */

I personally don't need a big change in indentation for it to look a lot better: ![image](/attachments/cf0790c6-5514-47cd-992b-b97dac91f5aa) ``` diff --git a/source/blender/editors/interface/views/tree_view.cc b/source/blender/editors/interface/views/tree_view.cc index baefd64d0bb..5327d9aa771 100644 --- a/source/blender/editors/interface/views/tree_view.cc +++ b/source/blender/editors/interface/views/tree_view.cc @@ -330,7 +330,7 @@ void AbstractTreeViewItem::add_indent(uiLayout &row) const /* Indent items without collapsing icon some more within their parent. Makes it clear that they * are actually nested and not just a row at the same level without a chevron. */ if (!is_collapsible() && parent_) { - uiDefBut(block, UI_BTYPE_SEPR, 0, "", 0, 0, 0.2f * UI_UNIT_X, 0, nullptr, 0.0, 0.0, 0, 0, ""); + uiDefBut(block, UI_BTYPE_SEPR, 0, "", 0, 0, 0.7f * UI_UNIT_X, 0, nullptr, 0.0, 0.0, 0, 0, ""); } /* Restore. */ ```

image.png

38 KiB

👍 1

Sybren A. Stüvel commented

2023-12-21 15:59:37 +01:00

This is of course subjective and I don't want to nitpicky about this stuff too much. But the elbow is a non-standard UI design, other UI toolkits seem to do fine without it, and I guess that's the objective reasoning.

@brecht The elbow was, as Harley also pointed out, more of an attempt at a workaround for the confusion caused by the inconsistent indentation. I chose to only use a horizontal line for the last child (instead of drawing one for each child, which is way more common) in order to keep things visually cleaner, and stay closer to the just-the-vertical-line design currently in main. If we want to avoid the elbow here, that's fine by me, but IMO we should then actually solve the underlying problem that I tried to solve with that elbow.

I personally don't need a big change in indentation for it to look a lot better:

@Harley Although it looks better, that wouldn't solve the issue of the big indentation difference of the labels, depending on whether the node has any children or not.

> This is of course subjective and I don't want to nitpicky about this stuff too much. But the elbow is a non-standard UI design, other UI toolkits seem to do fine without it, and I guess that's the objective reasoning. @brecht The elbow was, as Harley also pointed out, more of an attempt at a workaround for the confusion caused by the inconsistent indentation. I chose to only use a horizontal line for the last child (instead of drawing one for each child, which is way more common) in order to keep things visually cleaner, and stay closer to the just-the-vertical-line design currently in `main`. If we want to avoid the elbow here, that's fine by me, but IMO we should then actually solve the underlying problem that I tried to solve with that elbow. > I personally don't need a big change in indentation for it to look a lot better: @Harley Although it looks better, that wouldn't solve the issue of the big indentation difference of the labels, depending on whether the node has any children or not.

👍 1

Sander de Regt commented

2023-12-29 17:21:30 +01:00

First-time contributor

I see both the pros and the cons for the elbow approach. For some reason the treeview in Geometry nodes is less confusing for me than it is in the Bone Collections. Maybe because the 'sections' are more naturally defined there. Have you considered using the Collections icon inside Bone Collections as well? The 'storage box' icon makes it easier for me to see at a glance where a new collection starts.

Harley Acheson commented

2023-12-30 03:05:33 +01:00

Sorry, but taking a closer look at this area of code, I keep finding problems that make any solution a bit more complicated. For example with current main, using 2D zooming the relationship lines go out of horizontal alignment with the disclosure triangles. And the width of the lines stay the same as well and get too thin:

This impacts your PR in that the "elbows" have lengths that are incorrectly sized with local zooming.

I think the following fixes most of the problems I am seeing:

diff --git a/source/blender/editors/include/UI_tree_view.hh b/source/blender/editors/include/UI_tree_view.hh
index d409f1376c5..e53aa5699a5 100644
--- a/source/blender/editors/include/UI_tree_view.hh
+++ b/source/blender/editors/include/UI_tree_view.hh
@@ -149,9 +149,10 @@ class AbstractTreeView : public AbstractView, public TreeViewItemContainer {
 
   void draw_hierarchy_lines(const ARegion &region) const;
   void draw_hierarchy_lines_recursive(const ARegion &region,
                                       const TreeViewOrItem &parent,
-                                      uint pos) const;
+                                      const uint pos,
+                                      const float aspect) const;
   AbstractTreeViewItem *find_last_visible_descendant(const AbstractTreeViewItem &parent) const;
 };
 
 /** \} */
diff --git a/source/blender/editors/interface/views/tree_view.cc b/source/blender/editors/interface/views/tree_view.cc
index baefd64d0bb..dd653ac83b5 100644
--- a/source/blender/editors/interface/views/tree_view.cc
+++ b/source/blender/editors/interface/views/tree_view.cc
@@ -134,16 +134,17 @@ AbstractTreeViewItem *AbstractTreeView::find_last_visible_descendant(
 }
 
 void AbstractTreeView::draw_hierarchy_lines_recursive(const ARegion &region,
                                                       const TreeViewOrItem &parent,
-                                                      const uint pos) const
+                                                      const uint pos,
+                                                      const float aspect) const
 {
   for (const auto &item : parent.children_) {
     if (!item->is_collapsible() || item->is_collapsed()) {
       continue;
     }
 
-    draw_hierarchy_lines_recursive(region, *item, pos);
+    draw_hierarchy_lines_recursive(region, *item, pos, aspect);
 
     const AbstractTreeViewItem *first_descendant = item->children_.first().get();
     const AbstractTreeViewItem *last_descendant = find_last_visible_descendant(*item);
     if (!first_descendant->view_item_but_ || !last_descendant || !last_descendant->view_item_but_)
@@ -160,41 +161,33 @@ void AbstractTreeView::draw_hierarchy_lines_recursive(const ARegion &region,
     ui_but_to_pixelrect(&first_child_rect, &region, block, &first_child_but);
     rcti last_child_rect;
     ui_but_to_pixelrect(&last_child_rect, &region, block, &last_child_but);
 
-    /* Small vertical padding. */
-    const short line_padding = UI_UNIT_Y / 4.0f;
-    const float x = first_child_rect.xmin + first_descendant->indent_width() -
-                    UI_ICON_SIZE * 0.5f + 2 * UI_SCALE_FAC;
-    immBegin(GPU_PRIM_LINES, 2);
-    immVertex2f(pos, x, first_child_rect.ymax - line_padding);
-    immVertex2f(pos, x, last_child_rect.ymin + line_padding);
+    const float x = first_child_rect.xmin +
+                    ((first_descendant->indent_width() - (6.5f * UI_SCALE_FAC)) / aspect);
+    const int first_child_top = first_child_rect.ymax + (4.0f * UI_SCALE_FAC / aspect);
+    const int last_child_bottom = last_child_rect.ymin + (2.0f * UI_SCALE_FAC / aspect);
+
+    immBegin(GPU_PRIM_LINE_STRIP, 3);
+    immVertex2f(pos, x, first_child_top);
+    immVertex2f(pos, x, last_child_bottom);
+    immVertex2f(pos, x + (UI_UNIT_X * 0.2f / aspect), last_child_bottom);
     immEnd();
   }
 }
 
 void AbstractTreeView::draw_hierarchy_lines(const ARegion &region) const
 {
+  const float aspect = BLI_rctf_size_y(&region.v2d.cur) / (BLI_rcti_size_y(&region.v2d.mask) + 1);
+
   GPUVertFormat *format = immVertexFormat();
   uint pos = GPU_vertformat_attr_add(format, "pos", GPU_COMP_F32, 2, GPU_FETCH_FLOAT);
-  uchar col[4];
-
-  immBindBuiltinProgram(GPU_SHADER_3D_LINE_DASHED_UNIFORM_COLOR);
-
-  float viewport_size[4];
-  GPU_viewport_size_get_f(viewport_size);
-  immUniform2f("viewport_size", viewport_size[2] / UI_SCALE_FAC, viewport_size[3] / UI_SCALE_FAC);
-  immUniform1i("colors_len", 0); /* "simple"  mode */
-  immUniform1f("dash_width", 8.0f);
-  /* >= is 1.0 for un-dashed lines. */
-  immUniform1f("udash_factor", 1.0f);
-  UI_GetThemeColorBlend3ubv(TH_BACK, TH_TEXT, 0.4f, col);
-  col[3] = 255;
-  immUniformColor4ubv(col);
-
-  GPU_line_width(1.0f);
+  immBindBuiltinProgram(GPU_SHADER_3D_UNIFORM_COLOR);
+  immUniformThemeColorAlpha(TH_TEXT, 0.3f);
+
+  GPU_line_width(1.0f / aspect);
   GPU_blend(GPU_BLEND_ALPHA);
-  draw_hierarchy_lines_recursive(region, *this, pos);
+  draw_hierarchy_lines_recursive(region, *this, pos, aspect);
   GPU_blend(GPU_BLEND_NONE);
 
   immUnbindProgram();
 }
@@ -329,9 +322,9 @@ void AbstractTreeViewItem::add_indent(uiLayout &row) const
 
   /* Indent items without collapsing icon some more within their parent. Makes it clear that they
    * are actually nested and not just a row at the same level without a chevron. */
   if (!is_collapsible() && parent_) {
-    uiDefBut(block, UI_BTYPE_SEPR, 0, "", 0, 0, 0.2f * UI_UNIT_X, 0, nullptr, 0.0, 0.0, 0, 0, "");
+    uiDefBut(block, UI_BTYPE_SEPR, 0, "", 0, 0, 0.7f * UI_UNIT_X, 0, nullptr, 0.0, 0.0, 0, 0, "");
   }
 
   /* Restore. */
   UI_block_layout_set_current(block, &row);

In a nutshell, the local zoom "aspect" is calculated from the region. It is used for line width calculation and is passed to the line drawing routine so everything stays aligned when you zoom. Your elbow is made a little smaller and is a bit lower to look less like it is pointing at the last item. The lines drawn with GPU_PRIM_LINE_STRIP rather than GPU_PRIM_LINES to avoid one position.

The current code is using the dashed line shader for some reason, even though it is set to "undashed" and uses a single color. So the above replaces it with GPU_SHADER_3D_UNIFORM_COLOR. The line color is made just a bit less bright. And the children are indented a bit from their parents. The result looks like this:

Sorry, but taking a closer look at this area of code, I keep finding problems that make any solution a bit more complicated. For example with _current_ main, using 2D zooming the relationship lines go out of horizontal alignment with the disclosure triangles. And the width of the lines stay the same as well and get too thin: ![image](/attachments/6378c4d5-d30c-4978-a2e3-d5e693d07e9f) This impacts your PR in that the "elbows" have lengths that are incorrectly sized with local zooming. I think the following fixes most of the problems I am seeing: ``` diff --git a/source/blender/editors/include/UI_tree_view.hh b/source/blender/editors/include/UI_tree_view.hh index d409f1376c5..e53aa5699a5 100644 --- a/source/blender/editors/include/UI_tree_view.hh +++ b/source/blender/editors/include/UI_tree_view.hh @@ -149,9 +149,10 @@ class AbstractTreeView : public AbstractView, public TreeViewItemContainer { void draw_hierarchy_lines(const ARegion &region) const; void draw_hierarchy_lines_recursive(const ARegion &region, const TreeViewOrItem &parent, - uint pos) const; + const uint pos, + const float aspect) const; AbstractTreeViewItem *find_last_visible_descendant(const AbstractTreeViewItem &parent) const; }; /** \} */ diff --git a/source/blender/editors/interface/views/tree_view.cc b/source/blender/editors/interface/views/tree_view.cc index baefd64d0bb..dd653ac83b5 100644 --- a/source/blender/editors/interface/views/tree_view.cc +++ b/source/blender/editors/interface/views/tree_view.cc @@ -134,16 +134,17 @@ AbstractTreeViewItem *AbstractTreeView::find_last_visible_descendant( } void AbstractTreeView::draw_hierarchy_lines_recursive(const ARegion &region, const TreeViewOrItem &parent, - const uint pos) const + const uint pos, + const float aspect) const { for (const auto &item : parent.children_) { if (!item->is_collapsible() || item->is_collapsed()) { continue; } - draw_hierarchy_lines_recursive(region, *item, pos); + draw_hierarchy_lines_recursive(region, *item, pos, aspect); const AbstractTreeViewItem *first_descendant = item->children_.first().get(); const AbstractTreeViewItem *last_descendant = find_last_visible_descendant(*item); if (!first_descendant->view_item_but_ || !last_descendant || !last_descendant->view_item_but_) @@ -160,41 +161,33 @@ void AbstractTreeView::draw_hierarchy_lines_recursive(const ARegion &region, ui_but_to_pixelrect(&first_child_rect, &region, block, &first_child_but); rcti last_child_rect; ui_but_to_pixelrect(&last_child_rect, &region, block, &last_child_but); - /* Small vertical padding. */ - const short line_padding = UI_UNIT_Y / 4.0f; - const float x = first_child_rect.xmin + first_descendant->indent_width() - - UI_ICON_SIZE * 0.5f + 2 * UI_SCALE_FAC; - immBegin(GPU_PRIM_LINES, 2); - immVertex2f(pos, x, first_child_rect.ymax - line_padding); - immVertex2f(pos, x, last_child_rect.ymin + line_padding); + const float x = first_child_rect.xmin + + ((first_descendant->indent_width() - (6.5f * UI_SCALE_FAC)) / aspect); + const int first_child_top = first_child_rect.ymax + (4.0f * UI_SCALE_FAC / aspect); + const int last_child_bottom = last_child_rect.ymin + (2.0f * UI_SCALE_FAC / aspect); + + immBegin(GPU_PRIM_LINE_STRIP, 3); + immVertex2f(pos, x, first_child_top); + immVertex2f(pos, x, last_child_bottom); + immVertex2f(pos, x + (UI_UNIT_X * 0.2f / aspect), last_child_bottom); immEnd(); } } void AbstractTreeView::draw_hierarchy_lines(const ARegion &region) const { + const float aspect = BLI_rctf_size_y(&region.v2d.cur) / (BLI_rcti_size_y(&region.v2d.mask) + 1); + GPUVertFormat *format = immVertexFormat(); uint pos = GPU_vertformat_attr_add(format, "pos", GPU_COMP_F32, 2, GPU_FETCH_FLOAT); - uchar col[4]; - - immBindBuiltinProgram(GPU_SHADER_3D_LINE_DASHED_UNIFORM_COLOR); - - float viewport_size[4]; - GPU_viewport_size_get_f(viewport_size); - immUniform2f("viewport_size", viewport_size[2] / UI_SCALE_FAC, viewport_size[3] / UI_SCALE_FAC); - immUniform1i("colors_len", 0); /* "simple" mode */ - immUniform1f("dash_width", 8.0f); - /* >= is 1.0 for un-dashed lines. */ - immUniform1f("udash_factor", 1.0f); - UI_GetThemeColorBlend3ubv(TH_BACK, TH_TEXT, 0.4f, col); - col[3] = 255; - immUniformColor4ubv(col); - - GPU_line_width(1.0f); + immBindBuiltinProgram(GPU_SHADER_3D_UNIFORM_COLOR); + immUniformThemeColorAlpha(TH_TEXT, 0.3f); + + GPU_line_width(1.0f / aspect); GPU_blend(GPU_BLEND_ALPHA); - draw_hierarchy_lines_recursive(region, *this, pos); + draw_hierarchy_lines_recursive(region, *this, pos, aspect); GPU_blend(GPU_BLEND_NONE); immUnbindProgram(); } @@ -329,9 +322,9 @@ void AbstractTreeViewItem::add_indent(uiLayout &row) const /* Indent items without collapsing icon some more within their parent. Makes it clear that they * are actually nested and not just a row at the same level without a chevron. */ if (!is_collapsible() && parent_) { - uiDefBut(block, UI_BTYPE_SEPR, 0, "", 0, 0, 0.2f * UI_UNIT_X, 0, nullptr, 0.0, 0.0, 0, 0, ""); + uiDefBut(block, UI_BTYPE_SEPR, 0, "", 0, 0, 0.7f * UI_UNIT_X, 0, nullptr, 0.0, 0.0, 0, 0, ""); } /* Restore. */ UI_block_layout_set_current(block, &row); ``` In a nutshell, the local zoom "aspect" is calculated from the region. It is used for line width calculation and is passed to the line drawing routine so everything stays aligned when you zoom. Your elbow is made a little smaller and is a bit lower to look less like it is pointing at the last item. The lines drawn with GPU_PRIM_LINE_STRIP rather than GPU_PRIM_LINES to avoid one position. The current code is using the dashed line shader for some reason, even though it is set to "undashed" and uses a single color. So the above replaces it with GPU_SHADER_3D_UNIFORM_COLOR. The line color is made just a bit less bright. And the children are indented a bit from their parents. The result looks like this: ![image](/attachments/e22a6284-06c1-4b82-b06d-0e678f850516)

image.png

14 KiB

image.png

62 KiB

Sybren A. Stüvel commented

2024-01-02 12:12:04 +01:00

Thanks @Harley that looks a lot better indeed.

There is still an issue with root nodes without children and without icon. They crawl up too close to the left edge.

Thanks @Harley that looks a lot better indeed. There is still an issue with root nodes without children and without icon. They crawl up too close to the left edge. ![image](/attachments/d8660e0c-ead8-4033-ab46-1a62eaa5704d)

image.png

42 KiB

Harley Acheson commented

2024-01-09 16:58:11 +01:00

@dr.sybren - There is still an issue with root nodes without children and without icon. They crawl up too close to the left edge.

It isn't clear to me where that childless root element should go. I tried it aligned to the text of the items with children and it looked weird there. Looked a bit better aligned nicer to the downward arrow, but still a bit awkward. Is there a position that looks best for you?

I was playing with this in the Spreadsheet editor, but it might be different from that in your capture. Is there an easy way for me to view that "Bone Collections" list?

> @dr.sybren - There is still an issue with root nodes without children and without icon. They crawl up too close to the left edge. It isn't clear to me where that childless root element should go. I tried it aligned to the text of the items with children and it looked weird there. Looked a bit better aligned nicer to the downward arrow, but still a bit awkward. Is there a position that looks best for you? I was playing with this in the Spreadsheet editor, but it might be different from that in your capture. Is there an easy way for me to view that "Bone Collections" list?

Harley Acheson commented

2024-01-09 23:22:33 +01:00

@dr.sybren

The attached is everything from the above but also moves over the childless root item:

@dr.sybren The attached is everything from the above but also moves over the childless root item: ![image](/attachments/d7057384-a129-4d98-9ae2-74a205d8b7ee)

treestuff2.diff

7.1 KiB

image.png

15 KiB

Sybren A. Stüvel force-pushed gui/tree-view-tweaks from dd4263d640 to c231d7cb35

2024-01-16 11:18:55 +01:00

Compare

Sybren A. Stüvel commented

2024-01-16 11:19:25 +01:00

Thanks @Harley ! I've updated the PR with your diff, and updated the screenshots in the description.

Nika Kutsniashvili commented

2024-01-16 11:33:10 +01:00

To further improve this I agree with Demeter that indentation should use same logic as Outliner, meaning items should always have space on the left that may or may not be occupied by collapse icon. Because while this looks good on given two examples in the screenshots, on different "setups" it looks weird and we need to think about every possible scenario

This is what it will look like with collection with children collapsed (ignore the wrong-facing icon).

Its unclear why two items that are in the same line of hierarchy, they're siblings, have different indentation?

This is how Outliner does it, it always leaves space for hierarchy, whether object has it or not

This looks much cleaner and easier to reach in my opinion. Current indentation logic is very hard to read in Asset Browser, because names aren't lined up and you need to keep moving your eyes left and right to read names if you're reading in order

To demonstrate better way this is how I think it should be drawn:

Bone Collections	Asset Browser	Bone Collections unfolded

To further improve this I agree with Demeter that indentation should use same logic as Outliner, meaning items should always have space on the left that may or may not be occupied by collapse icon. Because while this looks good on given two examples in the screenshots, on different "setups" it looks weird and we need to think about every possible scenario ![image](/attachments/34700461-3d27-4222-9551-1a26e1847ba0) This is what it will look like with collection with children collapsed (ignore the wrong-facing icon). Its unclear why two items that are in the same line of hierarchy, they're siblings, have different indentation? This is how Outliner does it, it always leaves space for hierarchy, whether object has it or not ![image](/attachments/5c1bca19-6330-4d9c-9949-1b55426c4361) This looks much cleaner and easier to reach in my opinion. Current indentation logic is very hard to read in Asset Browser, because names aren't lined up and you need to keep moving your eyes left and right to read names if you're reading in order ![image](/attachments/ec1ee71f-ac56-43d2-af8f-e2ad4e64c9a5) To demonstrate better way this is how I think it should be drawn: | Bone Collections | Asset Browser | Bone Collections unfolded | | -------- | -------- | -------- | | ![image](/attachments/b53a730b-ec8d-4b5a-ba6c-338ab2b6698c) | ![New Project.png](/attachments/219663f7-3ae2-41f1-bc35-7a62fb99764e) | ![image.png](/attachments/c11e155c-2709-4e6b-926e-d3cba654530c) |

image.png

55 KiB

image.png

6.1 KiB

image.png

3.8 KiB

image.png

5.9 KiB

image.png

6.1 KiB

image.png

5.9 KiB

image.png

35 KiB

New Project.png

3.3 KiB

image.png

5.9 KiB

Sybren A. Stüvel commented

2024-01-16 15:55:38 +01:00

To further improve this I agree with Demeter that indentation should use same logic as Outliner

This would be quite a different pull request, though. My goal was to have some simpler improvements that take away the immediate pain points.

This is what it will look like with collection with children collapsed (ignore the wrong-facing icon).

Its unclear why two items that are in the same line of hierarchy, they're siblings, have different indentation?

I agree that this is something to improve. However, I'd be quite happy if this PR could be accepted & landed, and then later someone could look into improving things even further. I'm not a member of the UI module, though, so I'll leave that decision to @Harley and @JulianEisel .

> To further improve this I agree with Demeter that indentation should use same logic as Outliner This would be quite a different pull request, though. My goal was to have some simpler improvements that take away the immediate pain points. > This is what it will look like with collection with children collapsed (ignore the wrong-facing icon). > > Its unclear why two items that are in the same line of hierarchy, they're siblings, have different indentation? I agree that this is something to improve. However, I'd be quite happy if this PR could be accepted & landed, and then later someone could look into improving things even further. I'm not a member of the UI module, though, so I'll leave that decision to @Harley and @JulianEisel .

Harley Acheson commented

2024-01-17 19:56:31 +01:00

@JulianEisel @pablovazquez

Do you guys mind weighing in here? I think they are really hoping for a change in this release and this might have risk of stalling.

I am personally neutral on the elbow itself and don't mind it there or not. I don't think it causes harm and I can see how it might help some users.

But I do quite like the changes to indentation in how it helps group children better.

One outstanding issue is the indentation of childless root elements. I think most people are preferring that they indent so that their names are aligned with items that do have children. Seems fine to me and we could do this all at once.

@JulianEisel @pablovazquez Do you guys mind weighing in here? I think they are really hoping for a change in this release and this might have risk of stalling. I am _personally_ neutral on the elbow itself and don't mind it there or not. I don't think it causes harm and I can see how it might help some users. But I do quite like the changes to indentation in how it helps group children better. One outstanding issue is the indentation of childless root elements. I think most people are preferring that they indent so that their names are aligned with items that do have children. Seems fine to me and we could do this all at once.

Sybren A. Stüvel commented

2024-01-18 12:49:16 +01:00

I think they are really hoping for a change in this release

Yup, we are.

One outstanding issue is the indentation of childless root elements. I think most people are preferring that they indent so that their names are aligned with items that do have children. Seems fine to me and we could do this all at once.

Just a note: this is for all childless elements, not just roots.

> I think they are really hoping for a change in this release Yup, we are. > One outstanding issue is the indentation of childless root elements. I think most people are preferring that they indent so that their names are aligned with items that do have children. Seems fine to me and we could do this all at once. Just a note: this is for all childless elements, not just roots.

Sybren A. Stüvel referenced this pull request

2024-01-22 11:44:59 +01:00

Nested Bone Collections 4.1 Notes #116795

Julian Eisel commented

2024-01-22 18:26:41 +01:00

Could the fixes for zooming be split off? This should be fixed either way, independent of this. It's hard to tell which parts fix the zooming and which parts implement the proposed design change.

The "elbow" was meant to mitigate visual issues with the hierarchy, because we couldn't agree on the best solution for the indentation. It would be good to see a proposal for the indentation as well, maybe we can address it that way still.
@jenkm didn't you have a proposal for this once?

Could the fixes for zooming be split off? This should be fixed either way, independent of this. It's hard to tell which parts fix the zooming and which parts implement the proposed design change. --- The "elbow" was meant to mitigate visual issues with the hierarchy, because we couldn't agree on the best solution for the indentation. It would be good to see a proposal for the indentation as well, maybe we can address it that way still. @jenkm didn't you have a proposal for this once?

Harley Acheson commented

2024-01-22 20:30:30 +01:00

@JulianEisel - Could the fixes for zooming be split off?

I just did so with #117420. If we approve that then I can then update this PR to remove conflicts.

> @JulianEisel - Could the fixes for zooming be split off? I just did so with #117420. If we approve that then I can then update this PR to remove conflicts.

❤️ 1

Yevgeny Makarov commented

2024-01-23 14:31:01 +01:00

You're using a very, very large collapse-icon.

The main element is the label. First, just place the labels with indents, this should already work and show the hierarchy.

The collapse chevron is a secondary element, its visual impact should not exceed that of the label. Adding chevrons should not affect the readability of the tree and hierarchy, it is an additional element. Adding a chevron should not change the label's indentation.

You should see the label first, not the bold icon.

If you use the icon+label, the chevron can be larger, due to the icon, which is relatively large. The Outliner is quite balanced in this regard.

Also, in addition to the size, look at the color of the chevron and the label, the label should be brighter for more impact.

Chevron does not mean another level of hierarchy or a new group, the chevron may or may not be there, you should clearly see the indentation and the chevron should not interfere with that.

The chevron can take up less space in width, the vertical hierarchy lines will be closer to each other, and the indent size will be reduced. I think you can do it in half the width.

You're using a very, very large collapse-icon. The main element is the label. First, just place the labels with indents, this should already work and show the hierarchy. The collapse chevron is a secondary element, its visual impact should not exceed that of the label. Adding chevrons should not affect the readability of the tree and hierarchy, it is an additional element. Adding a chevron should not change the label's indentation. You should see the label first, not the bold icon. If you use the icon+label, the chevron can be larger, due to the icon, which is relatively large. The Outliner is quite balanced in this regard. Also, in addition to the size, look at the color of the chevron and the label, the label should be brighter for more impact. Chevron does not mean another level of hierarchy or a new group, the chevron may or may not be there, you should clearly see the indentation and the chevron should not interfere with that. The chevron can take up less space in width, the vertical hierarchy lines will be closer to each other, and the indent size will be reduced. I think you can do it in half the width.

Sybren A. Stüvel commented

2024-01-23 17:09:56 +01:00

With smaller chevrons, no more elbow, and consistent label indentation, the bone collection tree could look something like this:

With smaller chevrons, no more elbow, and consistent label indentation, the bone collection tree could look something like this: ![image](/attachments/1d1150b2-ea7b-4f81-a873-f0a0f842b032)

image.png

36 KiB

👍 5 ❤️ 1

Harley Acheson added 1 commit 2024-01-23 20:59:54 +01:00

354119ce4e Merge branch 'main' of projects.blender.org:blender/blender into pr116200

Harley Acheson added 1 commit 2024-01-23 21:03:58 +01:00

7fcc33a7d8 formatting changes

Harley Acheson commented

2024-01-23 21:10:58 +01:00

@dr.sybren - I updated this PR to remove conflicts with new changes to main that I pushed that sizes and positions the hierarchy lines correctly as the region is zoomed - #117420. So it should now only contain the changes related to line drawing and indentation. If you prefer that I should not have altered your PR, just let me know.

Yevgeny Makarov commented

2024-01-24 13:13:36 +01:00

Before	After

hierarchy-comp.gif

31 KiB

hierarchy-after.png

30 KiB

hierarchy-before.png

30 KiB

👍 2 ❤️ 2

Sybren A. Stüvel commented

2024-01-25 10:12:26 +01:00

If you prefer that I should not have altered your PR, just let me know.

Nope, this is absolutely fine, and I appreciate the help!

@jenkm I've edited your comment to put the images side by side. If you don't like me doing that kind of stuff, please let me know.

As there is no description of your changes, this is what I think you did:

Less horizontal whitespace, compressing the indentation.
Darker chevron and lines.
Rounded elbows.

Let me know if I missed anything.

I like all of those changes, so thanks! For me it becomes clearer that the elbows are actually a good idea. The currently-in-main straight lines always feel like "pointing down" to elements that may not be directly related (f.e. "Arm.L (Tweak)" has a line pointing to Arm.R"). The little elbow at the bottom say more "this is grouped together", and demarking a clear end of that group. With the rounded elbows of @jenkm this is even stronger.

> If you prefer that I should not have altered your PR, just let me know. Nope, this is absolutely fine, and I appreciate the help! @jenkm I've edited your comment to put the images side by side. If you don't like me doing that kind of stuff, please let me know. As there is no description of your changes, this is what I think you did: - Less horizontal whitespace, compressing the indentation. - Darker chevron and lines. - Rounded elbows. Let me know if I missed anything. I like all of those changes, so thanks! For me it becomes clearer that the elbows are actually a good idea. The currently-in-main straight lines always feel like "pointing down" to elements that may not be directly related (f.e. "Arm.L (Tweak)" has a line pointing to Arm.R"). The little elbow at the bottom say more "this is grouped together", and demarking a clear end of that group. With the rounded elbows of @jenkm this is even stronger.

Harley Acheson added this to the User Interface project 2024-01-29 00:09:22 +01:00

Julian Eisel commented

2024-01-29 21:09:17 +01:00

Before After

This looks quite good to me, but I guess we'd need to see it in practice to look at the different possible cases that can cause confusion. @Harley do you think you could work on this? Seems like the animation module really wants to get this in for the 4.1 release.

> > | Before | After | > | -- | -- | > | ![hierarchy-before.png](/attachments/0e571012-7aa9-43b1-9430-2a424915ec4f) | ![hierarchy-after.png](/attachments/987c1ea7-890d-4fd6-bccd-f504c956102e) | This looks quite good to me, but I guess we'd need to see it in practice to look at the different possible cases that can cause confusion. @Harley do you think you could work on this? Seems like the animation module really wants to get this in for the 4.1 release.

Harley Acheson commented

2024-01-29 22:02:05 +01:00

@JulianEisel - This looks quite good to me...do you think you could work on this?

Yes, will just post my own PR for this, rather than updating Sybren's, using this last capture as design.

> @JulianEisel - This looks quite good to me...do you think you could work on this? Yes, will just post my own PR for this, rather than updating Sybren's, using this last capture as design.

~~Harley Acheson referenced this pull request 2024-01-30 00:52:58 +01:00~~

UI: TreeView Changes #117654

Sybren A. Stüvel referenced this pull request

2024-01-30 15:28:10 +01:00

UI: TreeView Changes #117654

Brecht Van Lommel commented

2024-02-13 18:52:32 +01:00

This was superseded by #117654.

Brecht Van Lommel closed this pull request

2024-02-13 18:52:35 +01:00