Automatically Offset Nodes on Insertion
ClosedPublic

Authored by Julian Eisel (Severin) on Jun 23 2015, 5:50 AM.

Details

Summary

Automatically Offset Nodes on Insertion

I guess this demo video from @Sebastian Koenig (sebastian_k) describes pretty well what this is, it's using an older patch version, but behaviour hasn't changed since then, it just got some polish

Open Design Questions
As always: How to access? Currently it's the default behavior, but since there is no option to turn this off that's not a really nice one. Some proposals were:

  • Use a modifier key to access
  • Use a time threshold to access (maybe with fancy animations)

Both should be rather easy to implement, but I'd like to wait on overall design approval first

Requested and designed together with @Sebastian Koenig (sebastian_k) during #LSOC ;)


Note: Added a function that iterates over node "chains" (better name?) and calls a callback. We could use this in other places like node_select_linked_to_exec too, but will handle that separately.
Also, I *may* have created some functions that are already there, but I don't know the codebase enough to tell this ;)

Diff Detail

Repository
rB Blender
Branch
auto_offset_nodes
There are a very large number of changes, so older changes are hidden. Show Older Changes

@Julian Eisel (Severin) @Sebastian Koenig (sebastian_k) @Jonathan Williamson (carter2422) Did we already define what keystroke activates / deactivates the functionality? Any proposals?

I don't think we need to define a keystroke for it. It would be enabled by default, and to disable it there is a button. If someone wants a keystroke for it, it should however be possible to add one.

Besides, I wonder if "auto render" couldn't be iconified instead as well? Also, is anyone really using it?

I do, and +1 for icon there as well

A bit off-topic, but indeed, we should look at iconifying other options or move them out of the header if not used frequently (also for other headers).

Don't hesitate to request icons from me. I won't guarantee that I will deliver immediately, but I should find time to do, lets say, 1 per week.

A bit off-topic, but indeed, we should look at iconifying other options or move them out of the header if not used frequently (also for other headers).

Don't hesitate to request icons from me. I won't guarantee that I will deliver immediately, but I should find time to do, lets say, 1 per week.

Cool!
How about using the render icon and putting small circular arrow ("FILE_REFRESH") on it?

Cool!
How about using the render icon and putting small circular arrow ("FILE_REFRESH") on it?

Sure. I'll cook something up and create a separate task for it.

This is offtopic, but if we need more header space, I would

  1. move the "free unused" option from header to the properties panel "performance". I don't know why this option is in the header anyway...
  2. "backdrop" is already in the properties panel. If you want functions like fit or zoom and don't know the shortcut you have to open it anyway.
  3. "auto render" is for me a performance option too.
  4. 2. and 3. could be icons

The feature is nice (I have to test it), but I woudn't set it on as default!

As for the "which direction to move to make space availabe". I think instead of option/button coudn't it be auto guessed and take into account over which node (previous or next) the user is hovering the node which he wants to insert? It would speed up the workflow.

This is offtopic, but if we need more header space, I would

  1. move the "free unused" option from header to the properties panel "performance". I don't know why this option is in the header anyway...
  2. "backdrop" is already in the properties panel. If you want functions like fit or zoom and don't know the shortcut you have to open it anyway.
  3. "auto render" is for me a performance option too.
  4. 2. and 3. could be icons

    The feature is nice (I have to test it), but I woudn't set it on as default!

I agree with almost everything except I don't see a good reason not to set it as default :)

As for the "which direction to move to make space available". I think instead of option/button couldn't it be auto guessed and take into account over which node (previous or next) the user is hovering the node which he wants to insert? It would speed up the workflow.

This looks like a very welcome and usefull addition, thanks for doing this. I would vote default on.

Mere user here, so not sure if this is entirely feasible, or how much more work it would require, but if we could somehow get a 'live preview' of shifted nodes while placing newly created ones we could entirely avoid the need to have the option (or at least reduce the need to have it immediately visible).
As @Michael P. (forest-house) suggested where the new node was dropped over a connection could determine which nodes were offset, if any. Placed more to towards the left of a connection would shift left nodes back, more to the right would move parent nodes to the right, and at some center threshold would disable the auto offset entirely (assuming corner cases of overlaping/close together nodes could be solved).

As @Michael P. (forest-house) suggested where the new node was dropped over a connection could determine which nodes were offset, if any. Placed more to towards the left of a connection would shift left nodes back, more to the right would move parent nodes to the right, and at some center threshold would disable the auto offset entirely (assuming corner cases of overlaping/close together nodes could be solved).

I don't think that would work, because when you need to insert a large node like RGB Curves between too nodes which are already close together, it can be already tricky to even just get the proper highlighting of the noodle (and hence the triggering of auto-insert). Trying to hit the correct distance to the left or right node is impossible like that.

@Julian Eisel (Severin), there is one bug still, which I didn't notice before. Adding a new node into a frame node still fails. Duplicating nodes inside a frame node works great, but adding new ones messes up. I suppose that is because at the time of insert they are not yet child of the parent frame, so they fail to behave like they should. Probably they have to be parented to the frame first before executing the auto-offset thing.

  • Cleanup: Use regular function instead of silly macro
  • Merge branch 'master' into auto_offset_nodes
  • Fix weird behavior when changing frames of a node
  • Use NODE_TEST instead of adding a new flag
  • Various Cleanup

@Sebastian Koenig (sebastian_k), your issue should be fixed now *fingers crossed* ;)

I didn't do anymore changes to icons or behavior yet, but I can add the left-side option + icon-button + shortcut next if everyone agrees on this?


@Campbell Barton (campbellbarton), @Lukas Toenne (lukastoenne), added some more utility functions to node.c, but not sure if this is the best place for them?

I didn't do anymore changes to icons or behavior yet, but I can add the left-side option + icon-button + shortcut next if everyone agrees on this?

Awesome, thanks for fixing! Seems to work good now.
The general consensus here seems to be, that a header button to enable or disable auto-offset would be okay (especially when there is some header cleanup like iconification happening anyway). Switching the direction with a second toggle next to it would be a reasonable solution as well.
Here's why:
Even though it might be nice to have the system automatically guess in witch direction the node tree should grow, i think that would be too error prone. When I want to switch the direction it is because I want to have a certain layout, and for that I think the user should be in control. Having a button makes the option visible for the user, which is also good.
However, for usability's sake I propose to add another option for that in a second step:
Make it possible to toggle the direction not only with the button, but with a shortcut that you can press while hovering the node over the connection. The header bar info text should print the hotkey in the tooltip. Similar to the behavior of edge-slide, where you can press 'E' midway to tell whether it should be even or not.

I would say that while a header button is where the main functionality should be toggled, direction changing could be better to have on a modifier key, like when sculpting for instance and changing the effect of the brush; meaning - standard functionality to be a forward offset, and the one with a modifier - backward.

An info on this could be in the tooltip.

(That coming from a guy who didn't try how it works.)

EDIT: Oops, Sebastian already suggested it above. +1 to that, -1 to header direction toggle.

I would say that while a header button is where the main functionality should be toggled, direction changing could be better to have on a modifier key, like when sculpting for instance and changing the effect of the brush; meaning - standard functionality to be a forward offset, and the one with a modifier - backward.

An info on this could be in the tooltip.

(That coming from a guy who didn't try how it works.)

EDIT: Oops, Sebastian already suggested it above. +1 to that, -1 to header direction toggle.

Ok, *if* it is possible to use this action toggle (not modifier key as alt, ctrl, shift, they're all taken) as I described above with a header tooltip, then I agree and we might not necessarily need a header button for direction ;)

Julian Eisel (Severin) updated this revision to Diff 4533.EditedJun 29 2015, 2:04 AM
  • Implement option to toggle offset direction (currently displayed using a menu)
  • Try yet another icon variation

Didn't implement a shortcut toggle yet, might do that tomorrow.

  • Implement option to toggle offset direction (currently displayed using a menu)
  • Try yet another icon variation

    Didn't implement a shortcut toggle yet, might do that tomorrow.

Cool!
Functionality wise it works really good. But yeah, a shortcut toggle would be better I assume.
Also, if we have a button to toggle, it should be a toggle button instead of a menu. But I guess this was just for testing anyways :)
The icon is cute, looks a bit like a frog in a suit :D
It is less busy than the previous one, but the other one was more descriptive.

minor suggestions.

source/blender/blenkernel/BKE_node.h
376

Would add ntreeNodeFlagSet(tree, flag, bool) to use for enable/disable.

479

Would call nodeFindRootParent

source/blender/blenkernel/intern/node.c
840

For these kinds of iterators, it can be handy if the callback can return false as a signal to early-exit.

source/blender/editors/space_node/node_relationships.c
1431

no need to cast.

1468

no need to cast

1492–1503

Wouldn't alloc data here, it can just be stack-mem and passed in as an arg.

1506

probably shouldn't be const? (has to cast to non-const multiple times in this function)

source/blender/makesdna/DNA_space_types.h
996–997

could use a byte from pad4

1032

Not sure this is worth a typedef (its not used and fairly obscure setting).

source/blender/makesrna/intern/rna_space.c
4217–4229

(naming) use_insert_offset & insert_offset_direction

source/blender/makesdna/DNA_space_types.h
1032

Thought about making more reusable defines like:

#define DIR_COMMON_TOP    1
#define DIR_COMMON_BOTTOM 2
#define DIR_COMMON_LEFT   3
#define DIR_COMMON_RIGHT  4

to get rid of some other defines

  • Print auto-offset information to header during translate
  • Shortcut (Shift+V) to toggle auto-offset on/off
  • Shortcut (T) to toggle offset direction during translate
  • Remove offset direction menu
  • Addressing review points raised by @Campbell Barton (campbellbarton)

With the last update we only have the ability to toggle the direction while translating. We could also add this as a normal shortcut to make it accessible without translating, but then we'd need a visual indicator like another (icon-)button in the header. We can do this easily of course, just wasn't sure if that is what was desired.

  • Print auto-offset information to header during translate
  • Shortcut (Shift+V) to toggle auto-offset on/off
  • Shortcut (T) to toggle offset direction during translate
  • Remove offset direction menu
  • Addressing review points raised by @Campbell Barton (campbellbarton)

Great! I would be fine with it like it is, but just want to mention that it is not consistent with other tool toggles.
Usually the options that you toggle with a hotkey during transform, such as cut through with knife tool, are not permanent but only relevant during that one operation. Once you start the knife tool again it is set back to "normal".
So, while I don't mind how it works currently in node offset I think that for consistency's sake the direction toggle should only be active during one transform operation and then go back to normal.

Also, the last little bit of icing on the cake would be if the noodles would indicate the current direction. You could use the arrows that are used by reroute nodes when there are 2 reroutes in a row. So that if you toggle direction you'd see before which direction the tree would expand. (Only display the arrow during the transform)


But that really is just eye candy which could be implemented when you're bored ;)

Usually the options that you toggle with a hotkey during transform, such as cut through with knife tool, are not permanent but only relevant during that one operation. Once you start the knife tool again it is set back to "normal".

I second that and @Julian Eisel (Severin) s comment about the header icon: I'd like to take the snap tool again as an example. We have a main tool (snap/auto offset) that is customizable for every subsequent use of it (snap to grid, component / auto offset right / left), so a direction specifier combobox ,next to the icon seems right to me - @Jonathan Williamson (carter2422), what do you think about it?

Another minor thing I'd like to see changed is the Hotkey "T". "D" seems much more ideal for the direction to me as we try to use always the first character of the action in our tools to toggle them on / off.

Apart from that - really awesome functionality, much needed!

Usually the options that you toggle with a hotkey during transform, such as cut through with knife tool, are not permanent but only relevant during that one operation. Once you start the knife tool again it is set back to "normal".

I second that and @Julian Eisel (Severin) s comment about the header icon: I'd like to take the snap tool again as an example. We have a main tool (snap/auto offset) that is customizable for every subsequent use of it (snap to grid, component / auto offset right / left), so a direction specifier combobox ,next to the icon seems right to me - @Jonathan Williamson (carter2422), what do you think about it?

Another minor thing I'd like to see changed is the Hotkey "T". "D" seems much more ideal for the direction to me as we try to use always the first character of the action in our tools to toggle them on / off.

Apart from that - really awesome functionality, much needed!

I don't think a box for the direction is needed. It's not like you'd continuously edit your trees to the left, the main direction is right by design. So it's mainly a few occasions where you'd move to the left. Pressing the toggle for that I think is still okay.
I would be fine with "D". It is used for Grease Pencil already, but "T" is also taken by tool shelf. And you wouldn't be able to draw GP during transform anyway. :)

Hi @Sebastian Koenig (sebastian_k),

that maybe the case in your workflow, but you can't generalize this - as we have an infinite node space in every direction and we have users that come from ltr-languages as well as rtl-languages you can't predict how they build their node tree. If they like their nodes to offset generally to the left or to the right is a cultural habit and not at all by design. Even if the node execution itself seems to be from left to right that does not need to be reflected in the offset direction.

Therefore I still vote for a specification combobox. The alternative is to break the modal tool design (where your settings are normally reset on the next call) just like @Julian Eisel (Severin) did it just now - that would be acceptable too maybe but should be approved from our ui team. Personally I don't like to see design principles broken though.

Greetings, Thomas

Hi @Sebastian Koenig (sebastian_k),

that maybe the case in your workflow, but you can't generalize this - as we have an infinite node space in every direction and we have users that come from ltr-languages as well as rtl-languages you can't predict how they build their node tree. If they like their nodes to offset generally to the left or to the right is a cultural habit and not at all by design. Even if the node execution itself seems to be from left to right that does not need to be reflected in the offset direction.

Therefore I still vote for a specification combobox. The alternative is to break the modal tool design (where your settings are normally reset on the next call) just like @Julian Eisel (Severin) did it just now - that would be acceptable too maybe but should be approved from our ui team. Personally I don't like to see design principles broken though.

Greetings, Thomas

Well, I guess it's up to Pablo and Jonathan now ;)
I do think though that the node editor IS in fact design from left to right - otherwise we could swap the inputs/outputs. And I have yet to see one single screenshot of someone working our nodes right to left. Designing for corner cases isn't also good design IMHO.

@Thomas Beck (plasmasolutions) Actually, I misinterpreted your reasoning about left<-->right direction. Of course one can prefer to have output sit fixed and add nodes to the left. In that case the current behavior of toggle would be fine, but different from other tool-toggles. Using a menu is clunky from workflow point of view though.
So! UI-Team! What say you? :)

Yeah, that was a misinterpretation caused by two Germans that are talking in English ;) And just to prohibit further misinterpretation: Having a left / right combobox does not mean for me that you won't have a modal toggle possibility via a hotkey - that would be still needed..

Yeah, that was a misinterpretation caused by two Germans that are talking in English ;) And just to prohibit further misinterpretation: Having a left / right combobox does not mean for me that you won't have a modal toggle possibility via a hotkey - that would be still needed..

Ha! :)
But does that mean the toggle during transform like it is now can stay as long as there is a toggle box in the header too?

Julian Eisel (Severin) planned changes to this revision.EditedJul 3 2015, 2:02 PM

Geez, you both really know how to spam a differential. And I thought we wouldn't need a design task for this... ;P

Rather than waiting on input/decisions from other UI-team members, I think we should work this out on ourselves. The decision making process within the UI-team doesn't work reliably enough to make further development depending on it, unfortunately (speaking out of own experience here - not pointing fingers, just being careful with creating dependencies for projects). The discussion is open and if they feel like giving input they can always do so.

Re shortcut: Don't like to use D because it's already used by GPencil. This may not give us a conflict for this, since it's used as a modifier key there, but these overlapping shortcuts are the main reason why our event system is such a can of worms (see sticky keys) and I'd also like to keep this unassigned for further GPencil development.


I propose we proceed like this:

  • Find a free shortcut to toggle direction (good luck ;) )
  • Make it accessible without having to transform
  • Bring back direction toggle menu to header

To avoid further design discussion here, I opened a design task (T45289), let's keep this focused on review from now on, it's already a wall of text ;)

Julian Eisel (Severin) updated this revision to Diff 4761.EditedJul 29 2015, 2:15 AM
  • Add nice ease in+out animation ;)
  • Don't use hardcoded string for displaying shortcut in header, use real configurable shortcut
  • Merge branch 'master' into auto_offset_nodes
  • Cleanup

Note: Icon is missing here, we can check on that later

  • Forgot that in last update

There is one more problem with frames:
In the attached image I try to insert a node into the lower connection. The frame is not pushed to the right, but the composite node is.

Julian Eisel (Severin) updated this revision to Diff 4771.EditedJul 30 2015, 7:22 PM
  • Fix glitch reported by @Sebastian Koenig (sebastian_k)
  • Add theme option for auto-offset min margin - was agreed on in IRC, it's not really theme related, but in other places it's not so nice to add editor specific options

@Sebastian Koenig (sebastian_k), this fixes only the issue you reported here, not the two you reported in IRC, wasn't able to recreate them (let's check in IRC again)

@Campbell Barton (campbellbarton), think this is ready for further review now.

Wow, everything seems to be working now! Also my issues from yesterday seem to be gone now.
Thumbs up from me!! :)

Campbell Barton (campbellbarton) requested changes to this revision.Jul 31 2015, 3:13 PM

Generally good, requesting changes though.

source/blender/editors/include/UI_resources.h
229 ↗(On Diff #4771)

Having this as a theme option seems rather odd since its not for drawing.

This is related to interactions, I think it belongs in the Editing section.

source/blender/editors/transform/transform.c
4279

Looks like this is a memory leak?

4283

This should be sizeof(str_km), in fact it fails to compile for me (paranoid warnings.)

source/blender/makesdna/DNA_space_types.h
999

This should be NULL'd in readfile.c incase Blender autosaves or somehow writes the value to disk.


Worth commenting this is temporary/runtime data too (could put under linkdrag so temp data's grouped).

Note that this is basically used to pass an argument from one operator in a macro to another.

It would be nice to avoid this, but I didnt see a really good way to do so (unless you have the macro members peek into the next item in the list and manipulate settings). which could be worth supporting infact... but its also kind of ugly, and operators dont support pointers like this... so think its fine as a hack... But think it should be commented /* This is used for BLAH macro to pass FOO to BAR */

This revision now requires changes to proceed.Jul 31 2015, 3:13 PM
source/blender/blenkernel/BKE_node.h
485

Picky, space before * :)

  • Correction to previous update (for icons))
  • Minor typo in header print

Some screenshots:


Final (?!) icon design by @Paweł Łyczkowski (plyczkowski) :)

Julian Eisel (Severin) marked 15 inline comments as done.Aug 1 2015, 2:08 AM
This revision is now accepted and ready to land.Aug 1 2015, 3:09 AM
source/blender/makesrna/intern/rna_userdef.c
3853

Couldn't this use subtype PROP_PIXEL ?

  • Minor corrections to comments and descriptions
source/blender/makesrna/intern/rna_userdef.c
3853

It could, but since node_margin is multiplied with UI_DPI_FAC it's not really a pixel distance. I checked other cases and it seems we only use for DPI independent values.

  • Fix node being wrongly attached to frame with certain screen layouts (accidentally used window space instead of region space coords)

Good catch Sebastian! :)