[go: up one dir, main page]

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

Several improvements for copy/pasting nodes: #219

Open
wants to merge 2 commits into
base: 5.x
Choose a base branch
from

Conversation

MaksymKapelianovych
Copy link
Contributor
@MaksymKapelianovych MaksymKapelianovych commented Jul 26, 2024
  1. UFlowNode_Start now is substituted with UFlowNode_CustomInput when duplicating or copy/pasting. Error in Copy/Paste node #205
  2. Reset EventName in UFlowNode_CustomInput after duplicating or copy/pasting
  3. Fix copy/pasting comments. Copy Pasting comments broken in 5.x #215
  4. CanUserDeleteNode and CanDuplicateNode now use FlowNode's CDO if NodeInstance is NULL (always when pasting). This will prevent pasting nodes that cannot be duplicated, but are copied with other duplicable nodes.
  5. Fix pasting nodes into FlowAsset when FlowAsset cannot accept such nodes (node or asset class is denied).
  6. If among selected nodes there are nodes nodes that cannot be deleted, they will stay in graph as is and all "deletable" nodes will be deleted (currently none of the nodes will be deleted in such case)
  7. Improve duplicating and copy/pasting of AddOns (as a reference was taken AIGraphEditor code)
  • duplicated AddOn is pasted on the same level as selected AddOn
  • allow pasting AddOns only if there is one SelectedNode
  • check if SelectedNode for paste can accept AddOn
  • fix copying AddOns if both AddOn and its ParentNode are selected
  • fix copying AddOns if there are other GraphNodes selected (not AddOns)

1. UFlowNode_Start now is substituted with UFlowNode_CustomInput when duplicating or copy/pasting. MothCocoon#205
2. Reset EventName in UFlowNode_CustomInput after duplicating or copy/pasting
3. Fix copy/pasting comments. MothCocoon#215
4. CanUserDeleteNode and CanDuplicateNode now use FlowNode's CDO if NodeInstance is NULL (always when pasting). This will prevent pasting nodes that cannot be duplicated, but are copied with other duplicable nodes.
5. Improve duplicating and copy/pasting of AddOns (as a reference was taken AIGraphEditor code)
  - duplicated AddOn is pasted on the same level as selected AddOn
  - allow pasting AddOns only if there is one SelectedNode
  - check if SelectedNode for paste can accept AddOn
  - fix copying AddOns if both AddOn and its ParentNode are selected
  - fix copying AddOns if there are other GraphNodes selected (not AddOns)
@MaksymKapelianovych
Copy link
Contributor Author

For this issue I went with second option (Substitute UFlowNode_Start with UFlowNode_CustomInput) #205. Please let me know if you prefer different solution.

@MothDoctor @LindyHopperGT please take a look, especially AddOn part)

@LindyHopperGT
Copy link
Contributor
LindyHopperGT commented Jul 26, 2024

I am just now catching-up on this. I have a few thoughts.

I think there's a version of #3 that uses NodeInstanceClass if NodeInstance does not have a value.

AssignedNodeClasses is a filter, not a definition for the 'type' of UFlowGraphNode, but NodeInstanceClass defines what 'type' the runtime NodeInstance must be.

If both are null when CanDuplicateNode() is called, then that seems like the flow graph is degenerate or malformed, because that UFlowGraphNode has no type. In this case, you could make the argument that the answer should be 'false' (ie, "do not duplicate a malformed node").

I favor the #3 solution generally, because it is simplest. If we can detect the illegal operation (that is, trying to copy a set of nodes, that includes an uncopyable node) and then reject the entire copy operation as a result; we can prevent any further need to handle the complexities of copying some, but not all, the nodes and what that means.

I don't like the #2 solution that changes the node type, that is a strange semantic choice that doesn't follow from the concept of a 'copy request'.

I think, if we do allow the copy (which I prefer to reject the whole thing, if we can), then the #1 solution of filtering out the uncopyable nodes makes the most sense.

--

Take these comments with a disclaimer of me having only about 20mins of researching and thinking about this problem, first thing in the morning ... so I may be missing some nuance(s).

@MaksymKapelianovych
Copy link
Contributor Author

Solution №3 is implemented as a general one for all nodes, but I thought that it would be nice to implement №2 for Start node and to create CutomInput node if user deliberately copied Start node, maybe for some debug purposes. But it is not necessary to have this feature, we can either make it optional by enabling/disabling in FlowGraphSettings or completely remove it

UFlowNodeBase* Node = NodeInstanceClass->GetDefaultObject<UFlowNodeBase>();
return Node->bCanDuplicate;
}
return Super::CanDuplicateNode();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need to allow duplicating node if NodeInstance and NodeInstanceClass are invalid? This node is clearly malformed in this case. Maybe just return false?

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

Successfully merging this pull request may close these issues.

3 participants