Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

WaffleBoyTom
Copy link

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

Copy link

CLA Missing ID CLA Not Signed

@jstone-lucasfilm
Copy link
Member

@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
Copy link
Member

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

Copy link
Contributor

@kwokcb kwokcb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Great to see this worked out.
  • I've left some comments about adding some more conditional checking and code cleanup. Thanks.

if (ImGui::IsKeyReleased(ImGuiKey_C) &&
io2.KeyShift &&
!io2.KeyCtrl)
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you put in a check to see if you're not already in a nodegraph as creating nodegraphs nested inside of nodegraphs is currently disallowed in the UI.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix all of these issues ! Thanks :)

!io2.KeyCtrl)
{
// cut nodes
if (!readOnly())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be good to move this up to the above if condition instead of checking in multiple places. e.g. the nodegraph add is missing this check so can create a nodegraph under a read-only nodegraph.


if (_currUiNode != nullptr)
{
if (_currUiNode->getNode() != nullptr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume only 1 of these 2 if / else paths will be taken and uou can remove the other cases logic.
I'm guessing it's the else path that is used ?

{
_popup = true;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably want to do a check if there are any items in _copiedNodes. If it's empty I think you'll end up creating an empty nodegraph with the current logic.

@jstone-lucasfilm
Copy link
Member

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants