Process callbacks should not be disabled in State nodes

[accmltr]

Currently, in the State.gd script, the process callbacks for the script's node are disabled to implement the functionality of the State process events being signalled every process and physics frame:

    func _toggle_processing(active: bool):
    set_process(active and _has_connections(state_processing))
    set_physics_process(active and _has_connections(state_physics_processing))
    set_process_input(active and _has_connections(state_input))
    set_process_unhandled_input(active and _has_connections(state_unhandled_input))

This is not ideal, since this makes it very difficult to expand on the functionality of the State class AND its sub-classes in the future by adding new logic that needs to use the process callbacks. The same goes for the input callbacks which might be needed in the future, but are currently also being disabled.

Instead, local variables should be used to check whether the signal state_processing(delta:float) and signal state_physics_processing(delta:float) signals should be emitted:

var should_emit_physics_process_signal: bool = true
var should_emit_process_signal: bool = true
var should_handle_input: bool = true
var should_handle_unhandled_input: bool = true

func _toggle_processing(active: bool):
    should_emit_physics_process_signal = active and _has_connections(state_physics_processing)
    should_emit_process_signal = active and _has_connections(state_processing)
    should_handle_input = active and _has_connections(state_input)
    should_handle_unhandled_input = active and _has_connections(state_unhandled_input)

[uzkbwza]

i think this is not a good idea, because guarding like this in _process could have a major effect on performance if you have many nodes that have many states.

[accmltr]

I don't know what kind of impact it would have on performance, but I'm sure for most use cases it won't make a significant difference. It will be necessary down the line anyways, if the plugin grows in complexity.

[derkork]

Currently, in the State.gd script, the process callbacks for the script's node are disabled to implement the functionality of the State process events being signalled every process and physics frame

Actually the process callbacks are disabled to avoid that Godot has to pointlessly call process callbacks for nodes which are currently not active. On large state machines or state machines that have a lot of instances (e.g. units in an RTS or RPG) this can have a very noticable impact on performance. Its the reason the performance tests exist so I can see how the library performs with lots of agents. Before this change was made, the active flag of the state was used to determine whether to emit the signals (similar to what @accmltr suggested). Since the callbacks are now no longer active when the state is inactive, this check was no longer necessary either. So this change both simplified implementation and increased performance.

This is not ideal, since this makes it very difficult to expand on the functionality of the State class AND its sub-classes in the future by adding new logic that needs to use the process callbacks.

This may or may not be the case, depending on your stance on future-proofing things (more on that below) but the much more important point is: this library is not intended to be used this way.

The end-user should not derive own states from the built-in state classes but rather work with the provided signals. This way the library mainly stays out of the way and you have a lot less issues when internals of the library change. The performance improvement I mentioned was only possible to do without breaking everything because the library is designed in such a way that users only get callbacks and never implement state code of their own. Also making users implement their own states would break compatibility with C# users as they cannot derive from GDScript classes, which is also a reason why the library is written the way it is. Finally, by implementing own state classes you will now need to know how the library works internally and replicate this in your states instead of just connecting a few signals. And this would then bring up the question why bother with a library at all if you have to know and build all the details yourself.

It will be necessary down the line anyways, if the plugin grows in complexity.

I think there is no such thing as future-proofing in programming (and others think differently, which is also fine). We can talk about how any implementation detail may be a hindrance for adding a certain feature, once we know what that feature would be and what adding it entails. Then we can find a good design that solves the problem we now know well and also takes things like backwards compatibility and user-friendliness into account. But right now all I have is a vague statement that the way things are currently implemented may be a problem down the road without any evidence given to support this statement. On that basis, I find it hard to justify a change of implementation.

[accmltr]

@uzkbwza @derkork you guys are right, my bad. Didn't realise many process callbacks could have such an impact.