Details

Reviewers

Lukas Tönne (lukastoenne)
Campbell Barton (campbellbarton)

Group Reviewers

User Interface

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

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

In D1373#27890, @Thomas Beck (plasmasolutions) wrote:

@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

In D1373#27888, @Julian Eisel (Severin) wrote:

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.

In D1373#27895, @Paweł Łyczkowski (plyczkowski) wrote:

In D1373#27888, @Julian Eisel (Severin) wrote:

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

move the "free unused" option from header to the properties panel "performance". I don't know why this option is in the header anyway...
"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.
"auto render" is for me a performance option too.
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.

In D1373#27899, @Michael P. (forest-house) wrote:

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

move the "free unused" option from header to the properties panel "performance". I don't know why this option is in the header anyway...

"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.

"auto render" is for me a performance option too.

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 :)

In D1373#27900, @Michael P. (forest-house) wrote:

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 Tönne (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.

In D1373#28049, @Paweł Łyczkowski (plyczkowski) wrote:

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 ;)

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.

In D1373#28150, @Julian Eisel (Severin) wrote:

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.

Julian Eisel (Severin) mentioned this in D1382: Support Custom Header-Printing During Transform.Jun 30 2015, 3:13 AM

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`

Julian Eisel (Severin) added inline comments.Jul 2 2015, 6:25 AM

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.

In D1373#28401, @Julian Eisel (Severin) wrote:

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 ;)

In D1373#28415, @Sebastian Koenig (sebastian_k) wrote:

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!

In D1373#28419, @Thomas Beck (plasmasolutions) wrote:

In D1373#28415, @Sebastian Koenig (sebastian_k) wrote:

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

In D1373#28421, @Thomas Beck (plasmasolutions) wrote:

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..

In D1373#28424, @Thomas Beck (plasmasolutions) wrote:

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?

Righty right! ;)

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

Julian Eisel (Severin) added a parent revision: D780: Initial step for new modal op info API: Get shortcuts from modal keymap..Jul 3 2015, 3:22 PM

Julian Eisel (Severin) mentioned this in T45289: Automatically Offset Nodes on Insertion.Jul 3 2015, 3:32 PM

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 ;)

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.

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!! :)

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 ↗	(On Diff #4771)	Looks like this is a memory leak?
4283 ↗	(On Diff #4771)	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

Campbell Barton (campbellbarton) added inline comments.Jul 31 2015, 3:44 PM

source/blender/blenkernel/BKE_node.h
485	Picky, space before `*` :)

tyoc213 (tyoc213) added a subscriber: tyoc213 (tyoc213).Jul 31 2015, 5:51 PM

Addressing review points made by @Campbell Barton (campbellbarton)
Bring back icon
Minor tooltip edit
Remove unwanted changes for final merge

Correction to previous update (for icons))

Minor typo in header print

Some screenshots:

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

Campbell Barton (campbellbarton) accepted this revision.Aug 1 2015, 3:09 AM

Campbell Barton (campbellbarton) edited edge metadata.

This revision is now accepted and ready to land.Aug 1 2015, 3:09 AM

Campbell Barton (campbellbarton) added inline comments.Aug 1 2015, 6:50 AM

source/blender/makesrna/intern/rna_userdef.c
3853 ↗	(On Diff #4785)	Couldn't this use subtype `PROP_PIXEL` ?

Minor corrections to comments and descriptions

Julian Eisel (Severin) added inline comments.Aug 1 2015, 12:40 PM

source/blender/makesrna/intern/rna_userdef.c
3853 ↗	(On Diff #4786)	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! :)

Committed rBfa1945c9e52e7afe, rB31bf82c17d780660 and rB47bc66fc8dfe2b89e2, thanks all for the feedback :)

Souvik Karmakar (souvik) awarded a token.Aug 3 2015, 3:38 PM

Automatically Offset Nodes on Insertion
ClosedPublic
Actions

Details

Automatically Offset Nodes on Insertion

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 4506

release/datafiles/blender_icons.svg

release/scripts/startup/bl_ui/space_node.py

source/blender/blenkernel/BKE_node.h

source/blender/blenkernel/intern/node.c

source/blender/editors/include/ED_node.h

source/blender/editors/include/UI_icons.h

source/blender/editors/space_node/node_intern.h

source/blender/editors/space_node/node_relationships.c

source/blender/editors/transform/transform_conversions.c

source/blender/makesdna/DNA_space_types.h

source/blender/makesrna/intern/rna_space.c

Automatically Offset Nodes on InsertionClosedPublicActions

Details

Automatically Offset Nodes on Insertion

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 4506

release/datafiles/blender_icons.svg

release/scripts/startup/bl_ui/space_node.py

source/blender/blenkernel/BKE_node.h

source/blender/blenkernel/intern/node.c

source/blender/editors/include/ED_node.h

source/blender/editors/include/UI_icons.h

source/blender/editors/space_node/node_intern.h

source/blender/editors/space_node/node_relationships.c

source/blender/editors/transform/transform_conversions.c

source/blender/makesdna/DNA_space_types.h

source/blender/makesrna/intern/rna_space.c

Automatically Offset Nodes on Insertion
ClosedPublic
Actions

Revision Contents
Changeset List