Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix wrong undo-redo action when dropping files containing circular dependencies #89204

Merged
merged 1 commit into from
Mar 8, 2024
Merged

Fix wrong undo-redo action when dropping files containing circular dependencies #89204

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 25 additions & 12 deletions editor/plugins/canvas_item_editor_plugin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5695,8 +5695,11 @@ void CanvasItemEditorViewport::_create_preview(const Vector<String> &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();
Expand All @@ -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;
}
Expand Down Expand Up @@ -5853,7 +5856,7 @@ void CanvasItemEditorViewport::_perform_drop_data() {
return;
}

Vector<String> 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());
Expand All @@ -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 {
Expand All @@ -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();
}
}
Expand All @@ -5912,6 +5910,8 @@ bool CanvasItemEditorViewport::can_drop_data(const Point2 &p_point, const Varian

Vector<String> 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++) {
Expand All @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion editor/plugins/canvas_item_editor_plugin.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -645,7 +645,7 @@ class CanvasItemEditorViewport : public Control {
void _create_preview(const Vector<String> &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);
Expand Down
47 changes: 31 additions & 16 deletions editor/plugins/node_3d_editor_plugin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -4437,7 +4437,7 @@ void Node3DEditorViewport::_perform_drop_data() {

_remove_preview_node();

Vector<String> 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");
Expand All @@ -4453,20 +4453,15 @@ 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());
}
}
}

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();
}
}
Expand All @@ -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<String> 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");
Expand All @@ -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<BaseMaterial3D> base_mat = res;
Ref<ShaderMaterial> 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<StandardMaterial3D> 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) {
Expand All @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion editor/plugins/node_3d_editor_plugin.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,7 +444,7 @@ class Node3DEditorViewport : public Control {
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();

Expand Down
Loading