MothCocoon / FlowGraph

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

✨ Added support for Flow variables. #136

Closed ryanjon2040 closed 1 year ago

ryanjon2040 commented 1 year ago

Added support for reading and writing variables in Flow Nodes.

You can create a custom Flow Node and add a new Flow Property Bag variable. Then make sure to set Instance Editable to true. This will expose the pin to Flow Graph. image

Using any of the Set Node we can set the value of the property like this. Here I use Set String as an example. image

Then you can inherit from Flow Node Variable. image

You can now simply fetch the property using the member name you defined in the second screenshot. image

Here is the Flow Graph connection image

Supports Execution overrides. image

Flow Subsystem Variables exposes a lot of Get/Set nodes including UObject and UClasses. image

Alternatively, I've added a new node called Create Property. In the details panel, you can add as many properties as you want directly in Flow Graph. image

MothDoctor commented 1 year ago

Finally found time to look at this :) There's a lot of good work here around instanced structs here!

However, I'm afraid a lot of hard work is needed before it can be accepted to the base branch. I'm truly confused about what is designed here. First of all, there are nearly zero comments and it's difficult to understand it. Especially since we could have a different interpretation of what "flow variables" would mean. Which wouldn't be surprising ;)

In the screenshot, I see connections between variable pins on nodes. Awesome, but it does seem like connections aren't automatically resolved in runtime? There's

The way it's all implemented right now it looks more like Facts DB - a single subsystem for the entire game. Don't get me wrong, all this setup might be a nice way to implement advanced Facts DB (so many property types supported!). But it could be done without modifying the underlying Flow Graph plugin. Plus it would be safer to use, i.e. I used to have the Facts DB system have a gameplay tag as the entry key instead of a string. Facts DB concept is literally a simplified variable system using a globally unique subsystem, that can work with any gameplay-related node system ;)

As a user of the "actual variable system", I would expect that handling property operations are invisible to me. For instance, Flow Node should handle copying of variable value to Input property pin before calling ExecuteInput.

If you'd like to continue developing this as a variable mechanism similar to blueprints (or other graphs), I'd also suggest looking at these things

Yes, I know that it implementing it how I'd expect would take thousands of lines of code. And that might be a lot of work. That's why personally I'm not planning to work on it ;)

ryanjon2040 commented 1 year ago

Didn't got notification for this.

First of all, there are nearly zero comments and it's difficult to understand it. Especially since we could have a different interpretation of what "flow variables" would mean. Which wouldn't be surprising ;)

Sorry for the lack of comments. 😬

Some Flow Variables Subsystem doing Get/Set operations.

The reason why I added that get/set in subsystem is because Property Bag is not exposed to Blueprints, so I needed a way to retrieve the values passed via nodes.

_There are new node classes like UFlowNodeVariable and UFlowNodeCreateProperty, it's unclear to me what's their purpose.

Those are simply helper nodes that saves a few clicks. Nothing special.

Separated classes would be needed to Get/Set properties from Flow Asset itself, or external sources like a Facts DB (this is how blueprints do this, special nodes with FMemberReference). Sure, I see FFlowPropertyBag Property. Wonder if it's possible to "generate" this during the node's execution out of regular UPROPERTY? Without that properties can't be referenced inside nodes as other properties. We need to make properties work "natively" with UPROPERTY in any Flow Node class.

I'm sorry but I'm not sure I understand this completely. If you are comfortable, we can continue this discussion over at Discord.

As a user of the "actual variable system", I would expect that handling property operations are invisible to me. For instance, Flow Node should handle copying of variable value to Input property pin before calling ExecuteInput.

IIRC I think it does copy the variable to Input property. I'll have to look at the code.

What about supporting basic containers: array, sets, maps. Would it work here?

I've not looked into containers, but it should be possible.

_Cannot accept reparenting UFlowGraphSchema to UEdGraphSchemaK2. I understand why you decided to do that, it allows you to reuse a lot of code. But it already causes issues you pointed out "can't use Super" in many methods, need to hack it around.

If reparenting is not allowed, it can be a problem. 🤔

MothDoctor commented 1 year ago

Separated classes would be needed to Get/Set properties from Flow Asset itself, or external sources like a Facts DB (this is how blueprints do this, special nodes with FMemberReference). Sure, I see FFlowPropertyBag Property. Wonder if it's possible to "generate" this during the node's execution out of regular UPROPERTY? Without that properties can't be referenced inside nodes as other properties. We need to make properties work "natively" with UPROPERTY in any Flow Node class.

I'm sorry but I'm not sure I understand this completely. If you are comfortable, we can continue this discussion over at Discord.

Sure, we could discuss it someday. Probably it would be best to use voice chat, as it could be time-consuming to talk about advanced stuff in text chat.

_Cannot accept reparenting UFlowGraphSchema to UEdGraphSchemaK2. I understand why you decided to do that, it allows you to reuse a lot of code. But it already causes issues you pointed out "can't use Super" in many methods, need to hack it around.

If reparenting is not allowed, it can be a problem. 🤔

Yes, I know that it might be a lot of work to implement variables in a custom graph without including K2 classes. But this is necessary if we'd like to make it available for potentially hundreds of projects. Unreal Engine has a lot of different graphs supporting node properties, and very different implementations. It would incur technical debt if other graphs would use blueprint code ;)

MothDoctor commented 1 year ago

I am closing this down for tracking purposes because I haven't heard back in a few months. And there are many conflicts now.

@ryanjon2040 Feel free to re-open or create a new pull request, if you've decided to address issues raised above. Primarily, we cannot rely on K2Node classes here.