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

Respect order of extensions for ResourceFormatSavers with at_front #101543

Conversation

MrJoermungandr
Copy link
Contributor

This fixes a long standing behaviour issue for ResourceFormatSavers defined for extensions.
Since 4.4 beta is around the corner I have provided the minimal possible fix because this would unblock my usecase for a plugin im working on so im really hoping this can make it in.

The explanation below is for documenting the current issues with the system overall which got more and more convoluted over time and 1 time fixes have accumulated. If anyone thinks after reading this that more of the code in question should change for a "proper" fix or streamlining behaviour they are happily invited to take over this pr because i'm limited on time currently :)

1. The Problem

The ResourceSaver class allows adding more ResourceFormatSavers to add serialization for custom resources.
For GDExtension this is a manual process so you need to call the add_resource_format_saver() method.
This method allows you to pass the optional parameter bool at_front = false.
What this does is not documented but after looking through the source code i discouvered that this adds the Saver at the front ;) but what does that mean?
All savers registered are stored in a list.
Per default we append to the list if a new saver is registered.

When adding a new reource the editor always iterates over every saver in the list (in order) and all available savers get selected. The Savers for .tres and .res are always available. at_front assures that your custom saver is added before mentioned savers so .yourextension - defined by your custom saver - also gets collected first in the list when available savers get requested.

Therefore i determined the intended usecase for this option was to provide a way to opt out of the default behaviour in case you want to ignore the default savers.
NOTE: you can still manually select .tres and it will let you but this at least changes the default selected option.

Sadly it is not always desired that a custom resource can be saved with the default savers. This is also the case for gdscript files where only .gd is valid.

The problem this pr addresses is that the deafult behaviour does not work when at_front is selected.

2. Minimal fix

When creating a new script (through the add resource dialog) you can see that gdscript managed to only make the .gd extension available.
But how does it do so?
Let`s look at the (simplified) function that get's called when adding a resource:

List<String> extensions;
ResourceSaver::get_recognized_extensions(p_resource, &extensions);

List<String> preferred;
for (const String &E : extensions) {
    if (p_resource->is_class("Script") && (E == "tres" || E == "res")) {
        continue; // <- GDScript gets special treatment here
    }
    file->add_filter("*." + E, E.to_upper());
    preferred.push_back(E);
}

List<String>::Element *res_element = preferred.find("res");
if (res_element) {
    preferred.move_to_back(res_element);
}
List<String>::Element *tres_element = preferred.find("tres");
if (tres_element) {
    preferred.move_to_front(tres_element);
}

if (!preferred.is_empty()) {
    file->set_current_file("new_" + resource_name_snake_case + "." + preferred.front()->get().to_lower());
}

As you can see above the function manually deletes those options for GDScript which is imo not great but whatever.

In the last line the first option also gets selected to be injected in the file dialog which is good on its own. However in the 2 list calls from before we we manually and everytime move the .tres saver to the front rendering the at_front option useless.

It is actually enough to just move .res to the back since this already ensures that .tres is selected if no other savers are present - thats also what the pr does providing the easiest fix.

This ensures that when this Saver was added with at_front the default behaviour for the handled resource is to be saved with the custom extension while keeping the behaviour for e.g StandartMaterial3D the same since .material is a valid extension but because not at_front .tres is still the default.

3. Streamlining accumulated hacks

If your familiar with godot the gdshader resouce has a similar treatment so how is this handled?
The filesystem_dock handles the calls to create a new resource with resource_created. This is the (simplified) function:

	String type_name = new_resource_dialog->get_selected_type();
	if (type_name == "Shader") {
		make_shader_dialog->config(fpath.path_join("new_shader"), false, false, 0);
		make_shader_dialog->popup_centered();
		return;
	}

    EditorNode::get_singleton()->save_resource_as(Ref<Resource>(r), fpath);

As you can see even if selected in the Add Resource dialog there already exists a "system" to intercept these calls and provide the right add dialog.
It is therefore possible to add Script below Shader and get rid of the ifin the function i mentioned before eliminating the special case.

When looking at the full first function there also are some checks that could be simplified afterwards but this is not critical.
One may also document the current behaviour of the at_front parameter if desired.

In the future it is maybe also desired to just rename the bool to exclusive and enable completly disabling built in Savers for custom resources. But this is out of scope for this pr and most likely requires further discussion.

This is my first godot pr so lemme know if i f*ed anything up in the process. ^^
Also i'm realizing that the last part would fit into a proposal format, would be great to know if i should do that.

Thanks in advance for your time yall!

@MrJoermungandr
Copy link
Contributor Author

@Chaosus should also be tagged as a bug and is safe for 4.4 :) . Maybe @KoBeWi can take a look (since i observed that you did some stuff for edtior and uids)

@akien-mga akien-mga changed the title Respect order of extensions for ResourceFormatSavers with at_front Respect order of extensions for ResourceFormatSavers with at_front Jan 15, 2025
@fire
Copy link
Member

fire commented Jan 17, 2025

The problem this pr addresses is that the default behaviour does not work when at_front is selected.

Test plan

Verify that at_front is enabled in add_resource_format_saver, ignoring the default savers.

Misc

I looked into bool at_front a little, and it was added by reduz in commit 79a7473

@fire fire requested a review from a team January 17, 2025 20:12
@fire
Copy link
Member

fire commented Jan 17, 2025

@lyuma Can you look at this? This isn't very clear.

@MrJoermungandr
Copy link
Contributor Author

MrJoermungandr commented Jan 17, 2025

Hey fire thanks for responding :)
Without my addition it works as expected - the ResourceLoader get registered at_front.
The problem lies in the way the editor processess this information.

Without my addition

Note that in the filters it gets processed correctly. But the preinserted default name is still .tres and if only typing the filter gets ignored.
image

With my addition

The default extension also gets set correctly
image

This happens because of the prefered List local variable introduced in the function showed under 2. Minimal fix in my original comment :)

This merely stop the editor from forcing .tres to the front again - hence i labeled it as a bug ^^

(I also just realized that my original comment was not explicit enough about what the problem actually is - mb)

@KoBeWi
Copy link
Member

KoBeWi commented Jan 26, 2025

Can you provide some minimal project to test this? (i.e. one with custom saver)

@MrJoermungandr
Copy link
Contributor Author

MrJoermungandr commented Jan 26, 2025

@KoBeWi thats really difficult in this case since for gdscript it auto registers loaders so im not sure how i would add them outside of a gdextension (with the at_front parameter) but if you write me in rocket chat in the asset_pipeline channel i could hop on a call anytime tomorrow to show it with my extension idk ^^

Edit: i compiled a minimal extension to a dll and gdextension file that you should be able to drag into any new project
The resource you have to add is called TestResourceForSaver
lib.zip

you can also test it with a material afterwards to verify the bahaviour hasnt changed for the build in ones :)

@KoBeWi
Copy link
Member

KoBeWi commented Jan 31, 2025

Looks similar to #99987
And yeah, it's a bug. I checked and only Shader format is using at_front, and it seems unchanged (didn't require fix). The attached extension gets fixed.

@KoBeWi KoBeWi added bug and removed enhancement labels Jan 31, 2025
@KoBeWi KoBeWi modified the milestones: 4.x, 4.4 Jan 31, 2025
@Repiteo Repiteo merged commit 1bec1bf into godotengine:master Feb 3, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Feb 3, 2025

Thanks! Congratulations on your first merged contribution! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

5 participants