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

Add support for resource conversion plugins in filesystem dock #65585

Merged
merged 1 commit into from
Sep 17, 2024

Conversation

SaracenOne
Copy link
Member

@SaracenOne SaracenOne commented Sep 9, 2022

Based on a past discussion for a long overdue UX feature. Makes ResourceConversionPlugins avaliable to the filesystem dock, including allowing multiple files to be batch converted in one go and updating the usage of said resources in active scenes. Keeping in draft for a bit longer since I want to look at revising some of the API naming around detecting viable conversion plugins based purely on class name to avoid having to load the resource beforehand, and possibly look into introducing a warning dialogue explaining how conversion of types is a one-way operation.

That being said, I think the majority of the implementation is fine.

OLS4nq9wfq

@Chaosus Chaosus added this to the 4.0 milestone Sep 12, 2022
@YuriSizov YuriSizov modified the milestones: 4.0, 4.1 Feb 10, 2023
@YuriSizov YuriSizov modified the milestones: 4.1, 4.x Jun 14, 2023
@SaracenOne SaracenOne force-pushed the filesystem_dock_conversion branch 2 times, most recently from 74d4782 to 069075c Compare October 15, 2023 11:43
@SaracenOne
Copy link
Member Author

SaracenOne commented Oct 15, 2023

Decided to revisit this and add the remaining things I wanted to add because, to be frank, the current workflow for converting standard materials to shader materials is kind of atrocious. It now gives you warning when you attempt to convert files from the filesystem dock, and although there are currently no instances where multiple conversion types are avaliable, it will now collapse the conversion choices into a submenu if more than one is present in the future.

godot windows editor dev x86_64_E4Nv8kHpBS

I'm not happy about the fact that I had to add a new (and fairly reduntant) function to the API. I would have much rathered change 'handles' to a string type rather than a resource type, but I realize that doing so would break the API and subsequently break GDExtensions. As such, another function is added to ResourceConverterPlugins to check handling by type name rather than resource, meaning the resources won't have to be loaded when simply right-clicking on them

@SaracenOne SaracenOne marked this pull request as ready for review October 15, 2023 11:47
@SaracenOne SaracenOne requested review from a team as code owners October 15, 2023 11:47
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Dedicated icon syntax

@SaracenOne SaracenOne force-pushed the filesystem_dock_conversion branch 2 times, most recently from 817864a to c4ffc5d Compare October 15, 2023 12:13
@SaracenOne
Copy link
Member Author

Okay thanks, implemented changes recommend by @AThousandShips

@SaracenOne SaracenOne force-pushed the filesystem_dock_conversion branch 2 times, most recently from 79a2c86 to 3ed43aa Compare October 15, 2023 12:57
@SaracenOne
Copy link
Member Author

Okay, updated now

@KoBeWi
Copy link
Member

KoBeWi commented Oct 16, 2023

I think handles() takes a whole object to allow filtering objects by more than only a type (although it's not really used in practice).

You can use this trick to achieve the same functionality with a low cost:

	Ref<Resource> temp = ClassDB::instantiate(p_type);
	for (int i = 0; i < resource_conversion_plugins.size(); i++) {
		if (resource_conversion_plugins[i].is_valid() && resource_conversion_plugins[i]->handles(temp)) {
			ret.push_back(resource_conversion_plugins[i]);
		}
	}

Instantiating an empty resource is much cheaper than loading it.

@SaracenOne
Copy link
Member Author

Okay @KoBeWi and @AThousandShips, I think that should address most of the issues raised. One I'm not sure about is @KoBeWi's point about setting conversion dialog text in the constructor.

@SaracenOne SaracenOne force-pushed the filesystem_dock_conversion branch from 96a41a9 to 9f3e20c Compare September 15, 2024 19:45
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

The feature works correctly. I left some more style comments, but the code is good enough to be fixed in a follow-up.

@SaracenOne SaracenOne force-pushed the filesystem_dock_conversion branch from 9f3e20c to f44bce2 Compare September 16, 2024 14:12
@SaracenOne
Copy link
Member Author

Updated again! @KoBeWi I wasn't sure if that was what you meant by an 'early return', but can you confirm that the change I made was what you were suggesting?

@KoBeWi
Copy link
Member

KoBeWi commented Sep 16, 2024

Yes. You could also do return ret; to return empty Vector.

@akien-mga akien-mga merged commit 13064c4 into godotengine:master Sep 17, 2024
20 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

6 participants