GraphNode rework

[atomic-junky]

All GraphNodes are hardcoded and all their fields are implemented by hand, even when it's for very simple things.

The problem is that the slightest change can very easily break everything, and is a pain in the ass every time. The aim would therefore be to make the GraphNode code as simple as possible and put everything in the MonologueGraphNode class.

Currently, the way these changes have been made is that control nodes with the prefix "Field" automatically become fields, which will be registered and monitored automatically. The value of these nodes is automatically associated with a key in the name following the "Field" prefix.

For example : A LineEdit node with the name "Field_Sentence" will be linked to the "Sentence" field, and when loaded will take the value of the "Sentence" key. Each time the LineEdit changes, the value of "Sentence" will be updated. There's no need to specify that the "Sentence" key exists, as it will be created automatically.

[atomic-junky]

These changes are available in feature/graph-node-rework

[RailKill]

https://github.com/atomic-junky/Monologue/blob/c78399509d464d3ea65a32253d8f65c2eaf3fc0f/Scripts/MonologueNodePanel.gd#L31-L60

Polymorphism

I don't intend to be dismissive but this Field_ type-checking design is considered an anti-pattern and will cause similar problems as before. We want to design in a way where MonologueNodePanel is able to generate fields without type-checking, i.e. polymorphism. We can do so by moving the special handling of each case into each individual field's class instead.

Design Suggestion

Instead of using "Field_" prefix for nodes, I think a simpler, more direct solution would just be to create a common get_fields() method in MonologueGraphNode. Each graph node's get_fields() will return a Dictionary containing graph property name as key, and its field PackedScene as value like so:

# NodeSentence.gd

func get_fields():
    return {
        "sentence": preload("res://Objects/SubComponents/MonologueLineEdit.tscn"),
        "speaker_id": preload("res://Objects/SubComponents/MonologueOptionButton.tscn"),
        "voiceline_path": preload("res://Objects/SubComponents/FilePickerLineEdit.tscn"),
        ...
    }

    # store the PackedScene in superclass const so we don't have to write preload() many times

Then the MonologueNodePanel can read and generate fields as such:

# MonologueNodePanel.gd

func load_fields(node: MonologueGraphNode):
    var fields = node.get_fields()
    for property in fields.keys():
        var field = fields.get(property).instantiate()
        field.property = property
        field.panel = self
        field.text = graph_node.get(property, "")

        # if you need more metadata like dict_text, label text, add more stuff in the fields dictionary
        # or create an object to properly store them like how I did with PropertyChange.gd
        field.label.text = property.capitalize()

        add_child(field)

Each field can connect signals in their scene and handle things respectively as usual (especially for ChoiceNode's OptionContainer), but I've written this in _ready() to show an example of how the fields will connect signals to call its panel to propagate the changes to the graph node:

# MonologueField.gd
# I wrote this code using inheritance pattern, but it is actually better to use composition pattern
# Composition allows FilePickerLineEdit, MonologueLineEdit, MonologueTextEdit to all use it

var property: String
var panel: MonologueNodePanel

func _ready():
    connect("_on_text_submitted", commit_change)
    connect("_on_focus_exited", commit_change)

func commit_change(new_text = text):
    panel._on_node_property_changed([property], [new_text])

Doing it this way, we can remove all specific node panels like SentenceNodePanel, RootNodePanel, etc. as the field definitions are all in the graph node (e.g. SentenceNode), and the UI update logic are all in the field's scene (e.g. FilePickerLineEdit.tscn). MonologueNodePanel will just be the fields loader, a.k.a the middle man. Hope this gives you some ideas on how to go about this. Good luck!

[atomic-junky]

I like this idea, the prefix idea didn't seem very good to me either and it was mainly to test automation. I think I'll do something like this, I really like the fact of removing the NodePanel

[atomic-junky]

Here are the three ways I thought of to retrieve the fields. Let me know if there are others, or if you think one is better than another or should be modified. I don't know which is the best practice

Simple enum type

func get_fields():
    return {
        "Sentence": MonologueField.LINE_EDIT,
        "SpeakerID": MonologueField.OPTION_BUTTON,
        "DisplaySpeakerName": MonologueField.SUB_LINE_EDIT,
        ...
    }

MonologueField class

func get_fields():
        var speaker_field = MonologueField(Field.OPTION_BUTTON, "Speaker")

    return {
        "Sentence":  MonologueField(Field.LINE_EDIT, "Sentence"),
        "SpeakerID": speaker_field,
        "DisplaySpeakerName": MonologueField(Field.LINE_EDIT, "Display Name", speaker_field),
        ...
    }

# MonlogueField(field_type, field_label, parent, ...)

Global constants

func get_fields():
    return {
        "Sentence": MonologueLineEdit,
        "SpeakerID": MonologueOptionButton,
        "DisplaySpeakerName": MonologueSubLineEdit,
        ...
    }

[RailKill]

Personally I like the third option with the class_name. If you use enums, you're going to be matching enums and do special handling for each enum so it comes back to the same problem. How about using the third option with _init() function in those classes so it can accept the dict ID and property value on instantiation as such?

# NodeSentence.gd
func get_fields():
    return {
        "sentence": MonologueLineEdit.new("Sentence", sentence),
        "speaker_id": MonologueOptionButton.new("SpeakerID", speaker_id),
        "display_speaker_name": MonologueSubLineEdit.new("DisplaySpeakerName", display_speaker_name),
    ...
    }

This way, MonologueLineEdit, MonologueOptionButton, MonologueSubLineEdit can all have separate implementation in their own _init() to assign the values, because for LineEdit, it's text, but OptionButton is selected_index. We might not even need MonologueField? Good to consider.

[RailKill]

Word of caution, the class_name refers to the .gd script, not the .tscn. So maybe instead of passing values through _init() constructor, you may want to consider just setting values of the instantiated scene, for example:

func get_fields():
    return {
        "sentence": LINE_EDIT.instantiate().set_id("Sentence").set_value(sentence)
        "speaker_id": OPTION_BUTTON.instantiate().set_id("SpeakerID").set_value(speaker_id)
        ...
    }

It looks a little uglier, but you have the best control with .tscn PackedScenes instead of just the .gd script. Those LINE_EDIT constants need to be defined in the MonologueGraphNode.gd superclass:

const LINE_EDIT = preload("res://Objects/SubComponents/MonologueLineEdit.tscn")
const OPTION_BUTTON= preload("res://Objects/SubComponents/MonologueOptionButton.tscn")
...

and MonologueLineEdit, MonologueOptionButton, etc. need to inherit from the same class to have set_id() and set_value() as common method signatures, but each have their own implementation, let's say MonologueField.gd as the superclass:

func set_id(new_id) -> MonologueField
    # to be overidden and implemented in respective field type
    return self

func set_value(new_value) -> MonologueField
    # to be overidden and implemented in respective field type
    return self

Ultimately it's up to you when you implement it to see what works best.

[atomic-junky]

Hi, I'm making progress on reworking the graph nodes. But I find it quite laborious to implement. Since Godot doesn't support multiple inheritance, the nodes of each field must be children of a MonologueField node. On top of that, you have to create a lot of classes for each different MonolgoueField, which I find to be a useless mess.

So, trying to find a simpler solution, I thought we could simply use the variables defined in the GraphNode script to generate the NodePanel. For example, we could use the variables defined in a GraphNode script to generate the NodePanel:

# MonologueGraphNode.gd
class_name MonologueGraphNode extends GraphNode

var id: String = UUID.v4()
var _node_type: String = "NodeUnknown"

...

# NodeSentence.gd
class_name SentenceNode extends MonologueGraphNode

var speaker: MonologueEnum = "" # Not sure for this one
var sentence: String = ""
var voiceline_path: MonologuePath = ""
var foo: float = 1.0

# Not sure for this one
# Called when the field foo is loaded
func _load_foo():
  ... 

# Or
func load_fields() -> Dictionary:
  return {"foo": 3.0}

# MonologueNodePanel.gd
class_name MonologueNodePanel extends VBoxContainer

...

# It can be put in MonologueGraphNode
func _load_fields(graph_node: MonologueGraphNode):
  var properties: Array[Dictionary] = graph_node.get_property_list()
  var fields: Array[Control] = []

  for property in properties:
    if property.begins_with("_"):
      continue

    var name: String = property.name
    name = name.replace("_", " ")
    name = name.capitalize()

    var field_node: Control

    # Create and connect field nodes
    match property.type:
      TYPE_BOOL:
        ...
      TYPE_STRING:
        field_node = LineEdit.new()
        ...   

I don't think you're going to like this proposal, but I personally find that it allows you to group all possible cases in the same place without too many lines of code, and its operation is fairly intuitive, in line with the way Gdscript works. If a variable starts with “_”, it's hidden, otherwise it's considered a public variable and can therefore be modified in field. It also lets you quickly create and modify GraphNodes, simply by adding a variable of the right type to the GraphNode.

It's not impossible that this proposal is an aberration, but I admit that programming is far from being my profession. This proposal may also be an anti-pattern, but I don't really understand what it is. I have the impression that there will be fewer errors if we do it this way, but I'm probably wrong.

I'll leave it to you to decide whether it's a good idea or not. By the way, if you want, you can make the change yourself.

[RailKill]

Hmm you're right that I do feel it's kinda janky but I think your idea has some merits. Gimme some time to mess around with the implementation, I'm thinking about how to go about creating the controls for something like ActionNode, and TYPE_STRING is not going to suffice because it can be LineEdit, TextEdit or even FilePickerLineEdit, so there's some complication there. Lemme try a few things with your idea, I'll get back to you.

[RailKill]

Ok, I've given it a try, you can view it in https://github.com/atomic-junky/Monologue/tree/wip/node-rework

Just the property name and type alone is insufficient to define how the UI controls should work, especially when dealing with controls with more moving parts like in ActionNode, there is variable dropdown that displays different controls depending on the variable type. Little quirks like volume slider has "db" suffix but pitch slider does not. String types can be LineEdit, TextEdit or FilePickerLineEdit. So we still need something like MonologueField to define what GUI should represent that property, and how that GUI should behave.

I've implemented the best case (I think) for NodeSentence.gd in the wip/node-rework branch. _from_dict() is completely managed by the parent class MonologueGraphNode.gd and _to_dict() is collected from MonologueFields. Be careful not to save .json files using this branch because only NodeSentence works correctly right now.

The problematic ones are ActionNode and ChoiceNode. I'm working on ActionNode now, it's halfway done, you can play around with it. With MonologueField, I believe even ActionNode's GUI controls can be implemented, but I'd like your feedback on the foundation of this design, see if we can make it more simple. I think nodes like ActionNode are the ones that makes it difficult to simplify.

[atomic-junky]

It seems to be working properly, well done!

I'm wondering if we shouldn't rethink how the ActionNode works. I think the ActioNode is a bit of a mess. Maybe we could separate the different types of actions into different nodes. Besides, I don't think ActionCustom serves any purpose any more. And I don't know if the ActionTimer really has an interest, I can't see in what situation it serves a need but if I'm wrong, a timer node would make more sense I think and might be more logical.

[RailKill]

Alright, I think I can make it close to, or even simpler than your idea if I'm allowed to change the structure and break backwards compatibility. Maybe we just add some extra logic to load v2.x files if needed, but it'll be much better I think. Gimme some time, I come up with another implementation.

Edit: Concept already pushed to wip/node-rework branch. NodeSentence even cleaner than before, and MonologueNodePanel completely redundant now.