From 71426d0f5cc56a436be23a921f6972dd62e49ce5 Mon Sep 17 00:00:00 2001 From: David Luevano Alvarado Date: Tue, 5 Mar 2024 22:45:39 -0600 Subject: [PATCH] Fix wrong undo-redo action when dropping files containing circular dependencies --- editor/plugins/canvas_item_editor_plugin.cpp | 37 ++++++++++----- editor/plugins/canvas_item_editor_plugin.h | 2 +- editor/plugins/node_3d_editor_plugin.cpp | 47 +++++++++++++------- editor/plugins/node_3d_editor_plugin.h | 2 +- 4 files changed, 58 insertions(+), 30 deletions(-) diff --git a/editor/plugins/canvas_item_editor_plugin.cpp b/editor/plugins/canvas_item_editor_plugin.cpp index 2ab53effed7..b3a4362eeac 100644 --- a/editor/plugins/canvas_item_editor_plugin.cpp +++ b/editor/plugins/canvas_item_editor_plugin.cpp @@ -5695,8 +5695,11 @@ void CanvasItemEditorViewport::_create_preview(const Vector &files) cons } void CanvasItemEditorViewport::_remove_preview() { - if (preview_node->get_parent()) { + if (!canvas_item_editor->message.is_empty()) { canvas_item_editor->message = ""; + canvas_item_editor->update_viewport(); + } + if (preview_node->get_parent()) { for (int i = preview_node->get_child_count() - 1; i >= 0; i--) { Node *node = preview_node->get_child(i); node->queue_free(); @@ -5709,7 +5712,7 @@ void CanvasItemEditorViewport::_remove_preview() { } } -bool CanvasItemEditorViewport::_cyclical_dependency_exists(const String &p_target_scene_path, Node *p_desired_node) { +bool CanvasItemEditorViewport::_cyclical_dependency_exists(const String &p_target_scene_path, Node *p_desired_node) const { if (p_desired_node->get_scene_file_path() == p_target_scene_path) { return true; } @@ -5853,7 +5856,7 @@ void CanvasItemEditorViewport::_perform_drop_data() { return; } - Vector error_files; + PackedStringArray error_files; EditorUndoRedoManager *undo_redo = EditorUndoRedoManager::get_singleton(); undo_redo->create_action_for_history(TTR("Create Node"), EditorNode::get_editor_data().get_current_edited_scene_history_id()); @@ -5872,12 +5875,12 @@ void CanvasItemEditorViewport::_perform_drop_data() { // Without root node act the same as "Load Inherited Scene" Error err = EditorNode::get_singleton()->load_scene(path, false, true); if (err != OK) { - error_files.push_back(path); + error_files.push_back(path.get_file()); } } else { bool success = _create_instance(target_node, path, drop_pos); if (!success) { - error_files.push_back(path); + error_files.push_back(path.get_file()); } } } else { @@ -5893,12 +5896,7 @@ void CanvasItemEditorViewport::_perform_drop_data() { undo_redo->commit_action(); if (error_files.size() > 0) { - String files_str; - for (int i = 0; i < error_files.size(); i++) { - files_str += error_files[i].get_file().get_basename() + ","; - } - files_str = files_str.substr(0, files_str.length() - 1); - accept->set_text(vformat(TTR("Error instantiating scene from %s"), files_str.get_data())); + accept->set_text(vformat(TTR("Error instantiating scene from %s."), String(", ").join(error_files))); accept->popup_centered(); } } @@ -5912,6 +5910,8 @@ bool CanvasItemEditorViewport::can_drop_data(const Point2 &p_point, const Varian Vector files = d["files"]; bool can_instantiate = false; + bool is_cyclical_dep = false; + String error_file; // Check if at least one of the dragged files is a texture or scene. for (int i = 0; i < files.size(); i++) { @@ -5929,12 +5929,25 @@ bool CanvasItemEditorViewport::can_drop_data(const Point2 &p_point, const Varian if (!instantiated_scene) { continue; } + + Node *edited_scene = EditorNode::get_singleton()->get_edited_scene(); + if (_cyclical_dependency_exists(edited_scene->get_scene_file_path(), instantiated_scene)) { + memdelete(instantiated_scene); + can_instantiate = false; + is_cyclical_dep = true; + error_file = files[i].get_file(); + break; + } memdelete(instantiated_scene); } can_instantiate = true; - break; } } + if (is_cyclical_dep) { + canvas_item_editor->message = vformat(TTR("Can't instantiate: %s."), vformat(TTR("Circular dependency found at %s"), error_file)); + canvas_item_editor->update_viewport(); + return false; + } if (can_instantiate) { if (!preview_node->get_parent()) { // create preview only once _create_preview(files); diff --git a/editor/plugins/canvas_item_editor_plugin.h b/editor/plugins/canvas_item_editor_plugin.h index 723dbc7f593..46c68460554 100644 --- a/editor/plugins/canvas_item_editor_plugin.h +++ b/editor/plugins/canvas_item_editor_plugin.h @@ -645,7 +645,7 @@ class CanvasItemEditorViewport : public Control { void _create_preview(const Vector &files) const; void _remove_preview(); - bool _cyclical_dependency_exists(const String &p_target_scene_path, Node *p_desired_node); + bool _cyclical_dependency_exists(const String &p_target_scene_path, Node *p_desired_node) const; bool _only_packed_scenes_selected() const; void _create_nodes(Node *parent, Node *child, String &path, const Point2 &p_point); bool _create_instance(Node *parent, String &path, const Point2 &p_point); diff --git a/editor/plugins/node_3d_editor_plugin.cpp b/editor/plugins/node_3d_editor_plugin.cpp index f876d9ebd17..0d88cba590f 100644 --- a/editor/plugins/node_3d_editor_plugin.cpp +++ b/editor/plugins/node_3d_editor_plugin.cpp @@ -4326,7 +4326,7 @@ void Node3DEditorViewport::_remove_preview_material() { spatial_editor->set_preview_material_surface(-1); } -bool Node3DEditorViewport::_cyclical_dependency_exists(const String &p_target_scene_path, Node *p_desired_node) { +bool Node3DEditorViewport::_cyclical_dependency_exists(const String &p_target_scene_path, Node *p_desired_node) const { if (p_desired_node->get_scene_file_path() == p_target_scene_path) { return true; } @@ -4437,7 +4437,7 @@ void Node3DEditorViewport::_perform_drop_data() { _remove_preview_node(); - Vector error_files; + PackedStringArray error_files; undo_redo->create_action(TTR("Create Node"), UndoRedo::MERGE_DISABLE, target_node); undo_redo->add_do_method(editor_selection, "clear"); @@ -4453,7 +4453,7 @@ void Node3DEditorViewport::_perform_drop_data() { if (mesh != nullptr || scene != nullptr) { bool success = _create_instance(target_node, path, drop_pos); if (!success) { - error_files.push_back(path); + error_files.push_back(path.get_file()); } } } @@ -4461,12 +4461,7 @@ void Node3DEditorViewport::_perform_drop_data() { undo_redo->commit_action(); if (error_files.size() > 0) { - String files_str; - for (int i = 0; i < error_files.size(); i++) { - files_str += error_files[i].get_file().get_basename() + ","; - } - files_str = files_str.substr(0, files_str.length() - 1); - accept->set_text(vformat(TTR("Error instantiating scene from %s"), files_str.get_data())); + accept->set_text(vformat(TTR("Error instantiating scene from %s."), String(", ").join(error_files))); accept->popup_centered(); } } @@ -4475,12 +4470,17 @@ bool Node3DEditorViewport::can_drop_data_fw(const Point2 &p_point, const Variant preview_node_viewport_pos = p_point; bool can_instantiate = false; + bool is_cyclical_dep = false; + String error_file; if (!preview_node->is_inside_tree() && spatial_editor->get_preview_material().is_null()) { Dictionary d = p_data; if (d.has("type") && (String(d["type"]) == "files")) { Vector files = d["files"]; + // Track whether a type other than PackedScene is valid to stop checking them and only + // continue to check if the rest of the scenes are valid (don't have cyclic dependencies). + bool is_other_valid = false; // Check if at least one of the dragged files is a mesh, material, texture or scene. for (int i = 0; i < files.size(); i++) { bool is_scene = ClassDB::is_parent_class(ResourceLoader::get_resource_type(files[i]), "PackedScene"); @@ -4502,30 +4502,40 @@ bool Node3DEditorViewport::can_drop_data_fw(const Point2 &p_point, const Variant if (!instantiated_scene) { continue; } + Node *edited_scene = EditorNode::get_singleton()->get_edited_scene(); + if (_cyclical_dependency_exists(edited_scene->get_scene_file_path(), instantiated_scene)) { + memdelete(instantiated_scene); + can_instantiate = false; + is_cyclical_dep = true; + error_file = files[i].get_file(); + break; + } memdelete(instantiated_scene); - } else if (mat.is_valid()) { + } else if (!is_other_valid && mat.is_valid()) { Ref base_mat = res; Ref shader_mat = res; if (base_mat.is_null() && !shader_mat.is_null()) { - break; + continue; } spatial_editor->set_preview_material(mat); - break; - } else if (mesh.is_valid()) { + is_other_valid = true; + continue; + } else if (!is_other_valid && mesh.is_valid()) { // Let the mesh pass. - } else if (tex.is_valid()) { + is_other_valid = true; + } else if (!is_other_valid && tex.is_valid()) { Ref new_mat = memnew(StandardMaterial3D); new_mat->set_texture(BaseMaterial3D::TEXTURE_ALBEDO, tex); spatial_editor->set_preview_material(new_mat); - break; + is_other_valid = true; + continue; } else { continue; } can_instantiate = true; - break; } } if (can_instantiate) { @@ -4539,6 +4549,11 @@ bool Node3DEditorViewport::can_drop_data_fw(const Point2 &p_point, const Variant } } + if (is_cyclical_dep) { + set_message(vformat(TTR("Can't instantiate: %s."), vformat(TTR("Circular dependency found at %s"), error_file))); + return false; + } + if (can_instantiate) { update_preview_node = true; return true; diff --git a/editor/plugins/node_3d_editor_plugin.h b/editor/plugins/node_3d_editor_plugin.h index 13b51289a95..091432b35a2 100644 --- a/editor/plugins/node_3d_editor_plugin.h +++ b/editor/plugins/node_3d_editor_plugin.h @@ -444,7 +444,7 @@ private: bool _apply_preview_material(ObjectID p_target, const Point2 &p_point) const; void _reset_preview_material() const; void _remove_preview_material(); - bool _cyclical_dependency_exists(const String &p_target_scene_path, Node *p_desired_node); + bool _cyclical_dependency_exists(const String &p_target_scene_path, Node *p_desired_node) const; bool _create_instance(Node *parent, String &path, const Point2 &p_point); void _perform_drop_data();