derkork / godot-statecharts

A state charts extension for Godot 4
MIT License
734 stars 35 forks source link

Add Transition Properties: CancelEvent, CancelEventGuard, CancelCondition and EndGuard #43

Closed accmltr closed 10 months ago

accmltr commented 10 months ago

Hello there,

I think is necessary to add a way to cancel transitions that should not be continued.

Here is an example scenario: You have a sleeping enemy in your game, you want them to wake up 3 seconds after the player entered their detection radius. All is well when the player approaches them, the enemy transitions from a sleeping state to an active state after 3 seconds. However, if the player leaves the detection radius before the 3 seconds elapse, you would like the enemy to remain in a sleeping state.

Currently, to solve this problem you can create another transition on the enemy that instantly transitions back to its own sleeping state when the player exits the detection radius. But this is a hack, and is not ideal. Instead, there should be a way to add a check that runs every physics frame to see if the transition should be continued. Also, there should be a way to cancel the transition based on certain events that happen, and that event listener should also be able to have a guard property.

Locally I have already edited transitions in my project so they have cancel events and conditions, and they work nicely and make state management a lot easier: image

I think these would add substantial value to the plugin.

Kind regards

accmltr commented 10 months ago

It seems checking for a cancel condition each frame would be unnecessarily expensive on performance. Instead, a "Cancel Guard" should be more than sufficient, in the same way that the existing guard works well enough.

The "Cancel Guard" should only be checked when the "Cancel Event" happens.

derkork commented 10 months ago

I'm failing to see how adding a second transition is a "hack":

This is about as straightforward an implementation as I can think of. Specifically on the concept of a "cancel event/cancel guard", this is basically just cramming the two transitions into one. Instead of adding a second transition we now have two more fields which basically replicate the second transition. But these new fields have absolutely no meaning for transitions that are not timed, so they will confuse users and they internally add complexity to the implementation of the transition handling.

I feel this is just adding complexity for no real benefit unless there is a use case I cannot see right now which cannot be handled by the current system. If such a case exists, I'd like to know about it.

accmltr commented 10 months ago

It's a hack, since it is a substitute for something that should be a standalone feature: "Transition Cancelling".

Also, transitioning back to your own state with the hope of the plugin resetting all current transitions is a bad approach to solve a problem like this. It's also hard to figure out and makes it difficult to understand what is happening in your state machine.

accmltr commented 10 months ago

Also, if you want to cancel a specific transition out of multiple you can't with this hack. But if each transition has the option to be cancelled by an event, then each can have it's own condition for being cancelled.

accmltr commented 10 months ago

It would make working on AI in my project 10 times slower if I didn't add the feature locally.

derkork commented 10 months ago

This discussion shows me that the manual is somewhat misleading. Actually there can only be ONE transition active at any time per state. The first matching transition is picked and if it happens to be a delayed transaction it is marked as "pending" transition and will be executed later. The manual somewhat misleadingly states that:

If a transition has a time delay, it will be executed after the time delay has elapsed but only if the state to which the transition belongs is still active and was not left temporarily. Once a state is left, all transitions which were queued for execution will be discarded.

This implies that multiple transactions can be active at the same time for the same state which is not correct. However it also states that when you leave a state for any reason then any pending transition is cancelled (which is correct). As such relying on this behaviour is no hack - it is using documented functionality. I will update the manual as this is clearly wrong/confusing. This should also address the comments on cancelling one of multiple active transitions which cannot actually happen since there is only ever one transition active at any given time for any given state.

It would make working on AI in my project 10 times slower if I didn't add the feature locally.

It would help me a lot to understand your use case here and how the current implementation would slow it down dramatically. Could you give me a bit more input on that?

tmilker commented 7 months ago

@derkork I'd like some clarification on whether this is a bug or intentional and the correct solution so bear with me while I talk things out. You can actually see the behavior in question in the video documentation link you provide as well at around 20:11(as noted by commenters) but it's a bit complicated at that point. I'll try to describe a simpler scenario that the video sets up. You can recreate the scenario below by following the video until 10:52 and then setting the Delay Seconds on the To Idle transition to 3.

Scenario: You have a watcher that starts watching you(Observing state in the video) as the sprite attached to the mouse cursor enters it's Area2D and stops(Idle state) when it exits. This works fine with no delay. However, suppose you want to delay the watcher 3 seconds before it stops watching and then transition back to the Idle state. So you set the Delay Seconds on the To Idle transition to 3. This works fine if you move the cursor into the area, then back out and wait 3 seconds, the enemy's state goes back to Idle.

However, if you move back into the area and trigger another event, State Charts doesn't consider this a state change(because, of course, no state has changed), so it continues counting down the transition and then transitions back to Idle, even though you are within the detection area. So there's no way to "cancel" the transition. I understand that State Charts is supposed to be a black box to avoid state dependency so I get why there shouldn't be an explicit "cancel transition" command, I'd just like to know the right way to solve it.

I think this would be solved by just transitioning immediately to another state that then transitions to Idle after 3 seconds. Is this correct or is there a better way? This seems to be the thing that the OP of this issue was opposed to.

Thinking about it, as it currently functions seems correct to me and the OP was trying to treat the delayed transition as a state itself which I can imagine could get you into trouble in the future if the situation got more complex.

However, I can also see the confusion that if you're in a state with a transition delay and you try to send the event to go to that state(aborting or whatever) while the transition is being delayed, the delayed transition isn't canceled because you haven't actually left that state which doesn't meet your internal criteria for canceling the delay. This requires that you have additional states just to be able to abort, which I could see being frustrating for some people such as the OP. In the debugger, I do see that the event occurs, so State Charts could recognize this situation and stop any delayed transition if an event asks to go to the state it's already in.

derkork commented 7 months ago

The solution is really quite simple. Lets say we start with this setup:

image

Now we send an "Enemy enters" event, which will trigger the transition to "Watching":

image

Next we send an "Enemy leaves" event. This will trigger the transition to "Idle" but since it is delayed, we still stay in "Watching" while the transition is pending.

image

Now we send another "Enemy enters" event. Since the currently active state is "Watching" and it has no transition defined for this event, nothing will happen. We simply have not given the state chart any instruction on what to do when an enemy enters while we are in watching state:

image

This is a problem with our specification. We can fix this, by adding a rule for this case:

image

Now, we have a rule so if we send an "Enemy enters" event, the "Watching" state is currently active, has a rule and will transition to itself. This will temporarily leave and re-enter the state and in turn cancel any pending transition, so after that we are at this:

image

So we fixed the problem by adding the missing rule for this case, which is what I also suggested in the video comments (but adding a few pictures really helps explaining stuff). I hope this is helpful. Please let me know if you have additional questions or thoughts on this.

accmltr commented 7 months ago

Sorry for not replying, I got angry and left. I was wrong, you can do without the feature I was asking for. I just think it could be worthwhile to undergo multiple transitions simultaneously and cancel them individually if their initial cause were gone by the time they should complete.

It just felt more natural to think of a state with many transitions as being like an organism with many sensory organs around it at the same time, only deciding to react on one's feedback when it crosses a threshold (including a time threshold). My problem was just that, when making delayed transitions you had to worry about cancelling other transitions that were underway. But since you can't have multiple delayed transitions anyways, it doesn't matter I think.

I understand the way you intend for it to work though. All the best with the plugin.

tmilker commented 7 months ago

Never even considered self transition could be a thing, thanks, that simplifies it greatly. :D