Include node name for socket animation channel UI
ClosedPublic
Actions

Authored by Philipp Oeser (lichtwerk) on Nov 3 2021, 12:21 PM.

Details

Reviewers

Jacques Lucke (JacquesLucke)
Hans Goudey (HooglyBoogly)

Maniphest Tasks

T91917: Animated Values in Node Trees and Geometry Nodes Modifier are not distinguishable

Commits

rB8b516d871202: Include node name for socket animation channel UI

Summary

The channel names were often indistingushable in animation editors.
Now include the node _name_ (unfortunately, getting the _label_ could
result in bad performance in some circustances -- see previous version of D13085).
Similar to what rB77744b581d08 did for some VSE strip properties.

before

after

before

after

ref. T91917

Diff Detail

Repository

rB Blender

Branch

T91917 (branched from master)

Build Status

Buildable 18427
Build 18427: arc lint + arc unit

Event Timeline

Philipp Oeser (lichtwerk) requested review of this revision.Nov 3 2021, 12:21 PM

Philipp Oeser (lichtwerk) created this revision.

Harbormaster completed remote builds in B18427: Diff 44299.Nov 3 2021, 12:21 PM

Philipp Oeser (lichtwerk) added projects: Nodes, Animation & Rigging, User Interface.Nov 3 2021, 12:22 PM

I like the change in general, however the call to nodeFindNode is not good enough unfortunately. It currently has to iterate over all nodes and sockets internally to find the node. This could make the editor much slower under some circumstances.
You could try to extract the node name from the rna path instead.

This revision now requires changes to proceed.Nov 3 2021, 12:48 PM

retrieve name from rna path for performance reasons (label is not possible anymore)

Harbormaster completed remote builds in B18429: Diff 44301.Nov 3 2021, 1:07 PM

Philipp Oeser (lichtwerk) retitled this revision from Include node name/label for socket animation channel UI to Include node name for socket animation channel UI.Nov 3 2021, 1:08 PM

Philipp Oeser (lichtwerk) edited the summary of this revision. (Show Details)

Philipp Oeser (lichtwerk) edited the summary of this revision. (Show Details)Nov 3 2021, 1:11 PM

Nice, much better than before.

source/blender/editors/animation/anim_ipo_utils.c
39	The includes are not necessary anymore.
152	typo (`distinguishable`)

This revision is now accepted and ready to land.Nov 3 2021, 1:20 PM

Looks great! Maybe eventually we can hide the "Default Value" text too, this function looks complicated enough right now though.

Jay Edwards (JayE01) awarded a token.Nov 3 2021, 3:07 PM

Closed by commit rB8b516d871202: Include node name for socket animation channel UI (authored by Philipp Oeser (lichtwerk)). · Explain WhyNov 3 2021, 3:35 PM

This revision was automatically updated to reflect the committed changes.

Philipp Oeser (lichtwerk) added a commit: rB8b516d871202: Include node name for socket animation channel UI.

In D13085#345351, @Hans Goudey (HooglyBoogly) wrote:

Looks great! Maybe eventually we can hide the "Default Value" text too, this function looks complicated enough right now though.

I agree. Default Value doesn't say anything and makes the string too long to even see the useful bit in small sidebars. For a different patch (and a welcome one!).
Thanks for working on this quality of life improvement.

dupoxy (dupoxy) awarded a token.Nov 3 2021, 5:10 PM

In D13085#345424, @Pablo Vazquez (pablovazquez) wrote:

In D13085#345351, @Hans Goudey (HooglyBoogly) wrote:

Looks great! Maybe eventually we can hide the "Default Value" text too, this function looks complicated enough right now though.

I agree. Default Value doesn't say anything and makes the string too long to even see the useful bit in small sidebars. For a different patch (and a welcome one!).
Thanks for working on this quality of life improvement.

The thing is that we cant just swap the display from "Property (Node : Socket)" to the more appealing "Socket (Node)", because (at least in theory) other socket properties could be animated:

D13085.blend759 KBDownload

Of course I agree that in 99% of cases it will be Default Value being animated here and displaying that feels redundant.
So how could we do this?

display "Node : Socket" in the case of Default Value? And "Property (Node : Socket)" otherwise?
display "Socket (Node)" in the case of Default Value? And "Property (Node : Socket)" otherwise? (this might make sense for most people, but being strict this would be wrong because the socket is not the property being animated here...)