derkork / godot-statecharts

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

expression_guard.is_satisfied(): Cannot get path of node as it is not in a scene tree. #129

Open Jan-PieterInghels opened 1 week ago

Jan-PieterInghels commented 1 week ago

I'm encountering an issue where an error is thrown when an expression property is set on nodes which are freed.

This occurs in the following scenario:

I connect the following signal: area2D.body_exited()

func _on_area2D_body_exited(body):
    if body.is_in_group("player"):
        state_chart.set_expression_property('player_in_range', false)

The issue occurs when the expression property is attempted to be set on freed objects.

This happens when the node is freed and the signal is triggered as a result. The node can be freed by itself, or it can be freed by changing scenes. In both cases, when the signal is triggered, the following error message appears:

E 0:00:29:0310 expression_guard.gd:18 @ is_satisfied(): Cannot get path of node as it is not in a scene tree. <C++ Error> Condition "!is_inside_tree()" is true. Returning: NodePath() <C++ Source> scene\main\node.cpp:2084 @ Node::get_path()

expression_guard.gd:18 @ is_satisfied() transition.gd:74 @ evaluate_guard() state_chart_state.gd:271 @ _process_transitions() compound_state.gd:134 @ _process_transitions() compound_state.gd:126 @ _process_transitions() state_chart.gd:148 @ _run_changes() state_chart.gd:127 @ set_expression_property() monster_03.gd:151 @ _on_player_detector_body_exited() scene_manager.gd:31 @ _load_menu_scene()

Is this by design and should I work around this myself, or should this case be handled by the library itself?

Amazing tool by the way, it simplifies a lot of what I'm trying to do. Should come native with Godot imho!

derkork commented 1 week ago

In general the library tries to do not too much magic behind the scenes and liberally issues warnings when things are not the way they're supposed to be. I found this greatly helps debugging problems as state charts inherently have a lot of moving parts. In this particular case I'm not sure what the best course of action would be. The library could check whether the node is currently in the tree and just swallow the warning which would be useful in your case. On the other hand there are lots of cases where the developer can forget to add nodes to the tree after instantiating or dynamically adds and removes nodes to/from the tree. In these cases the warning would be really helpful to find out why the thing isn't working. So I am more inclined to keep the existing behavior to aid debugging. What are your thoughts about this?

Jan-PieterInghels commented 1 week ago

I agree with your reasoning, I'd prefer getting an explicit error thrown over implicit error handling behind the scenes, as long as there are valid use cases where the error communicates a mistake of the programmer. I haven't encountered those myself, but I assume the error is there for good reason.

That being said, in this example it's not a mistake and the functionality I'm implementing is being executed as I'd expect it to, meaning the error message is thrown, but it works. Therefore an error message feels a bit harsh to me. Maybe a warning is more appropriate? One example of this that I know of in native Godot is for Tweens. When you try to remove a tween in certain conditions, the following warning message might get thrown: Target object freed before starting, aborting Tweener.

Since I believe this is not a super niche use case, another suggestion could be to add an exported variable to every type of state which would be false by default, but if toggled true, 'error swallowing magic' happens. This way, error handling only happens implicitly. Maybe something like free_state_if_active or disable_node_path_error. The error / warning could then refer to it as well to clearly communicate the right course of action to get rid of it. What do you think?

derkork commented 4 days ago

Hi, sorry for the slow response, I am currently away from the computer for a few days and only have limited access to the internet. I would like to avoid configuration flags if I can do without them because they increase the complexity of the level of the library. I`ll try to come up with a way that “just works“.