derkork / godot-statecharts

A state charts extension for Godot 4
MIT License
679 stars 33 forks source link

Building a StateChart from code and triggering it to build his dependencies itself #87

Closed Shnyte closed 3 months ago

Shnyte commented 4 months ago

Hi,

I tried to build a StateChart from code, only 3 states, 3 transitions, a few buttons to test how it would react. Answer is "poorly".

By digging a little I understood that the states have to know the chart and the transitions, that there is a stateActive boolean to define as well for the active state, and that most of the initialization is performed in the _ready method of the various nodes.

I also tried to define the statechart in the editor, without any event for the transitions, but the chart starts to run immediately and I was not able to set anything up.

I tried with a dummy event string to prevent the chart from leaving the initial state, but as soon as I changed anything in the transition it went its merry way.

My goal was to try the use of enum and dictionary, to circumvent the use of strings as transition events, and build the state chart accordingly, assigning each transition the relevant event string from the dictionary. Those event string definitions would have been held in an autoload.

As a consequence, I wouldn't have to know any string in my code, but only define it once in the dictionary and access it from anywhere to make sure I use the right event value. Furthermore, anytime I would want to apply any redefinition of event names, I would only have to modify anything once, in the autoload, reload my scene and the chart would adapt seamlessly. Easy renaming. Well...that was the idea ^^

I think having a separate method to trigger the statechart to initialize itself once built, and then run, in that case would have helped me a long way.

Any plan on implementing such a mechanic to help my OCD ? Or maybe more to the point, something to help get rid of the strings ?

Thanks in advance for your answer to come

Shnyte

PS : maybe you might end up with the feeling that I'm mostly ranting, but I find your framework wonderful, it already helped me handle a lot, like inputs, characters' behaviour, animations and so on. It's already good as it is, it is simple to understand, easy to use, I just really don't like to have to know strings by heart to make everything work as intended.

Shnyte commented 4 months ago

OK never mind, I've been nasty. I've made an autoload, a new class extending the base Transition : image

and modified the call in EditorBar.

Then, in the editor, I just have to select one of the many values and it updates automatically with the string value defined in the dictionary : image

Since the setter is declared in the base Transition class, I just have to update the value and subsequent calls are made automatically as well image

No warning in the editor...Works like a charm !

Downside is I have to invoke the autoload from the class extending Transition, and I don't like it, but I'm not sure how to pass a global value to a node through the inspector.

Any idea ?

Love this framework !

derkork commented 4 months ago

Thanks a lot for taking the time to write this up! It's really nice to see what solutions other people come up with as I certainly don't have the answer for everything. I think to make this work out of the box we'll have to stick to strings, otherwise people would first need to write some dictionary with event names. While I am also more in the camp of "I like to have some constants for my events", strings are just fine for a lot of people and work out of the box. So I certainly want to keep this simple workflow. So where to go from here?

One thing I could think of is reversing your approach. E.g. we already have functionality in the editor that allows us to pick events used in a state chart and also to rename them across all transitions of the chart (which I think would not work if you just rename the enum key, because nobody will fix up your existing transitions then). If we leverage what we already have, we could add some source code generator which could write/update a GDScript file with constants that can be used from code. For example if we add a transition like this:

image

Then we could generate a GDScript file like:

class_name MyStateChartEvents  # this would be configurable

const KEY_TAB:String = "key_tab"  # the constant name is generated from the value

If we add more events, this file would be extended. If we rename or remove events this file would be kept up to date. This way your code can reference these constants and if you change/remove an event, the compiler would now point out places where you still use old events instead of your game doing undefined things at runtime. The upside is that this would require no breaking changes and could be optionally enabled if you want it - while providing an additional benefit over the status quo. But of course this also has some interesting edge cases, that we somehow need to figure out like:

So what do you think and do you maybe have an idea about these edge cases?

Shnyte commented 4 months ago

Hi

and thank you for your fast response.

My solution

Yeah, I believe that this "solution" of mine is mostly adapted to what I'm used to and to how I imagine interacting with the framework.

Decorating transitions

That said, that kind of solution could be proposed as an option inside the framework as in "you have transitions you handle with string events, and other transitions you handle with signals or constant value or even something else". Maybe adding one or two new Transition-extending classes could be of some use for more experienced and demanding users.

Decorating events

Or it could be handled by decorating events themselves : I'm thinking about setting up events as you did with guards, which is a neat solution. String events would be the basic one. Signal events could react to user-declared signals. Constant events could access a user declared enum passed in the inspector.

Here is a quick overview of what I have in mind when I'm talking about decorating events :

EnumEvent

StateChart_EnumEvent_Sequence_01

SignalEvent

StateChart_SignalEvent_Sequence_01

In both cases, configuration doesn't seem trivial to me. Since I don't yet know what GDScript's introspection in Godot allows or not, I admit that it might as well be wishful thinking at best. I will have to try it out.

Keeping track of the declared events

Now regarding what you suggested, if I understand correctly it is about keeping track of the various events declared in the transition objects through inspector and being able to call those events from code without using the string value.

My first thought is that, if you don't want to go all the way towards an event manager (such as the built-in Group Manager) to do exactly that, it could be a good intermediary solution, in addition to the fact that renaming events is already implemented.

But intermediary only nonetheless : with that feature, we would have the ability to call events from code once declared, but we would still have to declare them as strings in the inspector.

In my opinion, what you suggest would work best for events used once in a tree, thus eliminating the risk of typo in the inspector since it would only be declared for one transition. But in the end, if an event is to be used for another transition in the chart, or even shared between charts, I think it should be set in the inspector from those constants, and not the other way around.

Calling the events from code is a good thing, declaring them typo-risk-free looks even better.

Regarding the edge cases, please let me proceed to additional tests to understand how Godot environment works regarding reaching out to project-wide-scoped classes, pushing errors and so on before I suggest anything (I'm still new to it, and I don't know much about all that ; in addition during my tests yesterday, I realized Godot pushed a few intresting errors regarding the declaration of my autoload).

Shnyte

derkork commented 4 months ago

Thanks a lot for writing this up! There some interesting ideas there, especially things like the SignalEvent would nicely address some real-world use-cases we have here (e.g. waiting on animations). Then again, this would significantly increase configuration complexity plus we really don't have any introspection features in GDScript so things like a ConstantEvent or SignalEvent would again rely on strings for the constant or signal name as there is no easy way to get them from the source code (unless we want to write our own GDScript parser). Also signals can send any amount of parameters and to build a receiver function for them, we would need to know how many parameters (which we can't get easily). So we'd just trade configuration with strings for more complex configuration with strings in the end (ignoring the unsolved signal receiver problem).

Shnyte commented 4 months ago

Hi,

so I took my sweet time, but I've got a working solution.

Design

As discussed above, my goal was to provide flexibility to the end user of the framework by allowing to choose between various kinds of events to trigger a state change.

My original idea was to base the behaviour on inheritance, to pass the triggers through the state chart and have each state pass it to its transitions' events and let each react to the trigger (or not) by itself.

Class diagram : Event_class_diagramç01

where Keys is a nested enum, hosted in the Events class

Note that I had to make everything a node and to add them to the sceneTree, in order to be able to connect to the react_to_signal() method of the SignalEvent.

I settled on passing an event directly rather than creating one more inheritance tree out of an original Trigger class.

Implementation

Handling events

I created a "superclass" named Event with a default method evaluate (event: Event) -> bool:. As there is no abstract class yet (but devs are working on it), I implemented it in the superclass with a push error.

Then I went on to create a StringEvent extends Event class, which, as its name suggests, would react to an event based on a StringName. The main goal of this implementation would have been to carry the simplicity of the StringName usage with the framework (as you stated previously, and I still agree about this).

In order to test that, I needed to replace StringName by Event in the whole chain of calls from the StateChart to the leaves.

Me : "So be it, let's do that..." Framework : "There are StringNames everywhere btw" Me : "Huh ok, let's pass an object for tests' sake then..." GoDot : " StringNames and Objects are incompatible btw" Me : "Oo OK then, let's change it everywhere I guess..."

So that's done...

Helper methods in the StateChart script

I created helper methods send_string_event (event: StringName) and send_enum_event (event: Events.Keys) since I cannot overload the original send_event (event: Event...) method of the StateChart (which I would rather do, but hey...maybe one day).

image

There, the call creates the appropriate temporary tmp event in each method and stores the trigger in it, then it passes the event along through send_event (tmp)

Then the event is passed to the child state, up until the transition. There, the event is tested against the event stored in the Transition. If the stored event recognizes the trigger, evaluate (event: Event) returns true and transition is processed.

Global, not autoload, enum

I created the Events.Keys enum that I populated with a default value and test values.

The default value is there to provide an initial value to the trigger in the inspector to prevent unwanted behavior if one adds an EnumEvent to the StateChart tree without setting its value (provided the default value is not used anywhere as a trigger -> I added a test and a warning for that case).

Constructing the chart

In order to create the state chart, we now have to add an event to each Transition node. Adding the event as a child automatically passes it as value to the Transition so that it can keep a reference to its event (and test whether an event should trigger it or not).

Editor side bar

I modified the editor_sidebar script in order to be able to create each of the implemented Event type nodes : image

Note that, as a temporary measure, all the events have the same icon.

I modified the editor_sidebar scene in order to have 3 more buttons (one for each of the concrete Event implementations). Those buttons are part of the eventbutton group so that they can be made visible when a Transition is selected in the chart.

I had fun and adding various tests and warnings to the Events themselves, in order to spot the mistakes during state chart construction : image

StringEvent

They need to be set a String value in the inspector : image

EnumEvent

They know about the Events.Keys enum because it is embarked in the framework : image

image

Enum NodePath could be passed as a parameter in the inspector for these, but since you suggested to handle the StringNames given as events in the inspector by adding them to an enum, I thought it would be consistent with this suggestion of yours to already have said enum in the framework anyways.

Note that StateChart helper method is based upon the knowledge of this enum : image

SignalEvent

They need the StateChart to be passed as a parameter through the inspector so that it can call StateChart send_event (event: Event) method, passing itself as the event : image

This setting will allow the event to recognize itself when tested and to trigger the parent Transition to happen.

Apart from that, they need to be connected to the signal that will trigger them : image

image

image

Tests

I created a dummy state chart with 3 states, and 3 buttons alongside in order to test the transition triggers : image

2 buttons are connected to a method sending an event to the StateChart, either through send_string_event (event: StringName) or send_enum_event (event: Events.Keys). image

The third button has its pressed signal connected to the SignalEvent : image

The trap button sends a StringName to the StateChart. It is there to demonstrate that, since the event it is trying to trigger is a SignalEvent, it won't trigger anything.

Running the test scene

Initial state is State1 : image

Clicking the "To State2" button sends a String as trigger to the StateChart and transition is triggered : image

Clicking the "To State3" button sends an enum value as trigger to the StateChart and transition is triggered : image

CLicking the "To State1" button sends the pressed() signal to the SignalEvent, which in turn sends itself as event to the StateChart and triggers the Transition : image

Clicking the trap button doesn't do anything (it shouldn't, but it doesn't even generate an error, and the StateChart handles it quite well, GG BTW) : image

Conclusion

I'm absolutely convinced that it can be done in a quite more elegant fashion, but as a proof of concept, it works fine.

To improve upon all this :

derkork commented 4 months ago

Wow, that's a lot of work that went into this one! I'm not quite sure I understand which problem we're trying to solve here anymore, though. If it was to eliminate strings as source of error, I feel we haven't really made a lot of progress on this right now:

So I really don't think that this will significantly improve the experience plus it introduces a lot more surface area for making new kinds of mistakes. It goes in the same direction as the AnimationTreeState and AnimationPlayerState nodes which I think were a big mistake to add on my part. They make a single thing easier, but fall apart if anyone wants to do a slightly different thing.

So I'd rather keep the the library simple and give the end-users more flexibility by using what Godot offers in way that suits their project rather than predefining, how events can reach the state chart. So if someone wants constants for their events, they can write themselves some constants. If someone wants to react to certain signals, they can do so in a way that works for them and that is easy to setup using the techniques already present in the engine. And if someone wants to wrap a whole framework around this, well they can also do it in a way that makes sense for their project.