Add Shift + C hotkey to create a nodegraph from selected nodes

[WaffleBoyTom]

Addresses #2018. Shift + C will create nodegraph containing selected nodes. Connections are not preserved.

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: WaffleBoyTom / name: WaffleBoyTom (6b5ac80f9aacc7a95c9b2355cd41d7b679657dd3, aa448b7c80d7a9939bde766e040eb535438f7462, 3f8f4fc178cfe83b8acdff7698b4b72572dd6fa6, 7ac994cf511b6c02c4decfbea697bd042af3fd2f, 0f619339954d6a7321522c9d3ae5ab42dc23d50c, 7005877d7c70abb0e146a1c7f728c615d8e6eb6f, 7a2ad915e13b1b37d3f59510029c46d8d7e06fdd, 982def5eebc3893f6ed3d2e3940ed9b880c793eb, 680f957e1c4f56de117a3ffa77315df5ff5bc45c)
• :white_check_mark: login: jstone-lucasfilm / name: Jonathan Stone (bcb74f2bc01e9235e11fef125e85abfa23b7a162, 9a0d985f11150e669683fd025517a964bf35a3ca, 57d738abc003a95c311d0785edb3e1c5cc3a8955, a5d9e3e932e70a6b1392785c9120a6c18d491fc3, 344e4290242b077877bf88dd3cd904cc358160f1, c01e96be059b8b5aa8610c4c089ff7f06a94fd00, b07faaa71970b83fb1868f8171c56529f72e8bb6, 6483c1924583693c3bfdabe006eb76118e68e2a4)

[jstone-lucasfilm]

@WaffleBoyTom This looks like a great start, and to resolve the EasyCLA issue, I'd recommend following up through the support link that the Linux Foundation provides:

https://jira.linuxfoundation.org/servicedesk/customer/portal/4

[jstone-lucasfilm]

In parallel with the CLA process, I'm CC'ing @lfl-eholthouser for her thoughts on the implementation of the feature.

[jstone-lucasfilm]

I wanted to follow up on this proposal, @WaffleBoyTom, to see if you might have the bandwidth to address the requests above. Let us know either way!

[WaffleBoyTom]

I wanted to follow up on this proposal, @WaffleBoyTom, to see if you might have the bandwidth to address the requests above. Let us know either way!

Yes ! Sorry about the radio silence. I was planning on finishing this up this weekend hopefully !

[jstone-lucasfilm]

This looks very promising, thanks @WaffleBoyTom!

Make sure to resolve the EasyCLA issue above, following the link provided by the Linux Foundation, so that we can merge your work once it's approved by the MaterialX team.

[WaffleBoyTom]

This looks very promising, thanks @WaffleBoyTom!

Make sure to resolve the EasyCLA issue above, following the link provided by the Linux Foundation, so that we can merge your work once it's approved by the MaterialX team.

Hey ! I followed the link and signed the CLA so I think that should be okay ? I might be missing something

[jstone-lucasfilm]

@WaffleBoyTom It looks like the EasyCLA system still needs approvals for some of the commits above, so perhaps you can follow up by clicking their support link?

For further assistance with EasyCLA, [please submit a support request ticket] https://jira.linuxfoundation.org/servicedesk/customer/portal/4).

[WaffleBoyTom]

@WaffleBoyTom It looks like the EasyCLA system still needs approvals for some of the commits above, so perhaps you can follow up by clicking their support link?

For further assistance with EasyCLA, [please submit a support request ticket] jira.linuxfoundation.org/servicedesk/customer/portal/4).

sounds good, let me do that :)

[WaffleBoyTom]

@WaffleBoyTom It looks like the EasyCLA system still needs approvals for some of the commits above, so perhaps you can follow up by clicking their support link?

For further assistance with EasyCLA, [please submit a support request ticket] jira.linuxfoundation.org/servicedesk/customer/portal/4).

I think we did it :o

[jstone-lucasfilm]

@WaffleBoyTom I've been giving this new feature a try in my local build, but so far it's not behaving as I would expect, though I may be misunderstanding the intended design.

I'm clearing the contents of the Graph Editor, creating a small number of nodes at root scope, then selecting them all, as seen in the following screenshot:

I then hit the SHIFT-C key combination, but nothing happens in the editor, and the new logic seems to be hitting this early exit because isNodeGraph is true:

if (!_copiedNodes.empty() && !isNodeGraph)

Just to make sure I didn't accidentally break anything with my own merges and formatting fixes, I tried the same process again with the codebase at the time of your most recent commit, but I get the same results.

What might I be missing?

[WaffleBoyTom]

I then hit the SHIFT-C key combination, but nothing happens in the editor, and the new logic seems to be hitting this early exit because isNodeGraph is true:

if (!_copiedNodes.empty() && !isNodeGraph)

Just to make sure I didn't accidentally break anything with my own merges and formatting fixes, I tried the same process again with the codebase at the time of your most recent commit, but I get the same results.

What might I be missing?

Hey Jonathan ! It seems you're right.. !isNodeGraph was added to prevent nested node graphs but that means that the code fails in your example because tiledhexagons_color3 is a nodegraph. I struggled to find what the right checks should be to prevent nested nodegraphs and this clearly isn't it unfortunately. Would you happen to know how I can correct this ? Thanks :)

[kwokcb]

@WaffleBoyTom, @jstone-lucasfilm : I'll take a quick look. The current logic does need some changes. It should be:

1. Are we currently inside a nodegraph: _isNodeGraph
2. Have we selected any nodes which are nodegraphs: selectedNode->getCategory() == "nodegraph".

[WaffleBoyTom]

Thank you, I should be able to make these changes this weekend

[kwokcb]

This is what I've found that works:

           bool isNodeGraph = _isNodeGraph; // Check if we're already inside a nodegraph
            if (_currUiNode)
            {
                // Check if current node is a nodegraph (also caught below but a faster "early out"
                isNodeGraph |= (_currUiNode->getCategory() == mx::NodeGraph::CATEGORY);
            }
            if (!isNodeGraph)
            {
                for (ed::NodeId selected : selectedNodes)
                {
                    int pos = findNode((int)selected.Get());
                    if (pos >= 0)
                    {
                        UiNodePtr node = _graphNodes[pos];

                        // Check if the selected node is a nodegraph
                        isNodeGraph |= (node->getCategory() == mx::NodeGraph::CATEGORY);
                        if (isNodeGraph)
                        {
                            // Skip all nodes if one of the nodes is a nodegraph
                            break;
                        }

                        _copiedNodes.insert(
                            std::pair<UiNodePtr, UiNodePtr>(_graphNodes[pos], nullptr));
                    }
                }
            }

I checked if you dive in to a node which has a nodegraph implementation that this code isn't even reached since I assume any hot key is prevented from triggering. You probably want to hammer on this a bit more though :).

[WaffleBoyTom]

Thanks !

[jstone-lucasfilm]

@WaffleBoyTom Just checking in on this proposal, to see where things currently stand! Are there outstanding improvements that still need to be made?

[jstone-lucasfilm]

@WaffleBoyTom Would it make sense for us to close out this PR, and you can join us at the next Dev Days in May if you'd like to continue this project?

https://www.aswf.io/dev-days/

[jstone-lucasfilm]

@WaffleBoyTom I'll close out this PR for now, and feel free to pick up the thread of this work at Dev Days in May!