search

MothCocoon / FlowGraph

Design-agnostic node system for scripting game’s flow in Unreal Engine
https://discord.gg/Xmtr6GhbmW
MIT License
1.17k stars 228 forks source link

Several improvements for copy/pasting nodes: #219

Open MaksymKapelianovych opened 1 month ago

MaksymKapelianovych commented 1 month ago
  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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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