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

Flow Traits are now stored locally for every user #199

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

Conversation

MaksymKapelianovych
Copy link
Contributor

@MaksymKapelianovych MaksymKapelianovych commented May 6, 2024

Breakpoints state (node and pins) was not serialized and after launching editor all breakpoints was disabled.

@MothDoctor
Copy link
Contributor

Yes, this is intended behavior. It would serialize the Enabled property, it would make it possible to submit assets with enabled breakpoints to the repository. That would later infuriate other team members - suspending the game out of the blue. So they would disable breakpoint and submit to the repository. It would be a vicious cycle.

That's why blueprints only save "a breakpoint was placed here", but not an "Enabled" state.

Thus I cannot accept change in this form. I would gladly take it in, if you would find a way to keep this state as local, user-specific data. Not as information serialized to the game asset itself.

@MothDoctor MothDoctor self-assigned this May 7, 2024
@MaksymKapelianovych
Copy link
Contributor Author

Thanks for your quick response!
Yes, very good argument for not making this serializable. I will try to find the way how it is done in Blueprints and implement this properly.

@MaksymKapelianovych
Copy link
Contributor Author

Moved traits from UFlowGraphNode to UFlowGraphEditorSettings, which is using config EditorPerProjectUserSettings, so now traits will not be part of an asset. I decided to store them per node, as it was previously. Also enum EFlowTraitType was added to leave possibility to easily add new trait in future. Previously it was enough to add new UFlowGraphNode member FFlowPinTrait Something;, but now it is not the case anymore, so I decided enum would be optimal solution. All functions to work with traits now contained in UFlowDebuggerSubsystem as static functions, FFlowDebugTrait can be modified only by UFlowDebuggerSubsystem.
For reference was taken UBlueprintEditorSettings and FKismetDebugUtilities.

Tested scenarios:

  1. Add/remove/toggle/disable breakpoints and restarting editor - everything restores as it should
  2. Remove all breakpoints - PerNodeSettings map is cleared in config
  3. Delete pin/node with added breakpoint - breakpoint is deleted from PerNodeSettings
  4. Rename pin (regular and context pins) - new pin is generated and new one is created, so breakpoint is deleted with the old one
  5. Breakpoint actions on ghost nodes (with unset or deleted UFlowNode) - everything work as for normal node
  6. Breakpoins are not duplicated when duplicating node/graph

@MaksymKapelianovych MaksymKapelianovych changed the title Mark bEnabled as UPROPERTY to keep breakpoints state between editor sessions Flow Traits are now stored locally for every user May 14, 2024

// It can represent any trait added on the specific node instance, i.e. breakpoint
USTRUCT()
struct FLOW_API FFlowPinTrait
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved FFlowPinTrait to UFlowDebuggerSubsystem as it is used only in Editor module and represents only debug feature


// It can represent any trait added on the specific node instance, i.e. breakpoint
USTRUCT()
struct FLOWEDITOR_API FFlowDebugTrait
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed FFlowPinTrait to FFlowDebugTrait to better show purpose of this struct. Can rename back if needed)

@MaksymKapelianovych
Copy link
Contributor Author

It is very easy to add new trait types. For example, we want to add "Log" trait, which will print to log node/pin execution.
First we need to add new type to the EFlowTraitType:
image_2024-05-14_16-13-11

then we need to define how this trait will work:
image_2024-05-14_16-14-52
image_2024-05-14_16-14-52 (2)
image_2024-05-14_16-14-52 (3)

and create and bind all corresponding UI actions

And after adding Log trait to some nodes and pins we get this result:
image_2024-05-14_16-14-54

Of course new trait will require some visual representation and organizing its actions in context menu, but I think it is not urgent right now.

TArray<FFlowDebugTrait> NodeTraits;

UPROPERTY()
TArray<FFlowDebugTrait> PinTraits;
Copy link
Contributor Author

@MaksymKapelianovych MaksymKapelianovych May 14, 2024

Choose a reason for hiding this comment

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

Store as TArray and not as TMap<FGuid, TArray< FFlowDebugTrait >> (where Key is Guid of Pin), because usually there will be very small amount of traits, and I think TArray would be better choice as all its elements are small and grouped in the same memory, so it is more cache-friendly.

static void ToggleTrait(const UEdGraphPin* OwnerPin, EFlowTraitType Type);

/** Sets the hit flag for the all OwnerNode's traits */
static TArray<EFlowTraitType> SetAllTraitsHit(const UFlowGraphNode* OwnerNode, bool bHit);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it would be better to return bitmask here? I doubt that in future there will be over 32/64 trait types

@MothDoctor
Copy link
Contributor

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

# Conflicts:
#	Source/FlowEditor/Private/Graph/FlowGraphEditor.cpp
#	Source/FlowEditor/Public/Graph/Widgets/SFlowGraphNode.h
@MaksymKapelianovych
Copy link
Contributor Author

Updated!

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

Successfully merging this pull request may close these issues.

2 participants