derkork / godot-statecharts

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

Bug: It does not work if it is a duplicate object and an ExpressionGuard was used in the original #120

Closed alextkd2003 closed 1 month ago

alextkd2003 commented 1 month ago

Steps: Duplicate an object that has a Expression Guard in any of the transitions.

Result: The StateChart of the duplicate object will not work correctly. Errors will be displayed in the terminal

Errors:

E 0:00:02:0574 expression_guard.gd:35 @ is_satisfied(): self can't be used because instance is null (not passed) <C++ Error> Condition "p_show_error" is true. Returning: Variant() <C++ Source> core/math/expression.cpp:1507 @ execute()

expression_guard.gd:35 @ is_satisfied() transition.gd:57 @ evaluate_guard() state_chart_state.gd:112 @ _state_enter() compound_state.gd:65 @ _state_enter() parallel_state.gd:68 @ _state_enter() E 0:00:02:0574 expression_guard.gd:37 @ is_satisfied(): Expression execute error: self can't be used because instance is null (not passed) for expression: stamina <= 0 core/variant/variant_utility.cpp:1091 @ push_error() expression_guard.gd:37 @ is_satisfied() transition.gd:57 @ evaluate_guard() state_chart_state.gd:112 @ _state_enter() compound_state.gd:65 @ _state_enter() parallel_state.gd:68 @ _state_enter() ![image](https://github.com/derkork/godot-statecharts/assets/50635357/c58082ae-1dbd-4d71-9eaa-fa340e7a2662)
derkork commented 1 month ago

This probably happens because you don't set expression properties for the copy. Every copy has its own expression property map and the error message you get means that the expression was trying to read a property that was not set. Try calling set_expression_property on the duplicate in a _ready function, this should fix the problem.

alextkd2003 commented 1 month ago

@derkork will try, but it's a little confusing to see that I can use the editor to set the expression and if I do that I can't create copies because they have to be set using the _ready function.

derkork commented 1 month ago

I might not have fully understood the problem. The library does not provide any way to set expression properties via the editor. They always need to be set via code. If you have an expression stamina < 10 in a guard but never set the stamina expression property through your code (e.g. state_chart.set_expression_property("stamina", 20)) then you'll get the error message you got, because the guard has no data to make a decision from. And this will also happen if you don't copy the state chart.

If I'm on the wrong track here, could you please attach a minimal example scene/project that shows the problem, so I can have a look at it?

alextkd2003 commented 1 month ago

:)I mean the inspector, not editor (I will provide a demo later today :) )

image

image

I was using the set_expression_property but not in the _ready state function.

derkork commented 1 month ago

Well the important part is that the necessary expression properties must be set before an expression guard using these properties is evaluated. Otherwise the guard has nothing to work with. This can be done in a _ready function or in another function, like you did in your example. Duplicating a state chart doesn't change this requirement. If you didn't set the properties on your duplicate, then the expression guards there will not work. Also note that there is a difference between setting expressions (which you do in the inspector) and setting expression properties (which you do with set_expression_property). I may need to clarify this in the documentation a bit better.

alextkd2003 commented 1 month ago

I'm sorry I haven't had the time to make the demo yet. I'll put the steps here.

1 -> Create NPC and assign a Guard. 2 -> Press Ctrl + D to duplicate this NPC 3 -> The new NPC does not have the Guard assigned. The only way to see the assigned Guard is to first save it as a resource. 4 -> Go back delete the copy and save the original NPC's Guard as a resource. 5 -> Press Ctrl + D. The new NPC has the Guard 6 -> The same error is shown and it doesn't work.

Let's go back to your comment "Duplicating a state chart doesn't change this requirement"

In my example I have an EnemySpawner component, the spawner instantiates the Original NPC and yes the copies displays everything and the Guard in each copy. I do not want to create manually Guards. The EnemySpawner creates the duplicate NPC for me. For what you say this is a limitation.

Likewise, I just continue using send_event and everything works like a charm :).

derkork commented 1 month ago

I'm sorry, I cannot reproduce this with the instructions given (e.g. what is meant by "create an npc", do we create a new scene or just a bunch of nodes in an existing scene, what is meant by duplicating the NPC, e.g. do you duplicate the scene file, do you duplicate nodes in the NPC scene or do you duplicate an instance of the iNPC scene in another scene?).

I tried to build something that hopefully resembles your setup. We have a spawner scene with spawner code like this:

image

So it spawns an NPC, adds it to the tree and after 2 seconds it calls the trigger function on the NPC. Then we have the NPC scene:

image

NPC starts out in state A and uses a transition to B guarded by an expression guard. When the trigger function is called, it sends the trigger event and the NPC changes from state A to B.

Running this prints:

spawning an npc
/root/spawner/NPC entered a 
triggering new npc
/root/spawner/NPC entered b 
spawning an npc
/root/spawner/@NPC@2 entered a 
triggering new npc
/root/spawner/@NPC@2 entered b 
spawning an npc
/root/spawner/@NPC@3 entered a 
triggering new npc
/root/spawner/@NPC@3 entered b

So the scene is instatiated 3 times and the guard runs just fine for each copy without any manual intervention needed. Which means in general, this is supposed to work just fine. Now we just have to find out how your setup differs from mine to find the cause of the error.

alextkd2003 commented 1 month ago

I believe is related to were I'm using the set_expression_property, because the script run in process and I'm using this methop in physics could go out the active state and found a null expression.

Captura de pantalla 2024-05-23 152118

I believe it is related to where I'm using the set_expression_property. Because the script runs in the process function and I'm using this method in the physics function, it could go out of the active state and encounter a null expression.

TestStateChart.zip

derkork commented 1 month ago

Okay, as I suspected the messages are caused because you use an expression property in a guard expression without initializing it first. When the state chart starts up the following steps will happen:

image

  1. The root state is entered.
  2. Chill is entered because it is set as initial state of the root state.
  3. the _toangry transition is evaluated because Chill was entered.
  4. the expression guard tries to resolve dist_to_player which is not set - so you get the error message.

Later when the player moves near the enemy and the enemy enters angry state a similar thing happens. The _tofollow transition evaluates the time_angry property which was not set yet, hence you get the error message.

This would happen either way, no matter if you copy/instantiate the state chart or not. A guard simply cannot evaluate an expression when the properties used in the expression are not set. A very simple fix for this, is to initialize all properties to a sane default value in your _ready method. E.g. in enemy.gd, you already have an empty _ready method, so if you add something like this:

# Called when the node enters the scene tree for the first time.
func _ready() -> void:
    state_chart.set_expression_property('dist_to_player', pow(10, 6)) # a million pixels away seems to be a safe default
    state_chart.set_expression_property('time_angry', 0)

    pass # Replace with function body.

then the error will go away because now all properties are initialized and the expressions can be evaluated.


Unrelated to this, you can also used timed transitions to eliminate the need for the time_angry expression property. Simply make the _tofollow transition delayed by 2 seconds. Then when the player stays near the enemy it will automatically transition to Follow after 2 seconds and if the player moves away from the enemy this transition automatically gets aborted. Then you also don't need to track the time when you entered the state:

image

https://github.com/derkork/godot-statecharts/assets/327257/591dbebc-0266-4f4b-bf03-7f202d902ff8

alextkd2003 commented 1 month ago

hey @derkork it works wonderfully. I didn't understand why I had to initialize the expression in the ready, but with your explanation everything is clear. Maybe it is good to emphasize in the documentation that expressions must always be initialized in the _ready method, or maybe it is there, and I didn't see it. Likewise, thank you for being so attentive. Case close 😅