UFlowGraphNode::ReconstructNode and associated functions rewrite to clean up graph behavior #264
Conversation
1) Make all pin types and sources correctly update when modified, 2) fix unnecessary dirtying of flow asset upon opening (unnecessary Modify() calls)
- Added UFlowGraph "IsSaving" state; made UFlowGraphNode skip node reconstruction during save - Added UFlowGraphNode "IsDestroyingNode" state, made UFlowGraphNode skip node reconstruction during node deletion - Fixed minor bug from previous commit which broke certain transaction undos in ReconstructNode()
| @@ -93,16 +93,9 @@ void UFlowGraph::RefreshGraph() | |||
| } | |||
| } | |||
|
|
|||
| // This function will (eventually) result in all graph nodes being reconstructed | |||
| UnlockUpdates(); | |||
There was a problem hiding this comment.
I think it is better to remove UpdateAsset() call from UnlockUpdated(), because it's unclear that this function does this (unless you specifically look into the function body) and add it instead of deleted code below.
And actually instead of having two functions just to change one bool value I would write TGuardValue<uint32> LockUpdatesGuard(bLockUpdates, true); at the beginning of the corresponding block (or even simpler bLockUpdates = true/false). Or provide a better name for UnlockUpdates(), for example, UnlockUpdatesAndUpdateAsset() (too mouthful and clearly states two separate responsibilities of this function).
There was a problem hiding this comment.
I very much agree in principle! However, the UnlockUpdates() code basically connects directly into the new "Addons" code, and I haven't studied the addons code at all yet (I don't use any addons). So I think this topic might make a good future PR for further cleanup, but I'm not sure if I want to risk expanding this PR into it. It would take me a while to make sure I'm comfortable with it (might not have more free time for a few weeks).
(PS - thanks for the review!)
| // Corrupt? This should never happen | ||
| if (!IsValid(NodeInstance)) | ||
| { | ||
| UE_LOG(LogFlow, Error, TEXT("FlowGraphNode has no NodeInstance, graph may be corrupt!")) |
There was a problem hiding this comment.
Maybe add more info about the node being corrupt and also add ensureMsgf(Always)?
| { | ||
| if (UFlowAsset* FlowAsset = NodeInstance->GetFlowAsset()) | ||
| Modify(); |
There was a problem hiding this comment.
Shouldn't this call be before TryUpdateAutoDataPins()?
There was a problem hiding this comment.
Not necessary because the flow graph node isn't modified until this point (and it doesn't really matter when exactly you call Modify, as long as it gets called sometime during the process), but!! You made me realize I should have called Modify() on the FlowNode, elsewhere in my new functions. Thanks :)
There was a problem hiding this comment.
I missed that pins are being changed in FlowNodeInstance and not in the UFlowGraphNode, but yeah, I meant that)
| for (const FFlowPin& Left : LeftPins) | ||
| { | ||
| // For each required pin, make sure the existing pins array contains a pin that matches by name and type | ||
| if (!RightPins.ContainsByPredicate( [&Left] (const FFlowPin& Right) |
There was a problem hiding this comment.
I would move lambda out of if statement - easier to read.
- Added 2 missing FlowNode Modify() calls - Replaced logs with ensures - Cleanup Lambda
ReconstructNode()would frequently run 2 or 3 times before, usually only runs once nowReconstructNode(): no longer reconstructs before saving the graph, no longer reconstructs a node before deleting the nodeReconstructNode()completely rewritten to clean up logic and resolve bugs with 'false' graph asset dirtying or missed pin change updatesUFlowGraphNode::RefreshContextPins()function completely replaced by two new substantial functions:TryUpdateAutoDataPins()TryUpdateNodePins()