Page MenuHome
Differential D10931

Spreadsheet: Breadcrumbs and node pinning.
ClosedPublic
Actions

Authored by Jacques Lucke (JacquesLucke) on Apr 9 2021, 5:50 PM.

Details

Reviewers
Hans Goudey (HooglyBoogly)
Dalai Felinto (dfelinto)
Simon Thommes (simonthommes)
Commits
rB3810bcc16047: Spreadsheet: breadcrumbs and node pinning
Summary

This is not really ready for testing yet, there are still a couple of rough edges. Pinning specific nodes works already though (although only in one spreadsheet at a time).

I don't really think that we should display the breadcrumbs like this. It only works when the editor is very wide, and few nested node groups are used. Maybe a shortened version of it can be shown in the header, or it is displayed vertically in a dropdown. That is easy to change later on.

Diff Detail

Repository
rB Blender
Branch
spreadsheet-breadcrumbs (branched from master)
Build Status
Buildable 13973
Build 13973: arc lint + arc unit

Event Timeline

Jacques Lucke (JacquesLucke) requested review of this revision.Apr 9 2021, 5:50 PM
Jacques Lucke (JacquesLucke) created this revision.
Harbormaster completed remote builds in B13973: Diff 36029.Apr 9 2021, 5:50 PM
Jacques Lucke (JacquesLucke) edited the summary of this revision. (Show Details)Apr 9 2021, 5:56 PM
Jacques Lucke (JacquesLucke) updated this revision to Diff 36077.Apr 12 2021, 11:57 AM
  • support multiple pinned spreadsheets
  • support disabling preview
  • improve behavior when setting object evaluation state
  • fix delete bug
  • update pinned breadcrumbs if outdated
  • move breadcrumbs to popover
Harbormaster completed remote builds in B14005: Diff 36077.Apr 12 2021, 11:57 AM
Jacques Lucke (JacquesLucke) updated this revision to Diff 36078.Apr 12 2021, 12:12 PM
  • don't unpin automatically
Harbormaster completed remote builds in B14006: Diff 36078.Apr 12 2021, 12:12 PM
Jacques Lucke (JacquesLucke) updated this revision to Diff 36079.Apr 12 2021, 1:22 PM
  • unpin only, if no new context is found
  • avoid tagging header for redraw
  • indicate that there are more breadcrumbs with a * (just for testing)
  • remove layout separator
  • hash breadcrumbs
  • try to guess current context after unpinning
Harbormaster completed remote builds in B14007: Diff 36079.Apr 12 2021, 1:22 PM
Jacques Lucke (JacquesLucke) updated this revision to Diff 36081.EditedApr 12 2021, 3:28 PM
  • initial collapsable breadcrumbs

I'm not quite sure what "..." I should use. Just using a label with text looked super ugly and misaligned. We have an icon with three vertical dots, but not horizontal..

Harbormaster completed remote builds in B14008: Diff 36081.Apr 12 2021, 3:28 PM
Jacques Lucke (JacquesLucke) retitled this revision from Spreadsheet: Breadcrumbs and node pinning (WIP). to Spreadsheet: Breadcrumbs and node pinning..Apr 12 2021, 3:28 PM
Jacques Lucke (JacquesLucke) updated this revision to Diff 36082.Apr 12 2021, 3:50 PM
  • cleanup
Harbormaster completed remote builds in B14009: Diff 36082.Apr 12 2021, 3:50 PM
Jacques Lucke (JacquesLucke) added reviewers: Hans Goudey (HooglyBoogly), Dalai Felinto (dfelinto), Simon Thommes (simonthommes).Apr 12 2021, 3:54 PM

Was just looking at T87314. If we want, we can still consolidate the Node and Final methods, probably. It can be part of this patch, but it can also be done separately afterwards.

Jacques Lucke (JacquesLucke) updated this revision to Diff 36117.Apr 13 2021, 10:18 AM

Spreadsheet: Breadcrumbs with combined node/final evaluation modes.

Harbormaster completed remote builds in B14028: Diff 36117.Apr 13 2021, 10:18 AM
Jacques Lucke (JacquesLucke) updated this revision to Diff 36119.Apr 13 2021, 11:05 AM
  • fix
Harbormaster completed remote builds in B14030: Diff 36119.Apr 13 2021, 11:05 AM
Simon Thommes (simonthommes) added a comment.Apr 13 2021, 12:08 PM

I think the general behaviour as it is now is nice!
Small request though:
I think after changing the active modifier the context should also update to the state of that new active modifier.

Jacques Lucke (JacquesLucke) updated this revision to Diff 36130.Apr 13 2021, 4:15 PM
  • cleanup
Harbormaster completed remote builds in B14040: Diff 36130.Apr 13 2021, 4:15 PM
Jacques Lucke (JacquesLucke) added a comment.Apr 13 2021, 4:18 PM

I think after changing the active modifier the context should also update to the state of that new active modifier.

That is doable, but I think I'd rather do it as a bug fix instead, just to keep this patch a little bit simpler.

Simon Thommes (simonthommes) accepted this revision.Apr 13 2021, 5:16 PM
In D10931#279833, @Jacques Lucke (JacquesLucke) wrote:

I think after changing the active modifier the context should also update to the state of that new active modifier.

That is doable, but I think I'd rather do it as a bug fix instead, just to keep this patch a little bit simpler.

Sure, then I'd consider this patch ready from my perspective as a user.
With this we can also close T87314: Decide on design for switching between geometry contexts in regards to the node inspection, I would say.

This revision is now accepted and ready to land.Apr 13 2021, 5:16 PM
Hans Goudey (HooglyBoogly) requested changes to this revision.EditedApr 14 2021, 6:26 AM

I have to say, this works really wonderfully. It's such a pleasure to use now, and while there might be some further tweaking possible, especially in regards to the button in the node header itself, it feels quite close to optimal.

What you did with the context path in the header is great too. It's still a bit prominent IMO, but I can't think of any better way to make it shorter, and that method of clicking on it to make it shorter is clever.

I think breadcrumbs and SpreadsheetBreadcrumb might not be the best names-- "context path" might be clearer, since this is really used to define what the spreadsheet editor's context is.

"Show this node's geometry output in the spreadsheet in Node mode" button label needs updating to remove the reference to "Node mode".

Many of the inline comments are just requesting more comments. I think that's one thing that's missing in general actually, a brief higher level description about which direction updates are tagged in, the flow of information, etc.
That's important since the whole "find all spreadsheet editors and tag them for updates" thing isn't very obvious for someone without all the context we have at this moment. I'm not sure where it would be best to put that though, and I guess it could be a separate commit.

release/scripts/startup/bl_ui/space_spreadsheet.py
71

Unused variable

81

Unused variable

103–105

Why an operator button? This seems simpler. Plus you get a nicer tooltip (and the unused variables above won't be unused)

def draw_breadcrumb_path_icon(self, layout, space, icon='RIGHTARROW_THIN'):
    layout.prop(space, "display_breadcrumbs_collapsed", icon_only=True, emboss=False, icon=icon)
source/blender/blenkernel/intern/screen.c
1365

I would think you would only want to write the breadcrumbs if there is a pinned context, right? Otherwise it's just runtime data.

source/blender/editors/include/ED_spreadsheet.h
33

This isn't used right now. Any reason to keep it around?

source/blender/editors/space_spreadsheet/space_spreadsheet.cc
164

The first question a reader has is "what does outdated mean?" From reading the code it looks like it might mean that the object was deleted? That makes sense, but then I wonder.. what about deleted nodes? Modifiers? The function could have a comment at the top to make its goals clearer.

source/blender/editors/space_spreadsheet/spreadsheet_breadcrumb.cc
264

This is one of those ambiguous cases where one has to read through it to see what's actually being tagged for an update.
It's definitely obvious in most cases, but I do think some simple comment will save someone time in the future. Hopefully more time than it takes to write and address this comment...

A suggestion:

/**
 * Tag any data relevant to the spreadsheet's context for recalculation in order to collect information to display in the editor, which may be cached during evaluation.
 */
source/blender/makesdna/DNA_object_types.h
173 ↗(On Diff #36130)

Well, technically integer keys are "owned" by the map, since they're stored in it, right? It's just trivial ownership?

The point comes across though, no real need to change anything I don't think.

source/blender/makesdna/DNA_space_types.h
1904–1908

Messed up formatting here, and we need another comment.

source/blender/makesrna/intern/rna_nodetree.c
10828

I'll suggest being a bit more clear: "The data passing though the node is previewed in another editor". Something like that at least.

source/blender/makesrna/intern/rna_space.c
6850

Do you want this to be editable?

Actually, is this even used? I didn't find anything with search.

source/blender/modifiers/intern/MOD_nodes.cc
1190–1193

Once I figured this out I saw that this section of the code is actually quite elegant. The explicit typing combined with the way it's broken up-- well done.

However, it would have been a bit faster to figure out with a sentence or two of prose to offer a different perspective than the code. I don't think that's just me, at least I hope not.

Suggest:

/**
 * Based on every visible spreadsheet context path, get a list of sockets that need to have their intermediate geometries cached for display.
 */

Or something like that anyway. Maybe the important thing to realize is that it is not based on any of the flags in the nodes themselves, it's only based on the spreadsheets.

This revision now requires changes to proceed.Apr 14 2021, 6:26 AM
Jacques Lucke (JacquesLucke) updated this revision to Diff 36160.Apr 14 2021, 10:33 AM

first set of updates, still missing a larger rename

  • fix tooltip
  • remove unused variables
  • use property instead of operator
  • remove unused function
  • comments
  • remove NodeTreePath.node_name from rna
  • comment
Harbormaster completed remote builds in B14055: Diff 36160.Apr 14 2021, 10:33 AM
Jacques Lucke (JacquesLucke) updated this revision to Diff 36161.Apr 14 2021, 11:11 AM
  • rename breadcrumbs to context path internally
Harbormaster completed remote builds in B14056: Diff 36161.Apr 14 2021, 11:11 AM
Jacques Lucke (JacquesLucke) added inline comments.Apr 14 2021, 11:12 AM
source/blender/blenkernel/intern/screen.c
1365

It's not just runtime data. The breadcrumbs are the ground truth for what is currently displayed in the spreadsheet. If there are multiple possible contexts for the spreadsheet, the breadcrumbs will remember which one is actually displayed at any moment.

(note getting multiple contexts at the same time is not super easy right now, because pinned node groups don't fully work yet as I just noticed; but that's something that can be fixed a little later)

source/blender/makesdna/DNA_object_types.h
173 ↗(On Diff #36130)

You could argue that the integers are owned by map, but that argument does not seem to help anyone :D

source/blender/makesrna/intern/rna_space.c
6850

Ah right, that is a leftover from when I was doing some stuff in Python that is now in C++.

Dalai Felinto (dfelinto) accepted this revision.Apr 14 2021, 12:23 PM
Dalai Felinto (dfelinto) added 1 blocking reviewer(s): Hans Goudey (HooglyBoogly).
Hans Goudey (HooglyBoogly) accepted this revision.Apr 14 2021, 4:10 PM
Hans Goudey (HooglyBoogly) added inline comments.
source/blender/blenkernel/intern/screen.c
1365

Right, thanks.

This revision is now accepted and ready to land.Apr 14 2021, 4:10 PM
Hans Goudey (HooglyBoogly) added inline comments.Apr 14 2021, 5:16 PM
source/blender/makesrna/intern/rna_space.c
7489

full -> fully

Closed by commit rB3810bcc16047: Spreadsheet: breadcrumbs and node pinning (authored by Jacques Lucke (JacquesLucke)). · Explain WhyApr 15 2021, 9:03 AM
This revision was automatically updated to reflect the committed changes.
Jacques Lucke (JacquesLucke) added a commit: rB3810bcc16047: Spreadsheet: breadcrumbs and node pinning.

Revision Contents
Changeset List

PathSize
release/
scripts/
startup/
bl_operators/
45 lines
7 lines
bl_ui/
27 lines
source/
blender/
blenkernel/
intern/
57 lines
blenloader/
intern/
10 lines
editors/
include/
33 lines
space_node/
49 lines
1 line
space_spreadsheet/
2 lines
70 lines
27 lines
199 lines
util/
1 line
makesdna/
38 lines
makesrna/
4 lines
intern/
35 lines
166 lines
modifiers/
intern/
161 lines

Diff 36029

View Options

release/scripts/startup/bl_operators/node.py

Loading...
View Options

release/scripts/startup/bl_operators/spreadsheet.py

Loading...
View Options

release/scripts/startup/bl_ui/space_spreadsheet.py

Loading...
View Options

source/blender/blenkernel/intern/screen.c

Loading...
View Options

source/blender/blenloader/intern/readfile.c

Loading...
View Options

source/blender/editors/include/ED_spreadsheet.h

Loading...
View Options

source/blender/editors/space_node/node_edit.c

Loading...
View Options

source/blender/editors/space_node/node_ops.c

Loading...
View Options

source/blender/editors/space_spreadsheet/CMakeLists.txt

Loading...
View Options

source/blender/editors/space_spreadsheet/space_spreadsheet.cc

Loading...
View Options

source/blender/editors/space_spreadsheet/spreadsheet_breadcrumb.hh

Loading...
View Options

source/blender/editors/space_spreadsheet/spreadsheet_breadcrumb.cc

Loading...
View Options

source/blender/editors/util/CMakeLists.txt

Loading...
View Options

source/blender/makesdna/DNA_space_types.h

Loading...
View Options

source/blender/makesrna/RNA_access.h

Loading...
View Options

source/blender/makesrna/intern/rna_nodetree.c

Loading...
View Options

source/blender/makesrna/intern/rna_space.c

Loading...
View Options

source/blender/modifiers/intern/MOD_nodes.cc

Loading...