Add support for Named Reroutes

[MaksymKapelianovych]

Found a little issue - new named reroute declarations have the same name, either node placed from palette, created from pin or converted from regular reroute. There is 2 ways of fixing it - make node title editable and force edit title when node is created (how it is done in Material Graph) or force making unique name when new node is created. Here is the code for the second option (I prefer this one, because IMO the first one will have considerably more code)

Add to "FlowGraphNode_NamedRerouteDeclaration.h" virtual void PostPlacedNewNode() override;

Add to "FlowGraphNode_NamedRerouteDeclaration.cpp"

void UFlowGraphNode_NamedRerouteDeclaration::PostPlacedNewNode()
{
    UFlowNode_NamedRerouteDeclaration* Declaration = Cast<UFlowNode_NamedRerouteDeclaration>(GetFlowNode());
    if (const UFlowAsset* FlowAsset = GetFlowNode()->GetFlowAsset();
        FlowAsset && Declaration)
    {
        Declaration->MakeNameUnique();
    }
    Super::PostPlacedNewNode();
}

[MaksymKapelianovych]

Found another bug, that can cause crash. Steps to reproduce:

1. Create named reroute declaration and several usages

2. Rename reroute declaration

3. Undo previous action

The problem is in these two lines in UFlowNode_NamedRerouteDeclaration::PostEditChangeProperty():

Usage->SetNodeName();
Usage->GetGraphNode()->ReconstructNode();

Neither of these functions call Modify() on owning object.

To fix this you need to add

void UFlowNode_NamedRerouteUsage::SetNodeName()
{
  Modify();
...
}

and it is better not to call Usage->GetGraphNode()->ReconstructNode(); directly, instead call Usage->OnReconstructionRequested.ExecuteIfBound();

But... I do not see any reason at all to call OnReconstructionRequested.ExecuteIfBound(); in UFlowNode_NamedRerouteDeclaration::PostEditChangeProperty(). It works perfectly fine without it. (You also do not need to chech CustomNodeColor). This is my working version of this function:

void UFlowNode_NamedRerouteDeclaration::PostEditChangeProperty(FPropertyChangedEvent& PropertyChangedEvent)
{
    if (PropertyChangedEvent.GetMemberPropertyName() == GET_MEMBER_NAME_CHECKED(ThisClass, NodeTitle))
    {
        MakeNameUnique();
        for (const UFlowAsset* FlowAsset = GetFlowAsset();
             const auto Usage : FlowAsset->FindNamedRerouteUsages(DeclarationGuid))
        {
            if (Usage)
            {
                Usage->SetNodeName();
            }
        }
    }
    Super::PostEditChangeProperty(PropertyChangedEvent);
}

[MothDoctor]

Hi, please grab the latest changes as this change has merge conflicts with just accepted Flow AddOns.

[hero2xyz]

Potential errors,

1. the nodes (declaration/usage) are not properly registered by Flow debugger
2. copy/duplicate in the same graph
3. copy/duplicate in another graph Although everything seems to be working fine, it appears that my implementation is not entirely correct. I am open to suggestions on how to fix these errors. If there is no progress, I will close this PR until further notice.

[MaksymKapelianovych]

I've been checking this PR for two days but something felt wrong and I finally understood what was that - reroute nodes should be reversed. Sorry I did not notice this earlier.

Declaration node should have only Output pin (optionally Input), and Usages should have only Input pin. You see, now your implementation looks like this,

and if we convert this to regular reroutes we get this,

which is wrong - Output execution pins can have only one connection (even if you connect more pins by code, only first input pin from LinkedTo array will be executed).

You can see in this example that if I try to add more connections to Output pin of the reroute it will replace all other connections.

But reroutes can have Input pin with many connections and this is what we want to be able to convert to named reroutes and back. What should have been instead is when you have this setup with reroute,

after converting to named reroute graph should look like this (it is implementation of named reroutes I was working on),

but in your implementation it looks like this

I see that you took parts of code from Material Graph and made reroutes in the same way, but MaterialGraph is executed from right to left and reroutes are for value pins, not execution pins like in FlowGraph. The difference is that value pins have reversed rules for connecting pins - input pin can have only one connection, output pins can have many connections.

Sorry I missed it when I first checked the code, I think it should be fixed.

[MaksymKapelianovych]

@MothDoctor what do you think about this? Maybe I missed something again(

[hero2xyz]

@MaksymKapelianovych Thank you for mentioning the problems, yes, I think they should be solved. Although I really think that the ability to convert named reroutes to reroutes should be limited, maybe just connect the first reference to LinkedTo, because I honestly don't know how to adapt this functionality for Exec pins, in Material it makes sense, but not for our context.

Possible fixes,

Let me know what you think about that so I can start implementing it.

[MaksymKapelianovych]

From 4 proposed solutions the only suitable one is the third, IMO. But I really think that the main issue is with the current implementation of named reroutes: there should be many input nodes (usages) and one output node (declaration). Named reroutes are intended to eliminate long wires stretching through the graph, so they must follow the rules of basic reroutes and pin connections, and must be easily convertible to and from basic reroutes.

If you do not know how to implement this behaviour I can help or implement it myself (almost did it anyway)

[MaksymKapelianovych]

But if you want to keep current behaviour of your named reroutes, maybe it would be better to untie it from reroutes at all and to make it as an independent feature.

[hero2xyz]

But if you want to keep current behaviour of your named reroutes, maybe it would be better to untie it from reroutes at all and to make it as an independent feature.

As I understand it, my implementation redirects the output of the declaration to all the usages. In contrast, your approach is more conditional, using the usages to call the actual output from the declaration. Considering this, it seems more appropriate to refer to it as a "portal." To switch to this behavior, all I had to do is call the TriggerFirstOutput within the linked declaration. After all, this behavior seems more appropriate and controlled than my original implementation. If all goes well, I will proceed to fix the errors mentioned above when converting Named Reroutes to Reroutes.

[hero2xyz]

@MaksymKapelianovych I updated the code, can you check it?

[MaksymKapelianovych]

Other issues:

1. Node palette is not updated when new named reroute is added/deleted, only after pressing Refresh changes will appear on palette. To make it instant you need to add UFlowGraphSchema::OnNodeListChanged.Broadcast(); when new Declaration is added/deleted. Alternatively you can call UFlowGraph::RefreshGraph(), but it will trigger full graph refresh.

2. Declaration node is not properly registered by Flow debugger. Node execution is captured when its input is triggered, so to make minimal changes we just need to execute Declaration's input as for any other node. Change UFlowNode_NamedRerouteUsage::ExecuteInput to this (to be able to call TriggerInput you need to add friend class UFlowNode_NamedRerouteUsage; to UFlowAsset class declaration):

   void UFlowNode_NamedRerouteUsage::ExecuteInput(const FName& PinName)
   {
   if (LinkedDeclaration)
   {
       Finish();
       GetFlowAsset()->Triggerinput(LinkedDeclaration->GetGuid(), PinName);
   }
   }

   and remove line InputPins = {}; from UFlowNode_NamedRerouteDeclaration's contructor. Declaration node will have default input pin, it will be working fine, but if you want you can hide it in UFlowGraphNode_NamedRerouteDeclaration like this (also require adding friend class UFlowGraphNode_NamedRerouteDeclaration; to UFlowNode class declaration):

   void UFlowGraphNode_NamedRerouteDeclaration::AllocateDefaultPins()
   {
   check(Pins.Num() == 0);
   
   if (UFlowNode* FlowNode = Cast<UFlowNode>(GetFlowNodeBase()))
   {
       for (const FFlowPin& OutputPin : FlowNode->OutputPins)
       {
           CreateOutputPin(OutputPin);
       }
   }
   }

   Before:

After:

The rest looks good)

[MaksymKapelianovych]

I noticed that named reroute still does not dissapear from Palette after deleting named reroute declaration node from the graph. I went to investigate how to do it aaand ... guess it is not that simple and straightforward, especially when it is time to deal with transactions. I also checked Material and PCG graphs (they both have named reroutes) and their palettes do not have named reroute usage actions. So, maybe it would be easier to remove the named reroute actions from FlowGraph's palette rather than try to handle every corner case?

[hero2xyz]

@MaksymKapelianovych Can you check if the new commit fixes the problem?

[MaksymKapelianovych]

Yep, it is working, though I do not like the implementation (calling PostEditChangeProperty manually), but I do not know how to implement it better) I think it is totally fine for now.

[soraphis]

From 4 proposed solutions the only suitable one is the third, IMO. But I really think that the main issue is with the current implementation of named reroutes: there should be many input nodes (usages) and one output node (declaration). Named reroutes are intended to eliminate long wires stretching through the graph, so they must follow the rules of basic reroutes and pin connections, and must be easily convertible to and from basic reroutes. -- @MaksymKapelianovych

while i think you're correct, that if a named reroute is a "reroute" at hearth it should have multiple inputs but only one output. I have to say: thinking of a "named reroute" as a "Sequence" (with don't-care order of outputs) makes the node itself way more useful.

limiting the named reroute to the capability of a reroute itself means I'd have to create a sequence, move every output into it's own named reroute and use these.

I terms of usage (disregarding technical details) this would feel more like the node in the material graph.

[MaksymKapelianovych]

While it would be useful functionality, it will not correlate with existing reroutes, so it should be implemented as a completely separate feature then (without converting to and from reroutes). In theory we can have both features, but it is not up to me) Material graph is different, it has reroutes for values, not for execution, values pins evaluation is reversed compared to execution and executes from right to left (as well as in AnimGraph, for example). Now in this PR reroutes working just like in Material graph. What you want is completely valid, but slightly different feature. @soraphis

[soraphis]

What you want is completely valid, but slightly different feature

Yeah, I agree. And it kind of is my point: I feel like, we should not add this with the current design to the main package. As this would look too similar to the "sequence"-approach, but the sequence approach is way more useful.

but that's just my 2cents, as I would probably disable this one then in my project

[hero2xyz]

@MaksymKapelianovych What do you think about adding both functionalities in this PR? I modified my code to add the ability to invert the multi-in single-out and single-in multi-out functionality dynamically. To achieve this, it was necessary to modify FFlowPin and add a pin visibility property (bIsVisible). I felt it was necessary to hide the pins instead of adding or removing them in each PropertyChange, as I had problems with removing an input and then trying to call TriggerInput in the linked usage (or declaration).

I also tried using AllocateDefaultPins, but this created orphaned pins in each transaction. Now the code looks like this, For reference: https://discord.com/channels/742802606874820619/818589094392889344/1257316766757945415 Let me know if you want me to commit this new feature, or if I should leave the PR as it is now. I believe that extending FFlowPin should be addressed in a separate PR.

[MaksymKapelianovych]

I think my decision-making capabilities end there and it is up to @MothDoctor in what form to add this feature. It would be wrong for my personal preferences to influence other people's workflow with the Flow. But I can check if there are no issues in code)