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.

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

Allow to pick which Resources will be made unique #77855

Merged

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Jun 4, 2023

Closes godotengine/godot-proposals#7009

godot.windows.editor.dev.x86_64_GvhuVWmTln.mp4

"Make Unique (Recursive)" option will now open a popup which allows you to select which resources will be made unique. By default all resources are selected, except Image, Mesh and Shader. (more exceptions can be added, feel free to suggest them)

@KoBeWi KoBeWi added this to the 4.x milestone Jun 4, 2023
@KoBeWi KoBeWi force-pushed the the_inevitable_heat_death_of_the_universe branch 3 times, most recently from 29ba4e3 to 6ed2075 Compare June 4, 2023 22:30
@akien-mga akien-mga requested a review from YuriSizov June 5, 2023 16:38
@YuriSizov YuriSizov modified the milestones: 4.x, 4.2 Jun 6, 2023
@YuriSizov YuriSizov requested a review from a team June 6, 2023 12:14
Copy link
Member

@groud groud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are too fast! From a usability standpoint, looks quite good to me.

Maybe the only thing I would add is the disabling of the menu option if the resource has no sub-resource, but that can be done in another PR.

@Calinou

This comment was marked as resolved.

@KoBeWi KoBeWi force-pushed the the_inevitable_heat_death_of_the_universe branch from 6ed2075 to e23f6aa Compare June 6, 2023 13:02
@YuriSizov
Copy link
Contributor

Maybe the only thing I would add is the disabling of the menu option if the resource has no sub-resource, but that can be done in another PR.

You mean instead of completely hiding it?

@groud
Copy link
Member

groud commented Jun 6, 2023

You mean instead of completely hiding it?

Hmm, not sure I understand. I've only checked on 4.0.3 where the entry is not hidden when the resource has no sub-resource. Maybe that was changed though ?

Peek.06-06-2023.17-20.mp4

@YuriSizov
Copy link
Contributor

Hmm, not sure I understand. I've only checked on 4.0.3 where the entry is not hidden when the resource has no sub-resource. Maybe that was changed though ?

No, it was implemented with a check, but perhaps the check is faulty?

https://github.com/godotengine/godot/blob/e23f6aa75c46be7419a8f8e7f52821a5f1f8a1be/editor/editor_resource_picker.cpp#L221-L230

@KoBeWi
Copy link
Member Author

KoBeWi commented Jun 6, 2023

The check only checks if the property exists, disregarding whether it was assigned. So it regards null as existing sub-resource. I can fix that.

@KoBeWi KoBeWi force-pushed the the_inevitable_heat_death_of_the_universe branch 2 times, most recently from 733e216 to 481ba4b Compare June 7, 2023 14:51
Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation looks fine (a couple nitpicks though), but usability needs to be improved a bit.

Currently, it's unclear which resources you are selecting for two reasons:

  • We don't display file paths for external resources; we either display types or types + names;
  • We don't display which field each resource belongs to.
godot windows editor dev x86_64_2023-07-14_20-59-36

Here's what I think it could look instead:

godot windows editor dev x86_64_2023-07-14_21-10-08

Here's a potential diff that you can use:

diff --git a/editor/editor_resource_picker.cpp b/editor/editor_resource_picker.cpp
index bfeff41a4a..da78566848 100644
--- a/editor/editor_resource_picker.cpp
+++ b/editor/editor_resource_picker.cpp
@@ -378,6 +378,7 @@ void EditorResourcePicker::_edit_menu_cbk(int p_which) {
 
 				duplicate_resources_tree = memnew(Tree);
 				vb->add_child(duplicate_resources_tree);
+				duplicate_resources_tree->set_columns(2);
 				duplicate_resources_tree->set_v_size_flags(SIZE_EXPAND_FILL);
 			}
 
@@ -386,7 +387,7 @@ void EditorResourcePicker::_edit_menu_cbk(int p_which) {
 			_gather_resources_to_duplicate(edited_resource, root);
 
 			duplicate_resources_dialog->reset_size();
-			duplicate_resources_dialog->popup_centered(Vector2(300, 400) * EDSCALE);
+			duplicate_resources_dialog->popup_centered(Vector2(500, 400) * EDSCALE);
 
 			Ref<Resource> unique_resource = edited_resource->duplicate(true);
 		} break;
@@ -946,21 +947,31 @@ void EditorResourcePicker::_ensure_resource_menu() {
 	edit_menu->connect("popup_hide", callable_mp((BaseButton *)edit_button, &BaseButton::set_pressed).bind(false));
 }
 
-void EditorResourcePicker::_gather_resources_to_duplicate(const Ref<Resource> p_resource, TreeItem *p_item) const {
+void EditorResourcePicker::_gather_resources_to_duplicate(const Ref<Resource> p_resource, TreeItem *p_item, String p_resource_name) const {
 	p_item->set_cell_mode(0, TreeItem::CELL_MODE_CHECK);
 
-	const String res_name = p_resource->get_name();
+	String res_name = p_resource->get_name();
+	if (res_name.is_empty() && p_resource->get_path().is_resource_file()) {
+		res_name = p_resource->get_path().get_file();
+	}
+
 	if (res_name.is_empty()) {
 		p_item->set_text(0, p_resource->get_class());
 	} else {
 		p_item->set_text(0, vformat("%s (%s)", p_resource->get_class(), res_name));
 	}
+
 	p_item->set_icon(0, EditorNode::get_singleton()->get_object_icon(p_resource.ptr()));
 	p_item->set_editable(0, true);
+
 	Array meta;
 	meta.append(p_resource);
 	p_item->set_metadata(0, meta);
 
+	if (!p_resource_name.is_empty()) {
+		p_item->set_text(1, p_resource_name);
+	}
+
 	if (p_resource->get_class() != "Image" && p_resource->get_class() != "Shader" && p_resource->get_class() != "Mesh" && p_resource->get_class() != "FontFile") {
 		// Automatically select resource, unless it's something that shouldn't be duplicated.
 		p_item->set_checked(0, true);
@@ -980,7 +991,7 @@ void EditorResourcePicker::_gather_resources_to_duplicate(const Ref<Resource> p_
 		}
 
 		TreeItem *child = p_item->create_child();
-		_gather_resources_to_duplicate(res, child);
+		_gather_resources_to_duplicate(res, child, E.name);
 
 		meta = child->get_metadata(0);
 		// Remember property name.
diff --git a/editor/editor_resource_picker.h b/editor/editor_resource_picker.h
index 43b6bd7036..897312de7b 100644
--- a/editor/editor_resource_picker.h
+++ b/editor/editor_resource_picker.h
@@ -105,7 +105,7 @@ class EditorResourcePicker : public HBoxContainer {
 	void drop_data_fw(const Point2 &p_point, const Variant &p_data, Control *p_from);
 
 	void _ensure_resource_menu();
-	void _gather_resources_to_duplicate(const Ref<Resource> p_resource, TreeItem *p_item) const;
+	void _gather_resources_to_duplicate(const Ref<Resource> p_resource, TreeItem *p_item, const String p_property_name = "") const;
 	void _duplicate_selected_resources();
 
 protected:

editor/editor_resource_picker.cpp Outdated Show resolved Hide resolved
editor/editor_resource_picker.cpp Outdated Show resolved Hide resolved
Co-authored-by: Yuri Sizov <yuris@humnom.net>
@KoBeWi KoBeWi force-pushed the the_inevitable_heat_death_of_the_universe branch from 481ba4b to 6276fd2 Compare July 14, 2023 21:00
@KoBeWi
Copy link
Member Author

KoBeWi commented Jul 14, 2023

@YuriSizov Thanks, I used your patch and added you as co-author.

p_item->set_cell_mode(0, TreeItem::CELL_MODE_CHECK);

String res_name = p_resource->get_name();
if (res_name.is_empty() && !p_resource->is_built_in()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the check I've suggested originally follows a similar check used in a few places of this file, including a similar resolution for the assign_button. But I suppose this should also work.

@YuriSizov YuriSizov merged commit 93d180b into godotengine:master Jul 16, 2023
13 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@KoBeWi KoBeWi deleted the the_inevitable_heat_death_of_the_universe branch July 16, 2023 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow users to pick which Resources will be made unique when using "Make Unique (Recursive)"
4 participants