Page MenuHome

UI: Improve node editor breadcrumbs display
ClosedPublic

Authored by Hans Goudey (HooglyBoogly) on Feb 13 2021, 11:32 PM.
Tokens
"Love" token, awarded by HEYPictures."Love" token, awarded by luischerubini."Like" token, awarded by Draise."Love" token, awarded by Roger_Menzi."Like" token, awarded by jimman2003."Love" token, awarded by asmitty."Love" token, awarded by damian."Love" token, awarded by amalbubble."Love" token, awarded by CreatorSiSo."Love" token, awarded by looch."Love" token, awarded by lone_noel."Love" token, awarded by duarteframos."Love" token, awarded by TimBrown."Like" token, awarded by Fracture128.

Details

Summary

This patch upgrades node editor breadcrumbs to have slightly more visual
weight, to including the base path of object/modifier/world, etc, have more
visually pleasing spacing, and icons.

In the code, a rather generic "context path" is added to interface code. The idea
is that this could be used to draw other breadcrumbs in areas like the property
editor or the spreadsheet, and features could be added to all of those areas at
the same time.

Thanks to @Fabian Schempp (fabian_schempp) for the original patch!


In a previous version of this patch, the node breadcrumbs were interactive.
In order to simplify the review process, that change is not included for now.
That could be addressed separately in the future, along with menus or
tooltips for elements of the path.

Diff Detail

Repository
rB Blender

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Fabian Schempp (fabian_schempp) marked 7 inline comments as done.Mar 2 2021, 8:39 PM
  • Changes based on review by Hans Goudey.
  • Fixed a minor layout issue, with the space after the chevron being a bit to small compared to the space before.
Fabian Schempp (fabian_schempp) marked an inline comment as done.Mar 2 2021, 8:48 PM

I'm quite happy with this, nice job Fabian!

I asked @Pablo Vazquez (pablovazquez) to check on this before it's committed.

source/blender/editors/space_node/node_intern.h
350

node_context.c -> node_context.cc

source/blender/editors/space_node/node_relationships.c
2148 ↗(On Diff #34669)

Missing newline at the end of the file.

This revision is now accepted and ready to land.Mar 2 2021, 11:49 PM

Another great addition! Thanks for working on it @Fabian Schempp (fabian_schempp) !

Dalai Felinto (dfelinto) requested changes to this revision.Mar 3 2021, 10:17 AM

Please just hold on before committing this. To make it clickable as it is, it means that we are effectively adding overlay buttons in those editors. As a general rule for the Blender UI we do not do overlay buttons (or avoid as much as possible). I will check with the UI team how to proceed.

This revision now requires changes to proceed.Mar 3 2021, 10:17 AM

I want to share some notes to explain my concerns a bit. They all assume we keep the breadcrumbs clickable. I want to share them to build some shared understanding and language to get us on the same page. The design can go different ways, but should do so deliberately, and this should be explicit in the patch description.

  • As it is, there is no way for users to know that those labels are clickable.

If you see the overlay region we have in the 3d viewport, the decoration for the menus has a solid background:

  • Blender uses top-down, left-right as the navigation rules. So navigating deeper and deeper the nodegroups should (probably) be on top, not on the botom.
  • The tooltip for selecting the node group is too technical although correct:

"Navigate up n levels of the active Node Tree Context Path, where n is set by steps property"

Keep in mind that users would be clicking on it, not setting the n directly. The tooltips should be thought from the user point of view.

I want to share some notes to explain my concerns a bit. They all assume we keep the breadcrumbs clickable. I want to share them to build some shared understanding and language to get us on the same page. The design can go different ways, but should do so deliberately, and this should be explicit in the patch description.

  • As it is, there is no way for users to know that those labels are clickable.

If you see the overlay region we have in the 3d viewport, the decoration for the menus has a solid background:

This is a complicated point. I tried to use the embossed buttons, which looks good, but also is not correct since its a hirarchical navigation, where the whole navigation should be recognizable as compound element not a list of Button. Also the breadcrumb is used in at least 5 editors (Geometry Nodes, Compositor, Shader Nodes, Spreadsheet, Properties), so it should be reusable in all this places in a consistent way. Which is also not simple, since the other breadcrumbs are not clickable. Im excited to see what we will decide here.

  • Blender uses top-down, left-right as the navigation rules. So navigating deeper and deeper the nodegroups should (probably) be on top, not on the bottom.

As Hans noted there are some minor problems with it on the top but my general tendendy is also to put it on the top.

  • The tooltip for selecting the node group is too technical although correct:

"Navigate up n levels of the active Node Tree Context Path, where n is set by steps property"

Keep in mind that users would be clicking on it, not setting the n directly. The tooltips should be thought from the user point of view.

Fully agree with that, wasn't obvious to me that this is the tool tip that is visible to the user.

This revision now requires review to proceed.Mar 3 2021, 2:20 PM
  • Made breadcrumb more generic by moving path generation to interface_context_path.cc and breadcrumb drawing to a UI template.
  • Reverted interactive breadcrumb to a plain visual breadcrumb for now, as this raised concernes.
Fabian Schempp (fabian_schempp) edited the summary of this revision. (Show Details)
  • Reverted unneeded change.
  • Reverted unneeded changes.

Reverted unneeded changes.

  • Added missing newline to the end of node_relationship.c
Fabian Schempp (fabian_schempp) marked 2 inline comments as done.Mar 11 2021, 8:02 PM
  • Fixed error with auto reformatting.

I'm updating this patch to latest master, and doing some fairly significant changes, so I may as well commandeer this.

  • Use blender::ui:: namespace
  • Use references instead of pointers
  • Forward declare non-dereferenced types instead of including DNA
  • Remove unnecessary items from context item
  • Renamve structs and variables
  • Move drawing to C++
  • Remove unnecessary changes
Hans Goudey (HooglyBoogly) retitled this revision from Nodes: Breadcrumbs for node editor to UI: Improve node editor breadcrumbs display.Oct 8 2021, 8:58 PM
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Oct 8 2021, 8:58 PM
This revision now requires review to proceed.Oct 8 2021, 8:59 PM
  • Merge branch 'master' into node-editor-breadcrumbs
  • Move to UI_interface.hh

Currently the breadcrumbs have the same 0.902 alpha that the rest of the text in
the node editor uses. I'm not quite sure why the text uses alpha, maybe that could
be changed in the theme changes.

Ideally we could allow using a separate theme color for this, but I'm not sure
how that would be done while also using the layout system. Maybe @Julian Eisel (Severin) has
an idea?

Currently the breadcrumbs have the same 0.902 alpha that the rest of the text in
the node editor uses.

After testing the patch it doesn't bother me as much. Even with that transparency (which doesn't come from the theme since TH_TEXT is only RGB not RGBA).

I think this should use the Node Group icon when going inside the node group. Right now it is using the Material/View Layers icon:

For Geometry Nodes this is not noticeable since we are always inside a node group.

I think this should use the Node Group icon when going inside the node group.

Agree, I missed this when testing.

It should use NODETREE icon, to look something like:

One day we'll hopefully have a dedicated icon for geometry nodes but for the time being this change will make things more clear.

After discussing this with Ton we agreed that the breadcrumbs should be on top-left, as it is related to the current datablock which is also on top. It matches the 3D Viewport context info as well.

Breadcrumbs display can be later added to the new Overlay popover in D12886.

I think this should use the Node Group icon when going inside the node group.

I find this a bit weird, since nested shader node groups are still shader node groups, likewise for the compositor. This will be especially true for geometry nodes when it gets a proper icon, since the root node group used for the modifier and other node groups are meant to be interchangeable.
But in the interest of moving this patch forward I still made the change.

  • Merge branch 'master' into node-editor-breadcrumbs
  • Move to top left of region
  • Use the generic node group icon for all nested groups

Only one last thing we just noticed with Dalai, is that for the Compositor, the Compositing Nodetree item in the breadcumb doesn't really add anything to the user. Compositing nodes belong to the Scene, and are not part of a node group that the user can later append. If we were to follow that logic, Materials would have "Shader Nodetree" in the breadcrumbs. So I think we can do without.

Other than that, the rest looks great! Even the top-left position didn't bother me as much as I thought it would. +1 !

I would actually expect context_path_for_space_node() to be in the node space code, e.g. in a node_context_path.cc. It's weird to have that logic in a general UI file. Think you can just turn context_path_add_generic() into a public context_path_add()?

Yes, I like that better! I figured there might have been a reason why it was like that in the patch when I took it over.

  • Move most of the implementation to the node editor
  • Offset the breadcrumbs when the toolbar is expanded

T82812: Including node group attribute into NodeTreePath comes to my mind, any chance this can be tackled along?

Since this is mostly about drawing the existing data better, I'm not sure it would fit in this patch. I find the issue a little hard to understand also.

Julian Eisel (Severin) added inline comments.
source/blender/editors/interface/interface_context_path.cc
76

If they are all supposed to be disabled, just disable the parent row?

This revision is now accepted and ready to land.Oct 26 2021, 10:38 AM