Merge pull request #113366 from aaronp64/free_gamestate_snapshots

Fix `GameStateSnapshot`s not being freed

Author: akien-mga

modules/objectdb_profiler/editor/objectdb_profiler_panel.cpp

@@ -41,7 +41,9 @@
 #include "core/os/time.h"
 #include "editor/debugger/editor_debugger_node.h"
 #include "editor/debugger/script_editor_debugger.h"
+#include "editor/docks/inspector_dock.h"
 #include "editor/editor_node.h"
+#include "editor/inspector/editor_inspector.h"
 #include "editor/themes/editor_scale.h"
 #include "scene/gui/button.h"
 #include "scene/gui/label.h"
@@ -167,7 +169,7 @@ TreeItem *ObjectDBProfilerPanel::_add_snapshot_button(const String &p_snapshot_f
 void ObjectDBProfilerPanel::_show_selected_snapshot() {
 	if (snapshot_list->get_selected()->get_text(0) == (String)diff_button->get_selected_metadata()) {
 		for (int i = 0; i < diff_button->get_item_count(); i++) {
-			if (diff_button->get_item_text(i) == current_snapshot->get_snapshot()->name) {
+			if (diff_button->get_item_text(i) == current_snapshot->name) {
 				diff_button->select(i);
 				break;
 			}
@@ -184,7 +186,7 @@ void ObjectDBProfilerPanel::_on_snapshot_deselected() {
 	_update_enabled_diff_items();
 }
 
-Ref<GameStateSnapshotRef> ObjectDBProfilerPanel::get_snapshot(const String &p_snapshot_file_name) {
+Ref<GameStateSnapshot> ObjectDBProfilerPanel::get_snapshot(const String &p_snapshot_file_name) {
 	if (snapshot_cache.has(p_snapshot_file_name)) {
 		return snapshot_cache.get(p_snapshot_file_name);
 	}
@@ -201,7 +203,7 @@ Ref<GameStateSnapshotRef> ObjectDBProfilerPanel::get_snapshot(const String &p_sn
 	Vector<uint8_t> content = snapshot_file->get_buffer(snapshot_file->get_length()); // We want to split on newlines, so normalize them.
 	ERR_FAIL_COND_V_MSG(content.is_empty(), nullptr, "ObjectDB Snapshot file is empty: " + full_file_path);
 
-	Ref<GameStateSnapshotRef> snapshot = GameStateSnapshot::create_ref(p_snapshot_file_name, content);
+	Ref<GameStateSnapshot> snapshot = GameStateSnapshot::create_ref(p_snapshot_file_name, content);
 	if (snapshot.is_valid()) {
 		snapshot_cache.insert(p_snapshot_file_name, snapshot);
 	}
@@ -225,8 +227,8 @@ void ObjectDBProfilerPanel::_view_tab_changed(int p_tab_idx) {
 	// Populating tabs only on tab changed because we're handling a lot of data,
 	// and the editor freezes for a while if we try to populate every tab at once.
 	SnapshotView *view = cast_to<SnapshotView>(view_tabs->get_current_tab_control());
-	GameStateSnapshot *snapshot = current_snapshot.is_null() ? nullptr : current_snapshot->get_snapshot();
-	GameStateSnapshot *diff = diff_snapshot.is_null() ? nullptr : diff_snapshot->get_snapshot();
+	GameStateSnapshot *snapshot = current_snapshot.ptr();
+	GameStateSnapshot *diff = diff_snapshot.ptr();
 	if (snapshot != nullptr && !view->is_showing_snapshot(snapshot, diff)) {
 		view->show_snapshot(snapshot, diff);
 	}
@@ -237,6 +239,11 @@ void ObjectDBProfilerPanel::clear_snapshot(bool p_update_view_tabs) {
 		view->clear_snapshot();
 	}
 
+	const Object *edited_object = InspectorDock::get_inspector_singleton()->get_edited_object();
+	if (Object::cast_to<SnapshotDataObject>(edited_object)) {
+		EditorNode::get_singleton()->push_item(nullptr);
+	}
+
 	current_snapshot.unref();
 	diff_snapshot.unref();
 
@@ -333,7 +340,7 @@ void ObjectDBProfilerPanel::_edit_snapshot_name() {
 ObjectDBProfilerPanel::ObjectDBProfilerPanel() {
 	set_name(TTRC("ObjectDB Profiler"));
 
-	snapshot_cache = LRUCache<String, Ref<GameStateSnapshotRef>>(SNAPSHOT_CACHE_MAX_SIZE);
+	snapshot_cache = LRUCache<String, Ref<GameStateSnapshot>>(SNAPSHOT_CACHE_MAX_SIZE);
 
 	EditorDebuggerNode::get_singleton()->get_current_debugger()->connect("breaked", callable_mp(this, &ObjectDBProfilerPanel::_on_debug_breaked));
 

modules/objectdb_profiler/editor/objectdb_profiler_panel.h

@@ -69,9 +69,9 @@ class ObjectDBProfilerPanel : public Control {
 	HashMap<int, PartialSnapshot> partial_snapshots;
 
 	LocalVector<SnapshotView *> views;
-	Ref<GameStateSnapshotRef> current_snapshot;
-	Ref<GameStateSnapshotRef> diff_snapshot;
-	LRUCache<String, Ref<GameStateSnapshotRef>> snapshot_cache;
+	Ref<GameStateSnapshot> current_snapshot;
+	Ref<GameStateSnapshot> diff_snapshot;
+	LRUCache<String, Ref<GameStateSnapshot>> snapshot_cache;
 
 	void _request_object_snapshot();
 	void _begin_object_snapshot();
@@ -95,7 +95,7 @@ class ObjectDBProfilerPanel : public Control {
 	void receive_snapshot(int p_request_id);
 	void show_snapshot(const String &p_snapshot_file_name, const String &p_snapshot_diff_file_name);
 	void clear_snapshot(bool p_update_view_tabs = true);
-	Ref<GameStateSnapshotRef> get_snapshot(const String &p_snapshot_file_name);
+	Ref<GameStateSnapshot> get_snapshot(const String &p_snapshot_file_name);
 	void set_enabled(bool p_enabled);
 	void add_view(SnapshotView *p_to_add);
 

modules/objectdb_profiler/editor/snapshot_data.cpp

@@ -318,11 +318,9 @@ void GameStateSnapshot::recompute_references() {
 	}
 }
 
-Ref<GameStateSnapshotRef> GameStateSnapshot::create_ref(const String &p_snapshot_name, const Vector<uint8_t> &p_snapshot_buffer) {
-	// A ref to a refcounted object which is a wrapper of a non-refcounted object.
-	Ref<GameStateSnapshotRef> sn;
-	sn.instantiate(memnew(GameStateSnapshot));
-	GameStateSnapshot *snapshot = sn->get_snapshot();
+Ref<GameStateSnapshot> GameStateSnapshot::create_ref(const String &p_snapshot_name, const Vector<uint8_t> &p_snapshot_buffer) {
+	Ref<GameStateSnapshot> snapshot;
+	snapshot.instantiate();
 	snapshot->name = p_snapshot_name;
 
 	// Snapshots may have been created by an older version of the editor. Handle parsing old snapshot versions here based on the version number.
@@ -347,29 +345,17 @@ Ref<GameStateSnapshotRef> GameStateSnapshot::create_ref(const String &p_snapshot
 			continue;
 		}
 
-		snapshot->objects[obj.id] = memnew(SnapshotDataObject(obj, snapshot, resource_cache));
+		snapshot->objects[obj.id] = memnew(SnapshotDataObject(obj, snapshot.ptr(), resource_cache));
 		snapshot->objects[obj.id]->extra_debug_data = (Dictionary)snapshot_data[i + 3];
 	}
 
 	snapshot->recompute_references();
 	print_verbose("Resource cache hits: " + String::num(resource_cache.hits) + ". Resource cache misses: " + String::num(resource_cache.misses));
-	return sn;
+	return snapshot;
 }
 
 GameStateSnapshot::~GameStateSnapshot() {
 	for (const KeyValue<ObjectID, SnapshotDataObject *> &item : objects) {
 		memdelete(item.value);
 	}
 }
-
-bool GameStateSnapshotRef::unreference() {
-	bool die = RefCounted::unreference();
-	if (die) {
-		memdelete(gamestate_snapshot);
-	}
-	return die;
-}
-
-GameStateSnapshot *GameStateSnapshotRef::get_snapshot() {
-	return gamestate_snapshot;
-}

modules/objectdb_profiler/editor/snapshot_data.h

@@ -33,7 +33,6 @@
 #include "editor/debugger/editor_debugger_inspector.h"
 
 class GameStateSnapshot;
-class GameStateSnapshotRef;
 
 class SnapshotDataObject : public Object {
 	GDCLASS(SnapshotDataObject, Object);
@@ -77,8 +76,8 @@ class SnapshotDataObject : public Object {
 	static void _bind_methods();
 };
 
-class GameStateSnapshot : public Object {
-	GDCLASS(GameStateSnapshot, Object);
+class GameStateSnapshot : public RefCounted {
+	GDCLASS(GameStateSnapshot, RefCounted);
 
 	void _get_outbound_references(Variant &p_var, HashMap<String, ObjectID> &r_ret_val, const String &p_current_path = "");
 	void _get_rc_cycles(SnapshotDataObject *p_obj, SnapshotDataObject *p_source_obj, HashSet<SnapshotDataObject *> p_traversed_objs, LocalVector<String> &r_ret_val, const String &p_current_path = "");
@@ -88,24 +87,8 @@ class GameStateSnapshot : public Object {
 	HashMap<ObjectID, SnapshotDataObject *> objects;
 	Dictionary snapshot_context;
 
-	// Ideally, this would extend EditorDebuggerRemoteObject and be refcounted, but we can't have it both ways.
-	// So, instead we have this static 'constructor' that returns a RefCounted wrapper around a GameStateSnapshot.
-	static Ref<GameStateSnapshotRef> create_ref(const String &p_snapshot_name, const Vector<uint8_t> &p_snapshot_buffer);
+	static Ref<GameStateSnapshot> create_ref(const String &p_snapshot_name, const Vector<uint8_t> &p_snapshot_buffer);
 	~GameStateSnapshot();
 
 	void recompute_references();
 };
-
-// Thin RefCounted wrapper around a GameStateSnapshot.
-class GameStateSnapshotRef : public RefCounted {
-	GDCLASS(GameStateSnapshotRef, RefCounted);
-
-	GameStateSnapshot *gamestate_snapshot = nullptr;
-
-public:
-	GameStateSnapshotRef(GameStateSnapshot *p_gss) :
-			gamestate_snapshot(p_gss) {}
-
-	bool unreference();
-	GameStateSnapshot *get_snapshot();
-};