Invalid access error because of `LimboState._exit()` call order

[ydeltastar]

Godot version

4.3.1.rc

LimboAI version

v1.3-dev [08884e618059bfb8bf5646d6544017ae0a515860]

LimboAI variant

Module (custom editor or template build)

Issue description

LimboState._exit() is called after Node._exit_tree when the scene is deleted. This makes references to other nodes null or invalid which can cause error because the scene already exited the tree.

extends LimboState

@export var animation_player:AnimationPlayer
@export var animation_name:StringName

func _enter() -> void:
    animation_player.animation_finished.connect(_on_finished)
    animation_player.play(animation_name)

func _exit() -> void:
    animation_player.animation_finished.disconnect(_on_finished)

func _on_finished(anim_name: StringName):
    if anim_name == animation_name:
        print_debug("Animation finished")

I have a state with a setup like this. When I use free(), queue_free() or get_tree().unload_current_scene(), it will crash with an invalid access to previously freed animation_player at _exit().

How to reproduce

state-exit.zip

• Run the project.
• Click the "Reload" button which will delete the Godot icon scene and instantiate it again.
• It will crash with an error.

[limbonaut]

The _exit is called on NOTFICIATION_EXIT_TREE intentionally, as it provides a way for the behavior tree tasks to release their resources (like a pathfinder instance or a signal connection). It should be documented better, though. Nodes must be checked if they are valid in _exit. It's not strictly necessary to have it this way in LimboState, as nodes have access to NOTFICIATION_EXIT_TREE on their own, but I wanted the API for tasks and states to be similar, meaning you hold to resources on enter (or setup), and release on exit. This is a big gotcha, though, so it should be documented.

[ydeltastar]

The _exit is called on NOTFICIATION_EXIT_TREE

This wouldn't cause the issue because it happens before they are removed from the tree. Currently _exit is called on NOTIFICATION_PREDELETE which happens after it and nodes below the LimboState were deleted from memory so references are invalid.

https://github.com/limbonaut/limboai/blob/08884e618059bfb8bf5646d6544017ae0a515860/hsm/limbo_state.cpp#L193-L197

It looks like it is regression https://github.com/limbonaut/limboai/pull/131

[limbonaut]

Okay, I'll revert it and try a different fix for that issue.

[limbonaut]

Does it actually crash in your case? It generates an error and a breakpoint for me, but the process is alive and I can continue execution.

[ydeltastar]

Yeah, wrong wording. It's an error since it's GDScript.

[andmish]

@limbonaut I noticed the same issue a while ago too, but I already got used to just putting this quick one-liner in all exits that solves it

func _exit() -> void:
    if not agent.is_inside_tree(): return
    # your code here

I don't know is it possible or not, but can this agent.is_inside_tree() used somehow in c++ part as a solution, will it fix it or I'm missing something? like

case NOTIFICATION_PREDELETE: {
    if (!agent.is_inside_tree()) {
        return;
    }

    if (is_active()) {
        _exit();
    }

And sorry, but I can't say this https://github.com/limbonaut/limboai/pull/226 new solution is a solution at all, it's just brings back the same issue as before. The main issue in https://github.com/limbonaut/limboai/issues/128 wasn't just about states getting inactive (before you also just could call set_active(true) yourself, after re-parenting right in gdscript), it's about that re-parenting as an action is affecting states and AI logic in the first place, which it shouldn't do at all.

There are a lot of gameplay situations (especially in heavily animation driven games, with animation blends) where re-parenting need to be done during(in the middle) animation or tween and then current state need to be switched to new state at the end of that animation/tween (examples, AI approaching and getting into the vehicle, character controller and ladder states, or some tricks you will want to do using re-parenting for ledge grabbing/moving along states), so re-parenting there now will triggers _exit() every time, interrupts the animations/tweens and resets state to initial state (which is also can be completely undesirable, as sometimes initial states are here just for init/reset parameters/variables, I know you can redefine initial_state before re-parenting, but it's not about it and will just adds more complications). Or even simpler example: character Walking forward on platforms and you re-parent character from one platform to another when it steps on new one for some gameplay reason, if you have some logic in the Walking _exit(), it will be triggered now every time (resetting to initial state, interrupting character walking animation, etc. depends what you do here), it's just weird behavior for a state machine.

[ydeltastar]

This should go in NOTIFICATION_EXIT_TREE instead. I think it will fix both cases. This will not work too. Since it can't guess the intention, the callbacks should be manually disabled while reparenting. Like I pointed out in my comment, by setting process_mode to disabled or setting the state inactive with set_active(false). NOTIFICATION_PREDELETE shouldn't be used for logic that could be interacting with the node tree.

[andmish]

none of those solutions will work.

furthermore, you can't set_active(false) a State, you can set_active(false) only a HSM, and setting HSM to set_active(false) actually doesn't make any sense here as a solution at all, because after calling set_active(true) HSM will automatically start from initial_state

try your solutions here in walk.call_on_enter to proceed to crawl, that reparent in walk.call_on_enter brings us back to idle and just skips hsm.dispatch(CRAWL)

extends Node3D

@onready var new_parent = $"../NewParent"
@onready var hsm: LimboHSM = $LimboHSM
@onready var idle: LimboState = $LimboHSM/Idle
@onready var walk: LimboState = $LimboHSM/Walk
@onready var crawl: LimboState = $LimboHSM/Crawl

const WALK = &"WALK"
const CRAWL = &"CRAWL"

func _ready():
    hsm.initial_state = idle
    hsm.add_transition(idle, walk, WALK)
    hsm.add_transition(walk, crawl, CRAWL)

    idle.call_on_enter(func():
        print("IDLE ENTER")
        await get_tree().create_timer(2.0).timeout
        hsm.dispatch(WALK)
        )

    idle.call_on_exit(func():
        print("IDLE EXIT")
        )   

    walk.call_on_enter(func():
        print("WALK ENTER")
        await get_tree().create_timer(2.0).timeout
        reparent(new_parent, false)
        await get_tree().create_timer(2.0).timeout
        hsm.dispatch(CRAWL)
        )

    walk.call_on_exit(func():
        print("WALK EXIT")
        )

    crawl.call_on_enter(func():
        print("CRAWL ENTER")
        )

    hsm.initialize(self)
    hsm.set_active(true)

[ydeltastar]

Of course. I don't mean it currently works like this. I'm talking about what can be implemented to make it work. It should make perfectly sense that the state machine doesn't process things automatically when disabled like other Godot nodes. In this case using process_mode since set_active(true) will trigger side effects too.

[limbonaut]

@andmish @ydeltastar I think I found a decent solution, which I'm going to merge soon. Testing is appreciated. I've checked with both MRPs and it works fine.

[limbonaut]

Builds: https://github.com/limbonaut/limboai/actions/runs/11634183281

[ydeltastar]

It works well for my use case. May be worthwhile including a note in the docs about the free() vs queue_free() case.