Renaming nested StateMachine has unexpected behavior (rename to "null") and sometimes crashes

[Rodeo-McCabe]

Godot version

4.0 alpha 3

System information

Linux

Issue description

When renaming an AnimationNode that inside a BlendTree (used in an AnimationTree of course), the name is not changed to the entered text, and instead is changed to a seemingly arbitrary string (or sometimes "null"). In another project of mine, this causes crashes (possibly the same as #57370)

Also possibly related: #51889

Upon some further testing, I think this is is almost certainly a regression of #57370

Steps to reproduce

1. Open cube.tscn from the example project
2. Click on the AnimationTree in the scene tree dock
3. Edit the name of the Blend2 node in the AnimationTree editor.
4. Observe that the name is not changed to the text you entered.

Minimal reproduction project

AnimTreeNodeRenameBug.zip

[timothyqiu]

I can reproduce this in alpha3, but it seems already fixed on master.

[Rodeo-McCabe]

When the next alpha comes out in a couple days I will test again and close this issue if fixed.

[Rodeo-McCabe]

In alpha 4 using enter key to accept the entered text works more often, but also seems to crash more often. The unexpected values have stopped, but once it renamed itself to "null" instead ....

[and-rad]

This is a tricky one, but as far as I was able to determine so far it is caused by the animation blend tree editor listening to both the enter key being pressed and the text field losing focus. If either of these signal connections is disabled, the text field works as expected.

Maybe it's caused by both signals firing at the same time and the editor doing some funky stuff to nodes that have just become invalid as the graph is being updated? 🤷‍♂️ AnimationNodeBlendTreeEditor::_node_renamed is called for both signals by way of AnimationNodeBlendTreeEditor::_node_renamed_focus_out.

[SoapSpangledGames]

I have the same issue in 4.0 alpha 11. When a crash doesn't occur, the resulting name changes randomly or it changes to null. At times, the name of the previously selected node changes (to null or to some other name previously used by some other randomly chosen node) at the same time.

The name change seems to work more frequently when I tab out of the name field, but not always. That may just be random coincidence, though.

[and-rad]

Yeah, I can consistently avoid these problems when I deactivate either the connection to the LineEdit's text_submitted or focus_exited signals. Commenting out one of them fixes it completely, which must mean there is something wrong in how the animation tree editor handles these two signals.

godot/editor/plugins/animation_blend_tree_editor_plugin.cpp

Lines 150 to 151 in 1d06fec

	name->connect("text_submitted", callable_mp(this, &AnimationNodeBlendTreeEditor::_node_renamed), varray(agnode), CONNECT_DEFERRED);
	name->connect("focus_exited", callable_mp(this, &AnimationNodeBlendTreeEditor::_node_renamed_focus_out), varray(name, agnode), CONNECT_DEFERRED);

[snailrhymer]

I don't know much about pointers and memory, but I think I've largely worked out what's going on. The crashing and strange naming behaviour both seem to be caused by _node_renamed_focus_out expecting the name LineEdit as an argument, but it will have been freed when the graph updates as part of _node_renamed (here).

---

Crashes give this backtrace:

================================================================
handle_crash: Program crashed with signal 11
Engine version: Godot Engine v4.0.alpha.custom_build (181759e872eb29e11e1d441835199a7b25f64fb5)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[1] /lib/x86_64-linux-gnu/libc.so.6(+0x42520) [0x7fafadca1520] (??:0)
[2] /home/snailrhymer/Documents/godot/repo_clone/godot/bin/godot.linuxbsd.tools.64.llvm(__dynamic_cast+0x4f) [0x55a19ef7719f] (??:0)
[3] Node* Object::cast_to<Node>(Object*) (??:0)
[4] VariantObjectClassChecker<Node*>::check(Variant const&) (??:0)
[5] VariantCasterAndValidate<Node*>::cast(Variant const**, unsigned int, Callable::CallError&) (??:0)
[6] void call_with_variant_args_helper<AnimationNodeBlendTreeEditor, Node*, Ref<AnimationNode>, 0ul, 1ul>(AnimationNodeBlendTreeEditor*, void (AnimationNodeBlendTreeEditor::*)(Node*, Ref<AnimationNode>), Variant const**, Callable::CallError&, IndexSequence<0ul, 1ul>) (??:0)
[7] void call_with_variant_args<AnimationNodeBlendTreeEditor, Node*, Ref<AnimationNode> >(AnimationNodeBlendTreeEditor*, void (AnimationNodeBlendTreeEditor::*)(Node*, Ref<AnimationNode>), Variant const**, int, Callable::CallError&) (??:0)
[8] CallableCustomMethodPointer<AnimationNodeBlendTreeEditor, Node*, Ref<AnimationNode> >::call(Variant const**, int, Variant&, Callable::CallError&) const (??:0)
[9] Callable::call(Variant const**, int, Variant&, Callable::CallError&) const (??:0)
[10] MessageQueue::_call_function(Callable const&, Variant const*, int, bool) (??:0)
[11] MessageQueue::flush() (??:0)
[12] SceneTree::physics_process(double) (??:0)
[13] Main::iteration() (??:0)
[14] OS_LinuxBSD::run() (??:0)
[15] /home/snailrhymer/Documents/godot/repo_clone/godot/bin/godot.linuxbsd.tools.64.llvm(main+0x1eb) [0x55a199d7b31b] (??:0)
[16] /lib/x86_64-linux-gnu/libc.so.6(+0x29d90) [0x7fafadc88d90] (??:0)
[17] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x80) [0x7fafadc88e40] (??:0)
[18] /home/snailrhymer/Documents/godot/repo_clone/godot/bin/godot.linuxbsd.tools.64.llvm(_start+0x25) [0x55a199d7b065] (??:0)
-- END OF BACKTRACE --
================================================================

(custom build is master with some debugging lines)

It looks like the crash happens after _node_renamed has finished, as the message queue is calling _node_renamed_focus_out.

_node_renamed_focus_out expects the GraphNode's name LineEdit as its first argument (which is the name given when the focus exited signal is connected), but the LineEdit will have been freed when the graph is updated as part of _node_renamed.

This is causing a problem when the freed LineEdit is cast to an Object.

Infrequently, I get an error in place of the crash:

ERROR: Error calling deferred method: 'AnimationNodeBlendTreeEditor::AnimationNodeBlendTreeEditor::_node_renamed_focus_out': Cannot convert argument 1 from Object to Object..
   at: _call_function (core/object/message_queue.cpp:237)

which seems to be triggered by some addition or another I've made while debugging, but I can't pinpoint what.

---

I think the crash is averted if the pointer for the LineEdit happens to end up pointing to a Node, avoiding the above problems with casting. In this case, the strange name replacement is caused by le->call("get_text") here.

After the graph has been destroyed and rebuilt in _name_renamed, the pointer that used to point at the LineEdit may point at something else, often a Control node from the new graph. These tend to have a get_text function, so the call does return strings, e.g. "in", "On" or "Edit Filters", all text that appear on the OneShot nodes I was testing with.

Incorrect naming stops when le->call("get_text") is replaced with some placeholder string (everything instead gets the placeholder string as a name).

[Duskitten]

Can confirm that this is currently on the latest 4.0 A16 Build still
Trying to rename objects within the animationtree appears to cause one of 3 things to happen
is renamed to:

• "<Null>"
• Finds a random Node that exists, and uses that as a fallback + Iterates, example ("Blend2 2")
• Names it "AnimationNode + Random numbers"
• or if you're persistent enough on adding it, it just works fine and renames appropriately after the 4th or 5th try of copying and pasting the same value repeatedly

[Chevifier]

This still occurs in Godot 4.0 Beta 2. The work around seems to be to click or tab out of naming the node after typing in the name instead of hitting the Enter key. So the error seems to happen in the code that corresponds to the ENTER key being pressed

[EIREXE]

I'm 99% sure this is already fixed in 4.0, can someone confirm? I remember a PR doing it and it doesn't crash anymore on my system

[Calinou]

I'm 99% sure this is already fixed in 4.0, can someone confirm? I remember a PR doing it and it doesn't crash anymore on my system

The issue is still present in 4.0.beta according to #65289.

[EIREXE]

i see, thanks for clarifying

[TokageItLab]

I cannot reproduce it with any MRP. Is this an environment dependent issue? Does anyone have an MRP that causes the bug with the latest master?

[akien-mga]

I can't reproduce either in 4.0 RC 2. From the above comments it seems like others couldn't reproduce in November either, this was likely fixed in September or October.

[Tarik-B]

@akien-mga actually, I just did (over and over :x)

Edit : I've been having both the "arrows pointing to void" issue AND the occasional crash

[Tarik-B]

This always happens for me with both the 4.0rc2 version and my own master branch build (at 37589edf).
You'll find attached a minimal project I just created to reproduce the issue. You just need to open the scene and rename the "StateMachine" node of the animation tree.
godot_issue_58822.zip

[akien-mga]

Thanks for the new reproduction project!

It doesn't crash for me on 4.0.rc2 and latest master on Linux :/
On the other hand it does print an error each time the name is changed:

ERROR: Condition "!states.has(p_name)" is true. Returning: Vector2()
   at: get_node_position (scene/animation/animation_node_state_machine.cpp:1372)

And indeed the connection from Start jumps to point to empty space when renaming the node.

CC @TokageItLab

[Tarik-B]

Yes, the crash doesn't happen in this case (and I'm on Linux too) and I also have the same message as you (didn't see it before).

Actually, now that I think about it, I'm pretty sure I've only had a single crash after renaming (but I do know it happened). I'll post any more info I have here.

Thanks for replying that fast :)

[TokageItLab]

Probably this is a problem related to SubStateMachine (I believe that most of the problems mentioned above have been fixed). Currently the nested StateMachines are implemented in a very wrong way and their internal paths are also generated in a very hacky way, we plan to fix them in 4.1, but we can only send them cleanup PRs for 4.0RC. What should we do? (The connection to the nested StateMachine will be disconnected by those PR, resulting in a slight breaks compatibility.)
@SaracenOne @lyuma @akien-mga

[lyuma]

if there isn't time to fix it properly for 4.0, an interim fix is to forbid renaming in the conditions that cause the bug (if there is nesting)...

And if users really want to rename for now, they can manually find/replace within the .tscn or .tres.

Alternatively, we can see if it's a simple mistake that is easy to fix in the existing code.

[hunterloftis]

Just hit this today in 4.0.1-rc2.

if there isn't time to fix it properly for 4.0, an interim fix is to forbid renaming in the conditions that cause the bug (if there is nesting)...

What are these conditions? I'd love to avoid them.