Page MenuHome

Spreadsheet: Show data of node.
ClosedPublic

Authored by Jacques Lucke (JacquesLucke) on Apr 1 2021, 2:52 PM.

Details

Summary

This allows the spreadsheet editor to display intermediate data from the geometry node tree evaluation.

Currently, the data of the active node is displayed when the object evaluation state is set to "Node" in the spreadsheet editor.
Whether we really use the active node or introduce a more explicit operator to select the node to preview is still up to debate.
I'll cleanup up the corresponding parts of the code once we agreed on a behavior.

Diff Detail

Repository
rB Blender
Branch
spreadsheet-active-node (branched from master)
Build Status
Buildable 13915
Build 13915: arc lint + arc unit

Event Timeline

Jacques Lucke (JacquesLucke) requested review of this revision.Apr 1 2021, 2:52 PM
Jacques Lucke (JacquesLucke) created this revision.
source/blender/blenkernel/intern/geometry_component_instances.cc
113

So I guess "everything except object/collection instances" doesn't also refer to the instance transforms. Maybe add a comment here.

source/blender/editors/space_node/node_edit.c
674

typo: dependend -> dependent

676

It would be nice to make sure this only happens for geometry node trees.

source/blender/modifiers/intern/MOD_nodes.cc
1220

Do you expect to pass different functions instead of log_socket_value in the future? I'm just wondering if this really needs to be a lambda function. I guess that was decided in the previous commit that added the callback to the evaluator though.

source/blender/editors/space_node/node_edit.c
676

Indeed. It would be even better if we wouldn't need this code at all, by not using the active state of a node :D

source/blender/modifiers/intern/MOD_nodes.cc
1220

Yeah, I want to move the ui storage stuff into this callback as well. Also if we get socket inspection, we can use this callback as well.
It doesn't have to be a lambda. I'll probably move most of the code from this lambda into a separate function once we decided on the behavior. The main reason I used a lambda is that it made it easy to capture some variables from the outer scope.

Works great!

About the question of active node vs explicit node inspection my immediate reaction would be to make it explicit and use the same/a similar mechanism for the viewport inspection (and even other node systems).
I will try working with it some more to get more impressions.

  • add more explicit preview toggle

I used this icon in lack of a better option. Suggestions are welcome.

Dalai Felinto (dfelinto) edited the summary of this revision. (Show Details)Apr 7 2021, 2:32 PM

Working like a charm - open questions:

  • Pinning is still missing
  • How to indicate in the spreadsheet when there is no "active" node
This revision is now accepted and ready to land.Apr 7 2021, 3:30 PM

Thanks for making this change, I much prefer the way this works.
I do think that it has to be more clearly readable which node is being inspected. The icon alone does not grab a lot of attention and it is quite crucial which node is inspected, especially once this is connected to a viewport inspection.
So I would propose adding something more striking that identifies the inspected node in the UI.
An idea that comes to mind would be a thick border of multiple pixels around the node.


Maybe @Pablo Vazquez (pablovazquez) has a good idea for that.

Some other things to consider:

  • Context pinning should eventually work with this
  • Breadcrumbs should show the Object>Modifier>NodeTree>Node
  • Multiple geometry outputs need an additional solution. Right now it seems to arbitrarily pick the last output.

  • In my opinion there should be a shortcut associated to toggling this on a node, as it is something that will likely be switched around often. In parallel to the node wrangler addon this could just be Ctrl++LMB

(This could be used to cycle through multiple geometry outputs, but ideally it would be possible to select the needed output directly.)

Thanks for the feedback.

Pinning is still missing.

True. As mentioned before, this is a bit more tricky for nodes, but we can make it work. I'll work in it separately. I assume this could go in as a bug fix in bcon3 as it does not really change the ui (depending on how it is done).

How to indicate in the spreadsheet when there is no "active" node.

That's right, feedback from the designers would be good here.

An idea that comes to mind would be a thick border of multiple pixels around the node.

Drawing a big outline seems to be the "lazy solution", not sure if that is a good idea. Some more feedback and mockups on this would be appreciated indeed.

Multiple geometry outputs need an additional solution. Right now it seems to arbitrarily pick the last output.

Oh oops, I thought it would pick the first output (which I think is better for as long as don't have switching between sockets). Will fix that.

In my opinion there should be a shortcut associated to toggling this on a node, as it is something that will likely be switched around often.

Are you sure that will be necessary often? How often does a node have multiple geometries as output?
Either way, using a shortcut for this is fine with me. How should the previewed output be visualized though?

Jacques Lucke (JacquesLucke) retitled this revision from Spreadsheet: Show data of active node (WIP). to Spreadsheet: Show data of node..Apr 7 2021, 5:03 PM

Are you sure that will be necessary often? How often does a node have multiple geometries as output?

Misunderstanding, I think. I meant that in general changing the inspected node is something I expect to be done often (especially with viewport display). Not toggling the inspected output socket.
My bad, I phrased this poorly.

How should the previewed output be visualized though?

Depends on what we go for in terms of highlighting the inspected node. The specific socket should also be considered in that design. I think it might be good to make a separate task for that.
For now, I think, it's fine to only go with the first output as not many nodes have multiple geometry outputs and it's easy to work around this with a dummy node that does nothing.

@Jacques Lucke (JacquesLucke) my proposal to keep things simple:

  • Leave better ways to indicate which is the active/debugged node for a future pass if needed.
  • Pinning working for nodes can come in bcon3, but showing the full breadcrumbs is pre-bcon3 ideally.
  • Can we have a tooltip for the node "Preview" toggle? (e.g., "Show this node's output in the spreadsheet editor Node mode")
  • As to how to show that there is no active node: "In the breadcrumbs that can be a message": 'My Object' > 'Nodetree' > "<!> No Node Selected".

Other options:

  • Have a similar message in the main region itself (and remove the alternating lines in this case.
  • Always have a node I: (it can fallback to the Group Output Node explicitly)
  • Always have a node II: (it can fallback to the Group Output Node implicitly)
  • improve toggle in header
  • cleanup

@Dalai Felinto (dfelinto), the proposal sounds fine to me. Then this patch mainly needs code review node.
When it is done, I'll try to get breadcrumbs working before bcon3.

Initial impressions and suggestions:

  • This is super needed, thanks.
  • It wasn't obvious that I needed to change the spreadsheet evaluation state to get this to work.
    • Link the evaluation state dynamically so if a node is explicitly toggled the spreadsheet automatically reflects this.
  • It is not possible to uncheck a toggled node.
    • Make the toggle work as a true toggle so it can be unchecked. When untoggled, the evaluation state revert backs to Final.
  • Having an icon on all the nodes is additional visual noise. Although it is cleaner than having a viewer node.
    • Needs a design task perhaps!
  • It wasn't obvious that I needed to change the spreadsheet evaluation state to get this to work.
    • Link the evaluation state dynamically so if a node is explicitly toggled the spreadsheet automatically reflects this.
  • It is not possible to uncheck a toggled node.
    • Make the toggle work as a true toggle so it can be unchecked. When untoggled, the evaluation state revert backs to Final.

I didn't consider this before, but I totally agree with both these points and suggestions.

Before changing the spreadsheet automatically, I'd like to have more details on what should happen when there are multiple node editors and/or more spreadsheets..

I think I found a bug:

Basically no row, yet plenty of columns.

Hans Goudey (HooglyBoogly) accepted this revision.EditedApr 8 2021, 4:48 PM

After testing this I strongly agree that it should work like Charlie suggested. A node with the "viewer" flag should override the "Final" mode, and you should be able to click the toggle again to remove it.

Also, we should look into using this flag for viewport display in the future, combined with the above suggestions it would be quite intuitive.

Also agree that it's pretty noisy to have an icon on every node, but I don't currently have any better idea.

However, I understand it's important to get a basic working version in, so I'll accept this for now since there is still time to improve the situation for 2.93.

source/blender/blenkernel/BKE_object.h
73

I would add "set", so BKE_object_set_preview_geometry_set

source/blender/modifiers/intern/MOD_nodes.cc
1096–1172

I found it pretty hard to understand the purpose of these functions at first. Probably because "matching" is in all of the names bit it's not immediately clear what is being matched.
I would add a couple comments here to explain the big idea, or maybe naming can be improved:
Maybe instead of "matching" you can use "preview", so find_preview_tree_context, etc.

  • check if modifier is active
  • rename functions

This should be exposed in the (messy and due for re-organization) context menu too.