-Fixes to undo redo to avoid crash, closes #24251

-Changed Animation to have a special signal when tracks are changed, to avoid unnecesary track cache rebuilds in AnimationPlayer
-Added missing emit_changed whe modifying keys to Animation
-Changed AnimationPlayer to use the new refcounted connections instead of the previous hacky way to keep references
-Changed AnimationEditor to update the current track when keys are edited
-Fixed bug where undo/redo did not work with AnimationKeyEdit (was not being updated)
-Made sure UndoRedo does not mind deleted objects in undo/redo history, this would corrupt the history or clear it without need.

12 files changed, 68 insertions, 30 deletions

diff --git a/core/undo_redo.cpp b/core/undo_redo.cpp
index 6bc882eaec..00894b41d8 100644
--- a/core/undo_redo.cpp
+++ b/core/undo_redo.cpp
@@ -234,7 +234,9 @@ void UndoRedo::_pop_history_tail() {
 	}
 
 	actions.remove(0);
-	current_action--;
+	if (current_action >= 0) {
+		current_action--;
+	}
 }
 
 void UndoRedo::commit_action() {
@@ -258,11 +260,8 @@ void UndoRedo::_process_operation_list(List<Operation>::Element *E) {
 		Operation &op = E->get();
 
 		Object *obj = ObjectDB::get_instance(op.object);
-		if (!obj) {
-			//corruption
-			clear_history();
-			ERR_FAIL_COND(!obj);
-		}
+		if (!obj) //may have been deleted and this is fine
+			continue;
 
 		switch (op.type) {
 
diff --git a/editor/animation_track_editor.cpp b/editor/animation_track_editor.cpp
index 662137d60d..929d6c7642 100644
--- a/editor/animation_track_editor.cpp
+++ b/editor/animation_track_editor.cpp
@@ -45,18 +45,22 @@ class AnimationTrackKeyEdit : public Object {
 
 public:
 	bool setting;
-	bool hidden;
 
 	bool _hide_script_from_inspector() {
 		return true;
 	}
 
+	bool _dont_undo_redo() {
+		return true;
+	}
+
 	static void _bind_methods() {
 
 		ClassDB::bind_method("_update_obj", &AnimationTrackKeyEdit::_update_obj);
 		ClassDB::bind_method("_key_ofs_changed", &AnimationTrackKeyEdit::_key_ofs_changed);
 		ClassDB::bind_method("_hide_script_from_inspector", &AnimationTrackKeyEdit::_hide_script_from_inspector);
 		ClassDB::bind_method("get_root_path", &AnimationTrackKeyEdit::get_root_path);
+		ClassDB::bind_method("_dont_undo_redo", &AnimationTrackKeyEdit::_dont_undo_redo);
 	}
 
 	//PopupDialog *ke_dialog;
@@ -82,16 +86,13 @@ public:
 	void _update_obj(const Ref<Animation> &p_anim) {
 		if (setting)
 			return;
-		if (hidden)
-			return;
 		if (!(animation == p_anim))
 			return;
+
 		notify_change();
 	}
 
 	void _key_ofs_changed(const Ref<Animation> &p_anim, float from, float to) {
-		if (hidden)
-			return;
 		if (!(animation == p_anim))
 			return;
 		if (from != key_ofs)
@@ -168,6 +169,7 @@ public:
 				undo_redo->add_do_method(this, "_update_obj", animation);
 				undo_redo->add_undo_method(this, "_update_obj", animation);
 				undo_redo->commit_action();
+
 				setting = false;
 				return true;
 
@@ -191,6 +193,7 @@ public:
 					undo_redo->add_do_method(this, "_update_obj", animation);
 					undo_redo->add_undo_method(this, "_update_obj", animation);
 					undo_redo->commit_action();
+
 					setting = false;
 					return true;
 				}
@@ -267,6 +270,7 @@ public:
 				undo_redo->add_do_method(this, "_update_obj", animation);
 				undo_redo->add_undo_method(this, "_update_obj", animation);
 				undo_redo->commit_action();
+
 				setting = false;
 				if (change_notify_deserved)
 					notify_change();
@@ -286,6 +290,7 @@ public:
 					undo_redo->add_do_method(this, "_update_obj", animation);
 					undo_redo->add_undo_method(this, "_update_obj", animation);
 					undo_redo->commit_action();
+
 					setting = false;
 					return true;
 				}
@@ -301,6 +306,7 @@ public:
 					undo_redo->add_do_method(this, "_update_obj", animation);
 					undo_redo->add_undo_method(this, "_update_obj", animation);
 					undo_redo->commit_action();
+
 					setting = false;
 					return true;
 				}
@@ -316,6 +322,7 @@ public:
 					undo_redo->add_do_method(this, "_update_obj", animation);
 					undo_redo->add_undo_method(this, "_update_obj", animation);
 					undo_redo->commit_action();
+
 					setting = false;
 					return true;
 				}
@@ -335,6 +342,7 @@ public:
 					undo_redo->add_do_method(this, "_update_obj", animation);
 					undo_redo->add_undo_method(this, "_update_obj", animation);
 					undo_redo->commit_action();
+
 					setting = false;
 					return true;
 				}
@@ -350,6 +358,7 @@ public:
 					undo_redo->add_do_method(this, "_update_obj", animation);
 					undo_redo->add_undo_method(this, "_update_obj", animation);
 					undo_redo->commit_action();
+
 					setting = false;
 					return true;
 				}
@@ -365,6 +374,7 @@ public:
 					undo_redo->add_do_method(this, "_update_obj", animation);
 					undo_redo->add_undo_method(this, "_update_obj", animation);
 					undo_redo->commit_action();
+
 					setting = false;
 					return true;
 				}
@@ -639,6 +649,8 @@ public:
 	PropertyInfo hint;
 	NodePath base;
 
+	AnimationTrackEditor *track_editor;
+
 	void notify_change() {
 
 		_change_notify();
@@ -649,11 +661,11 @@ public:
 	}
 
 	AnimationTrackKeyEdit() {
-		hidden = true;
 		key_ofs = 0;
 		track = -1;
 		setting = false;
 		root_path = NULL;
+		track_editor = NULL;
 	}
 };
 
@@ -3414,6 +3426,14 @@ void AnimationTrackEditor::_update_tracks() {
 
 void AnimationTrackEditor::_animation_changed() {
 
+	if (key_edit && key_edit->setting) {
+		//if editing a key, just update the edited track, makes refresh less costly
+		if (key_edit->track < track_edits.size()) {
+			track_edits[key_edit->track]->update();
+		}
+		return;
+	}
+
 	timeline->update();
 	timeline->update_values();
 	if (block_animation_update) {
@@ -3944,6 +3964,7 @@ void AnimationTrackEditor::_update_key_edit() {
 	key_edit = memnew(AnimationTrackKeyEdit);
 	key_edit->animation = animation;
 	key_edit->track = selection.front()->key().track;
+	key_edit->track_editor = this;
 
 	float ofs = animation->track_get_key_time(key_edit->track, selection.front()->key().key);
 	key_edit->key_ofs = ofs;
diff --git a/editor/array_property_edit.cpp b/editor/array_property_edit.cpp
index 7a3edfde50..72beeaaf45 100644
--- a/editor/array_property_edit.cpp
+++ b/editor/array_property_edit.cpp
@@ -288,12 +288,17 @@ Node *ArrayPropertyEdit::get_node() {
 	return Object::cast_to<Node>(ObjectDB::get_instance(obj));
 }
 
+bool ArrayPropertyEdit::_dont_undo_redo() {
+	return true;
+}
+
 void ArrayPropertyEdit::_bind_methods() {
 
 	ClassDB::bind_method(D_METHOD("_set_size"), &ArrayPropertyEdit::_set_size);
 	ClassDB::bind_method(D_METHOD("_set_value"), &ArrayPropertyEdit::_set_value);
 	ClassDB::bind_method(D_METHOD("_notif_change"), &ArrayPropertyEdit::_notif_change);
 	ClassDB::bind_method(D_METHOD("_notif_changev"), &ArrayPropertyEdit::_notif_changev);
+	ClassDB::bind_method(D_METHOD("_dont_undo_redo"), &ArrayPropertyEdit::_dont_undo_redo);
 }
 
 ArrayPropertyEdit::ArrayPropertyEdit() {
diff --git a/editor/array_property_edit.h b/editor/array_property_edit.h
index b2a3564c8c..fd17ebe9f8 100644
--- a/editor/array_property_edit.h
+++ b/editor/array_property_edit.h
@@ -52,6 +52,8 @@ class ArrayPropertyEdit : public Reference {
 	void _set_size(int p_size);
 	void _set_value(int p_idx, const Variant &p_value);
 
+	bool _dont_undo_redo();
+
 protected:
 	static void _bind_methods();
 	bool _set(const StringName &p_name, const Variant &p_value);
diff --git a/editor/dictionary_property_edit.cpp b/editor/dictionary_property_edit.cpp
index 483f5ae2d0..9f4096ca02 100644
--- a/editor/dictionary_property_edit.cpp
+++ b/editor/dictionary_property_edit.cpp
@@ -102,12 +102,17 @@ Node *DictionaryPropertyEdit::get_node() {
 	return cast_to<Node>(o);
 }
 
+bool DictionaryPropertyEdit::_dont_undo_redo() {
+	return true;
+}
+
 void DictionaryPropertyEdit::_bind_methods() {
 
 	ClassDB::bind_method(D_METHOD("_set_key"), &DictionaryPropertyEdit::_set_key);
 	ClassDB::bind_method(D_METHOD("_set_value"), &DictionaryPropertyEdit::_set_value);
 	ClassDB::bind_method(D_METHOD("_notif_change"), &DictionaryPropertyEdit::_notif_change);
 	ClassDB::bind_method(D_METHOD("_notif_changev"), &DictionaryPropertyEdit::_notif_changev);
+	ClassDB::bind_method(D_METHOD("_dont_undo_redo"), &DictionaryPropertyEdit::_dont_undo_redo);
 }
 
 bool DictionaryPropertyEdit::_set(const StringName &p_name, const Variant &p_value) {
diff --git a/editor/dictionary_property_edit.h b/editor/dictionary_property_edit.h
index f6bd13785a..a2a81c261c 100644
--- a/editor/dictionary_property_edit.h
+++ b/editor/dictionary_property_edit.h
@@ -46,6 +46,8 @@ class DictionaryPropertyEdit : public Reference {
 
 	Variant get_dictionary() const;
 
+	bool _dont_undo_redo();
+
 protected:
 	static void _bind_methods();
 	bool _set(const StringName &p_name, const Variant &p_value);
diff --git a/editor/editor_inspector.cpp b/editor/editor_inspector.cpp
index 1078fabc2e..94761cadef 100644
--- a/editor/editor_inspector.cpp
+++ b/editor/editor_inspector.cpp
@@ -1937,7 +1937,7 @@ void EditorInspector::_edit_set(const String &p_name, const Variant &p_value, bo
 		}
 	}
 
-	if (!undo_redo || Object::cast_to<ArrayPropertyEdit>(object) || Object::cast_to<DictionaryPropertyEdit>(object)) { //kind of hacky
+	if (!undo_redo || bool(object->call("_dont_undo_redo"))) {
 
 		object->set(p_name, p_value);
 		if (p_refresh_all)
diff --git a/scene/animation/animation_player.cpp b/scene/animation/animation_player.cpp
index 54be2aa159..86fd1ef14c 100644
--- a/scene/animation/animation_player.cpp
+++ b/scene/animation/animation_player.cpp
@@ -991,25 +991,12 @@ void AnimationPlayer::remove_animation(const StringName &p_name) {
 
 void AnimationPlayer::_ref_anim(const Ref<Animation> &p_anim) {
 
-	if (used_anims.has(p_anim))
-		used_anims[p_anim]++;
-	else {
-		used_anims[p_anim] = 1;
-		Ref<Animation>(p_anim)->connect("changed", this, "_animation_changed");
-	}
+	Ref<Animation>(p_anim)->connect(SceneStringNames::get_singleton()->tracks_changed, this, "_animation_changed", varray(), CONNECT_REFERENCE_COUNTED);
 }
 
 void AnimationPlayer::_unref_anim(const Ref<Animation> &p_anim) {
 
-	ERR_FAIL_COND(!used_anims.has(p_anim));
-
-	int &n = used_anims[p_anim];
-	n--;
-	if (n == 0) {
-
-		Ref<Animation>(p_anim)->disconnect("changed", this, "_animation_changed");
-		used_anims.erase(p_anim);
-	}
+	Ref<Animation>(p_anim)->disconnect(SceneStringNames::get_singleton()->tracks_changed, this, "_animation_changed");
 }
 
 void AnimationPlayer::rename_animation(const StringName &p_name, const StringName &p_new_name) {
diff --git a/scene/animation/animation_player.h b/scene/animation/animation_player.h
index 9bba848298..fea4819821 100644
--- a/scene/animation/animation_player.h
+++ b/scene/animation/animation_player.h
@@ -181,8 +181,6 @@ private:
 	int cache_update_bezier_size;
 	Set<TrackNodeCache *> playing_caches;
 
-	Map<Ref<Animation>, int> used_anims;
-
 	uint64_t accum_pass;
 	float speed_scale;
 	float default_blend_time;
diff --git a/scene/resources/animation.cpp b/scene/resources/animation.cpp
index 3156e12c50..a7544cc031 100644
--- a/scene/resources/animation.cpp
+++ b/scene/resources/animation.cpp
@@ -29,6 +29,7 @@
 /*************************************************************************/
 
 #include "animation.h"
+#include "scene/scene_string_names.h"
 
 #include "core/math/geometry.h"
 
@@ -668,6 +669,7 @@ int Animation::add_track(TrackType p_type, int p_at_pos) {
 		}
 	}
 	emit_changed();
+	emit_signal(SceneStringNames::get_singleton()->tracks_changed);
 	return p_at_pos;
 }
 
@@ -719,6 +721,7 @@ void Animation::remove_track(int p_track) {
 	memdelete(t);
 	tracks.remove(p_track);
 	emit_changed();
+	emit_signal(SceneStringNames::get_singleton()->tracks_changed);
 }
 
 int Animation::get_track_count() const {
@@ -737,6 +740,7 @@ void Animation::track_set_path(int p_track, const NodePath &p_path) {
 	ERR_FAIL_INDEX(p_track, tracks.size());
 	tracks[p_track]->path = p_path;
 	emit_changed();
+	emit_signal(SceneStringNames::get_singleton()->tracks_changed);
 }
 
 NodePath Animation::track_get_path(int p_track) const {
@@ -1410,6 +1414,8 @@ void Animation::track_set_key_value(int p_track, int p_key_idx, const Variant &p
 
 		} break;
 	}
+
+	emit_changed();
 }
 
 void Animation::track_set_key_transition(int p_track, int p_key_idx, float p_transition) {
@@ -1445,6 +1451,8 @@ void Animation::track_set_key_transition(int p_track, int p_key_idx, float p_tra
 			// they don't use transition
 		} break;
 	}
+
+	emit_changed();
 }
 
 template <class K>
@@ -2554,6 +2562,7 @@ void Animation::track_move_up(int p_track) {
 	}
 
 	emit_changed();
+	emit_signal(SceneStringNames::get_singleton()->tracks_changed);
 }
 
 void Animation::track_set_imported(int p_track, bool p_imported) {
@@ -2588,6 +2597,7 @@ void Animation::track_move_down(int p_track) {
 		SWAP(tracks.write[p_track], tracks.write[p_track - 1]);
 	}
 	emit_changed();
+	emit_signal(SceneStringNames::get_singleton()->tracks_changed);
 }
 
 void Animation::track_swap(int p_track, int p_with_track) {
@@ -2598,6 +2608,7 @@ void Animation::track_swap(int p_track, int p_with_track) {
 		return;
 	SWAP(tracks.write[p_track], tracks.write[p_with_track]);
 	emit_changed();
+	emit_signal(SceneStringNames::get_singleton()->tracks_changed);
 }
 
 void Animation::set_step(float p_step) {
@@ -2716,6 +2727,8 @@ void Animation::_bind_methods() {
 	ADD_PROPERTY(PropertyInfo(Variant::BOOL, "loop"), "set_loop", "has_loop");
 	ADD_PROPERTY(PropertyInfo(Variant::REAL, "step", PROPERTY_HINT_RANGE, "0,4096,0.001"), "set_step", "get_step");
 
+	ADD_SIGNAL(MethodInfo("tracks_changed"));
+
 	BIND_ENUM_CONSTANT(TYPE_VALUE);
 	BIND_ENUM_CONSTANT(TYPE_TRANSFORM);
 	BIND_ENUM_CONSTANT(TYPE_METHOD);
@@ -2740,6 +2753,8 @@ void Animation::clear() {
 	tracks.clear();
 	loop = false;
 	length = 1;
+	emit_changed();
+	emit_signal(SceneStringNames::get_singleton()->tracks_changed);
 }
 
 bool Animation::_transform_track_optimize_key(const TKey<TransformKey> &t0, const TKey<TransformKey> &t1, const TKey<TransformKey> &t2, float p_alowed_linear_err, float p_alowed_angular_err, float p_max_optimizable_angle, const Vector3 &p_norm) {
diff --git a/scene/scene_string_names.cpp b/scene/scene_string_names.cpp
index 305e4fc735..634f4d7160 100644
--- a/scene/scene_string_names.cpp
+++ b/scene/scene_string_names.cpp
@@ -203,4 +203,6 @@ SceneStringNames::SceneStringNames() {
 	_mesh_changed = StaticCString::create("_mesh_changed");
 
 	parameters_base_path = "parameters/";
+
+	tracks_changed = "tracks_changed";
 }
diff --git a/scene/scene_string_names.h b/scene/scene_string_names.h
index 4d1a7f255c..bc75165b8c 100644
--- a/scene/scene_string_names.h
+++ b/scene/scene_string_names.h
@@ -205,6 +205,8 @@ public:
 
 	StringName parameters_base_path;
 
+	StringName tracks_changed;
+
 	enum {
 		MAX_MATERIALS = 32
 	};