Fix crash when re-importing model with AnimationPlayer panel open and node selected

[yahkr]

closes #95769
closes #95744

Edit: Removed CONNECT_DEFERRED from connection

player->connect(SNAME("current_animation_changed"), ... ), CONNECT_DEFERRED);

->

player->connect(SNAME("current_animation_changed"), ... ));

The re-import process attempts to update the scene via EditorNode::reload_instances_with_path_in_edited_scenes(). I believe this function disassembles the scene and then reassembles it with the updated resource. During this process, the AnimationPlayer player is null, leading to the crash."

OLD FIX
added null check to AnimationPlayer before use within AnimationPlayerEditor::_current_animation_changed

[AThousandShips]

I'm not sure this is a proper fix, there's no reason the player should be connected and be null IMO, I think there's some missing disconnection somewhere, I think this just hides a larger issue that should be fixed

[yahkr]

I'm not sure this is a proper fix, there's no reason the player should be connected and be null IMO, I think there's some missing disconnection somewhere, I think this just hides a larger issue that should be fixed

I definitely agree, I'd much rather not see this method being called at all with a null player.

Here's what I believe the timeline of events are:

EditorNode::reload_instances_with_path_in_edited_scenes():5983
EditorData::restore_edited_scene_state
AnimationPlayerEditor::set_state
AnimationPlayerEditor::_update_player()
AnimationPlayer::set_assigned_animation
emit_signal(SNAME("current_animation_changed"), playback.assigned);
//Called on player addresses 0x...fc0,  0x...4b6,  0x...fc0,  0x...4b6
//0x...fc0 becoming the new valid player, 0x...4b6 is the old one

->

EditorNode::reload_instances_with_path_in_edited_scenes():6138
AnimationPlayerEditor::_node_removed
// player is set to null, addresses 0x...4b6

->

AnimationPlayerEditor::_current_animation_changed
// signal resolves and player is null (crash) (i believe to be trying to hit the old 0x...4b6 player)
//new player 0x...fc0 then called and works as expected.

I attempted to disconnect the callbacks for these methods on removal, but no dice since the signals are emitted beforehand?

This null player gets cleaned up after this and the new one loads in as expected. Happy to add these disconnects in the node_removed, however I'm not super familiar with how signals are handled in cpp, so not certain they are necessary cleanup wise:

void AnimationPlayerEditor::_node_removed(Node *p_node) {
	if (player && original_node == p_node) {
		if (is_dummy) {
			plugin->_clear_dummy_player();
		}
		if (player->is_connected(SNAME("animation_list_changed"), callable_mp(this, &AnimationPlayerEditor::_animation_libraries_updated))) {
			player->disconnect(SNAME("animation_list_changed"), callable_mp(this, &AnimationPlayerEditor::_animation_libraries_updated));
		}
		if (player->is_connected(SNAME("current_animation_changed"), callable_mp(this, &AnimationPlayerEditor::_current_animation_changed))) {
			player->disconnect(SNAME("current_animation_changed"), callable_mp(this, &AnimationPlayerEditor::_current_animation_changed));
		}
		player = nullptr;

		set_process(false);

		track_editor->set_animation(Ref<Animation>(), true);
		track_editor->set_root(nullptr);
		track_editor->show_select_node_warning(true);
		_update_player();

		_ensure_dummy_player();

		pin->set_pressed(false);
	}
}

[AThousandShips]

Will take a look, but would need a good check for what causes this specific case, from my casual reading of the code there shouldn't be any time where:

• player == nullptr
• Signals haven't been disconnected

So either this is a race condition (always possible) or something isn't handled properly, I'll try and go through the code later today or tomorrow and see if I can identify any weird looking code

But yes that _node_removed code does look suspicious, so would be good to test adding the disconnect there and see if that solves it

[yahkr]

So either this is a race condition (always possible) or something isn't handled properly, I'll try and go through the code later today or tomorrow and see if I can identify any weird looking code

But yes that _node_removed code does look suspicious, so would be good to test adding the disconnect there and see if that solves it

Sorry that's what I meant by I attempted to add those disconnects. These do not exist in the repo as it stands. But adding them had no observable effect on this unfortunately. I do think they make sense there though...

if (player->is_connected(SNAME("animation_list_changed"), callable_mp(this, &AnimationPlayerEditor::_animation_libraries_updated))) {
    player->disconnect(SNAME("animation_list_changed"), callable_mp(this, &AnimationPlayerEditor::_animation_libraries_updated));
}
if (player->is_connected(SNAME("current_animation_changed"), callable_mp(this, &AnimationPlayerEditor::_current_animation_changed))) {
    player->disconnect(SNAME("current_animation_changed"), callable_mp(this, &AnimationPlayerEditor::_current_animation_changed));
}

will keep digging :)

[yahkr]

Well, that was simpler than I thought... Certainly odd that animation_list_changed is CONNECT_DEFERRED, but now both "current_animation_changed" connections are not. Seems to have alleviated the issue

[AThousandShips]

This was introduced in:

• Implement AnimationMixer as a base class of AnimationPlayer and AnimationTree #80813

Didn't find any explanation in the PR for this specific change (it wasn't before)

Looking at the code (and that the libraries updated signal uses it consistently) I assume this was a copy/paste error that snuck in

[SaracenOne]

To summarize my understanding of the issue:
You're correct because of the hot-reloading is essentially quietly swapping the whole scene around and the old animation player emits a deferred signal which is then picked up by the AnimationPlayerEditor which by that time has had its AnimationPlayer pointer set to null. Still trying to understand why it would have been flagged as deferred so it might be worth pinging @TokageItLab in case they remember. Either way, there should be mulitple ways to workaround this, but I would like to know if there was a specific reason the DEFERRED flag was added since I'd be nervous about other implications of changing it.

[TokageItLab]

Since the AnimationTree now has a direct AnimationLibrary, the AnimationEditor can directly create an AnimationPlayer if it has one, or a dummy AnimationPlayer if it has an AnimationTree and assigns it.

Will the AnimationPlayer be regenerated on re-import? If so, I think the problem is when moving from the old pointer to the new one.

[TokageItLab]

For now, crashes need to be prevented, so I approve it. The crash is pointed out in #96637, but since this PR will resolve the issue before reaching #96637, this PR takes precedence.

Probably the crash is caused by _current_animation_changed being called after the player is deleted, so disconnecting the signal when the player is deleted could also solve the problem, but I don't remember why I make the connection be deferred. I didn't see any regression with this PR with quick test, so it might be fine.

[Repiteo]

If there's no obvious regressions to speak of, this should be fine to add; it's a one-liner, so a revert would be painless if it comes to that

[Repiteo]

Thanks!