Several improvements for copy/pasting nodes:

[MaksymKapelianovych]

1. UFlowNode_Start now is substituted with UFlowNode_CustomInput when duplicating or copy/pasting. https://github.com/MothCocoon/FlowGraph/issues/205
2. Reset EventName in UFlowNode_CustomInput after duplicating or copy/pasting
3. Fix copy/pasting comments. https://github.com/MothCocoon/FlowGraph/issues/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)

[MaksymKapelianovych]

For this issue I went with second option (Substitute UFlowNode_Start with UFlowNode_CustomInput) https://github.com/MothCocoon/FlowGraph/issues/205. Please let me know if you prefer different solution.

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

[LindyHopperGT]

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]

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

[MothDoctor]

Hey @MaksymKapelianovych would you be willing to update this PR? A lot changed since the initial requests, mostly because of the huge Data Pins submitted.

#3 and #4 aren't needed anymore, as I fixed it while going through bug reports. I don't think #1 is needed. StartNode shouldn't be copied as a different node, as this would be confusing.

But #2 - resetting EventName while duplicate CustomEvent itself sounds useful! As well as #5 and #7! I would gladly merge that in :)

Not sure though if we still need #6?

[LindyHopperGT]

I think, If both are invalid, we don’t want to duplicate it. On Aug 1, 2024, at 5:18 AM, MaksymKapelianovych @.***> wrote: @MaksymKapelianovych commented on this pull request.

In Source/FlowEditor/Private/Graph/Nodes/FlowGraphNode.cpp:

}

bool UFlowGraphNode::CanDuplicateNode() const {

• return NodeInstance ? NodeInstance->bCanDuplicate : Super::CanDuplicateNode();

• if (NodeInstance)
• {

•   return NodeInstance->bCanDuplicate;
  

• }
• else if (NodeInstanceClass)
• {

•   UFlowNodeBase* Node = NodeInstanceClass->GetDefaultObject<UFlowNodeBase>();
  

•   return Node->bCanDuplicate;
  

• }
• return Super::CanDuplicateNode();

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?

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

[MaksymKapelianovych]

@LindyHopperGT quick question (I hope so), I am wondering why UFlowGraphNode::CanAcceptSubNodeAsChild accept TSet<EdGraphNode*> AllRootSubNodesToPaste if it is used inside for loops, where for each AddOn CanAcceptSubNodeAsChild is called anyway? When and why it can be useful for Node to know all RootAddOns at once?

[LindyHopperGT]

I will need to look at the code to answer this question. I will try to get back to you within a day. Today is a bit busy. Cheers,Greg TaylorOn Dec 19, 2024, at 1:38 PM, MaksymKapelianovych @.**> wrote: @LindyHopperGT quick question (I hope so), I am wondering why UFlowGraphNode::CanAcceptSubNodeAsChild accept TSet<EdGraphNode> AllRootSubNodesToPaste if it is used inside for loops, where for each AddOn CanAcceptSubNodeAsChild is called anyway? When and why it can be useful for Node to know all RootAddOns at once?

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

[MaksymKapelianovych]

@LindyHopperGT have you looked at the code, by any chance?

[LindyHopperGT]

When and why it can be useful for Node to know all RootAddOns at once?

Some situations occur where a FlowNodeBase can only have a specific combination of AddOn children. For instance, the PredicateNot addon only allows a single subnode child.

It is for this reason that we need to pass all of the root subnodes to paste into the function, so that it can determine if the set of pasted subnodes qualify its (client-code-determined) conditions.

Other examples include a base which can only accept subnodes with one of each of three different interfaces, let's call them A, B, and C. If you already have a child node with interface B, then a set of subnodes are attempting to paste with subnodes B and C, we would need to reject the paste operation, since allowing it would result in two B child nodes.

The exact rules of what subnodes are allowed is left to the FlowNodeBase & AddOns to mutually determine the answer.

The interface for CanAcceptSubnodeAsChild has seen some revisions, and I could imagine some refactoring that would reduce redundant calls. But I'd need to get to that when I am back from vacation (Jan 6)

Sorry for the slow reply. On vacation, but very busy and PC access has been limited.

On Thu, Dec 19, 2024 at 1:38 PM MaksymKapelianovych < @.***> wrote:

@LindyHopperGT https://github.com/LindyHopperGT quick question (I hope so), I am wondering why UFlowGraphNode::CanAcceptSubNodeAsChild accept TSet<EdGraphNode*> AllRootSubNodesToPaste if it is used inside for loops, where for each AddOn CanAcceptSubNodeAsChild is called anyway? When and why it can be useful for Node to know all RootAddOns at once?

— Reply to this email directly, view it on GitHub https://github.com/MothCocoon/FlowGraph/pull/219#issuecomment-2555818121, or unsubscribe https://github.com/notifications/unsubscribe-auth/AV5IMZXCTLAVRBO4DV2WOLD2GM4G7AVCNFSM6AAAAABLQCRTWGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNJVHAYTQMJSGE . You are receiving this because you were mentioned.Message ID: @.***>

[MaksymKapelianovych]

Okay, that makes sense, thank you, for your answer!

[MaksymKapelianovych]

@MothDoctor, Updated PR, left only 2, 5 and 6 (this can be easily removed, if you want). I decided not to proceed with 7, because many issues I tried to solve are already fixed, and for others I need more time and investigation how to fix it, taking into account significant code changes, since this PR was originally made.

[MothDoctor]

I'm sorry you waited that long, but this change is finally merged into the mainline! Thank you :)