Fix custom resource icons not appearing in file system #60606

Atlinx · 2022-04-29T01:18:43Z

This PR was implemented using the advice given in my original PR (#51089) along with suggestions made by @willnationsdev. It uses the existing file_cache inside of editor_file_system.cpp to cache the script attached to each resource. Additionally, this PR manually inspects each resource file for the ext_resource tag of type Script when fetching the script attached to that resource. This means we avoid unnecessary loading of subresources, etc.

Note that this implementation supports changing icons while inside the editor -- the changes will reflect when the user clicks on the filesystem again after setting a new icon.

Before

After

Atlinx · 2022-04-29T03:11:41Z

Ok, I've found a bug where the icons break if you move the resource's script. It's likely due to resource_script_path not updating on file system change. I'll probably have to find some way to update all instances of a resource when their scripts change.

Does anyone know how Godot currently handles fixing dependencies when files are moved/deleted? I can't seem to find where that behavior is implemented, and I don't want to reinvent the wheel if it already exists.

From my testing, Godot is creating a .tres.depren file whenever I move a dependency around. However, the original .tres file still remains incorrect. Is this intended behavior? If so, what is the purpose of .tres.depren files?

Atlinx · 2022-04-30T01:35:39Z

I've refactored my code to be backward compatible and removed any edits to core files.

Regarding the .depren issue, it appears .depren is something that's supposed to exist as a temporary file. I cannot reproduce this issue of lingering .depren files on the current v3.4.3 version of Godot when I drag the scripts/resources around.

Maybe that has something to do with #60412?

willnationsdev

I don't know yet whether the deps stuff is an adequate solution (cause I myself haven't researched that much), but I'll probably do some experimenting and get back to you if I discover any problems. I also still need to actually test that it works as expected, hehe. Otherwise, aside from the script and inheritance hardcoding, this is looking like a simple and elegant solution. Thanks!

willnationsdev · 2022-04-30T02:32:08Z

editor/editor_file_system.cpp

+String EditorFileSystemDirectory::get_file_attached_script(int p_idx) const {
+	Vector<String> deps = get_file_deps(p_idx);
+	for (int i = 0; i < deps.size(); i++) {
+		if (deps[i].ends_with(".gd") || deps[i].ends_with(".cs")) {


Rather than checking for specific languages' extensions, it's better to use ResourceLoader::get_resource_type(p_path) to extract a typename and then use an inheritance check to see if it inherits from the Script class. If so, you know it's a script, regardless of what language it is.

You can probably assume that a user-defined type WON'T be an implementation of a Script, so you could skip it, but ordinarily, you would first check ScriptServer::is_global_class() and then based on that either use EditorNode::get_editor_data().script_class_is_parent() if true or ClassDB::is_parent_class() if false. Note also that you can only use the former if you are in an editor context. If it's something that would be running in a live application outside the editor, then you have to fully load the script and then do script->get_language()->get_global_class_name(script->get_path()). It's a bit ridiculous, just because the ScriptServer doesn't do any work to maintain a reverse lookup of path-to-typename.

I shouldn't have to worry about this running outside of the editor right? If it is going to run outside, then is there a way I can check if I'm in the editor?

@Atlinx Also, I just realized examining this, that this is not really a good solution (or at least, it doesn't seem very reliable). Any given file can have 0 or more dependencies, any number of which might happen to be a script. But you don't know which of those dependencies is the script property of the file in question. So, imo, you really do need to have some sort of resource script type property being tracked in the FileInfo/FileCache in EditroFileSystem. And rather than forcing the use of ResourceFormatLoaderText, you would need to define a virtual function in ResourceFormatLoader that gets the scripted type of the resource, if it has one, which you would then override in the various derived implementations. Then the logic to get the file's resource script type would involve iterating over all of the loaders present in the ResourceLoader.

Does this mean we would have to break compatibility with 3.x? Adding another field to FileInfo/FileCache means incrementing the file cache file version, and I'm not sure if this counts as compatibility breakage. Either way, I'll try to rewrite this PR using your suggestions.

I'm guessing it would involve breaking compatibility. Even if you assume that it's possible for 3.x and 4.x to have their own branching continuations of file cache versions, the fact that they share the same sequential numeric versioning scheme means that as soon as 3.x attempts to increment beyond where it already is, it will be stepping into the realm of 4.x's prior iterations (probably?). So yeah, I doubt this is something that can be properly done in 3.x, simply because the EditorFileSystem doesn't have any mechanism by which it can efficiently keep track of the type of the script that may or may not be assigned to a given resource file.

The ResourceFormatLoader thing might also become a headache IF you end up also modifying get_resource_type to return a possible non-C++ class as there are many calls to this method throughout the codebase, all of which likely expect it to return an in-engine C++ type rather than a potential scripted type. If you don't change the logic of that method though, and instead define a new function to access the extra information, thereby demanding that any scenario that need it specifically call that separate function, then there's likely to be less conflict with other uses.

I apparently have a habit of coming up with rather large API-changing solutions, but the team generally frowns on doing that kind of thing unless it's absolutely necessary. So probably try to avoid that as much as you can.

editor/filesystem_dock.cpp

willnationsdev · 2022-04-30T03:02:30Z

@Atlinx Also...

Does anyone know how Godot currently handles fixing dependencies when files are moved/deleted?

I'm not sure of this myself, but I know that the engine now has a UUID system for resources (the ResourceUID::ID type?). Perhaps you could leverage that to track the file? I'm not really sure yet whether that is safe to record in the filecache / EditorFileSystem though, so it would certainly warrant further investigation before determining whether that's the best path forward.

willnationsdev · 2022-04-30T04:36:26Z

From what I can tell, the ResourceUID cache gets updated during _update_scan_actions() which processes all of the diffs between the last scan and the current scan. Then, after the filesystem has been re-generated and all files re-imported / UIDs re-cached, that's when _queue_update_script_classes() runs to trigger the re-assigning of all the various script class information to the file info. So - to me at least - it seems like it should be safe to use a ResourceUID::ID to refer to the script that the file is dependent on. That way, you'd be able to look up the path to the script regardless of whether it is renamed, moved, or modified such that the global class name changes.

Atlinx · 2022-04-30T04:59:19Z

Do you have any ideas about what the .depren files do? I'm still concerned that the original .tres not updating could break everything. Hypothetically a user could open a project with these .depren files lying around and we would be reading from the original .tres files, which are outdated.

willnationsdev · 2022-04-30T05:02:01Z

Do you have any ideas what the .depren files do? I'm still concerned that the original .tres not updating could break everything.

I'm not quite sure. Usually things like that are used to quickly redirect any requests for a given file to wherever it was moved to. But I don't know yet why you would even need something like that if the ResourceUID stuff was added to help resolve issues with file paths moving around. I think the .depren stuff is new, so I'll need to investigate what the use case and methodology even is before I can give a definitive answer.

(Edit: Well, judging from the age of the commits that introduced it, I guess it isn't new and I just never noticed these were a thing, lol. Nevermind me. XD)

I'll just hold off on my own work until this PR is more refined I suppose since I'll be heavily relying on the ability to derive a resource's attached script's global class name from the EditorFileSystem in my Inspector optimizations for the exported resources PR I'm doing, (i.e. it'll build off of whatever it is you do).

Atlinx · 2022-04-30T05:15:41Z

Yeah, I suspect .depren files were used as temp files.

On another note, for the scripts, do they even have a UID? Script's don't have a tres file, and looking in the uid_cache.bin file generated by Godot, I can only find .tres, .png, and .svg file paths. Then again, it could be the binary encoding into a bin file that's preventing me from seeing everything, even with a hex editor.

willnationsdev · 2022-04-30T05:20:44Z

On another note, for the scripts, do they even have a UID?

Seems like that's the distinction. ResourceFormatLoader defines a virtual function, get_resource_uid(p_path), and that method is overridden in both ResourceFormatLoaderText and ResourceFormatLoaderBinary, but I don't see many other overridden implementations that show up in a Find All References search. That said, it doesn't mean such resource files can't have ResourceUIDs. It just means that none of them yet support it (as far as I can tell).

willnationsdev · 2022-04-30T05:22:47Z

So basically, if ResourceLoader::get_resource_uid (which iterates over all the format loaders and calls their respective implementations) is able to derive a valid UID from the path, then you get to use it (yay!). Otherwise, you have to rely on something less effective.

Atlinx · 2022-04-30T05:35:53Z

Ok, I've pushed a WIP branch with script UID fetching -- I'm going to go to sleep now and I'll revisit this PR again when I'm free. @willnationsdev, do you use Discord or the RocketChat by any chance? Having a direct line of communication might make it easier to talk things out. I really appreciate the help you've given to me so far! :D

willnationsdev · 2022-04-30T05:58:03Z

@Atlinx Yes, I'm on Discord regularly w/ the same username on the GodotEngine server. Just mention or DM me if you'd like.

willnationsdev

Thanks for all the quick work on this so far!

willnationsdev · 2022-04-30T06:02:49Z

editor/editor_file_system.cpp

+	Vector<String> deps = get_file_deps(p_idx);
+	for (int i = 0; i < deps.size(); i++) {
+		String resource_type = ResourceLoader::get_resource_type(deps[i]);
+		if (ClassDB::is_parent_class(resource_type, "Script")) {


Script::get_class_static()

willnationsdev · 2022-04-30T06:17:22Z

editor/editor_file_system.cpp

+	if (script_path_base_dir == EditorFileSystemDirectory::get_path()) {
+		// If our resource script path happens to be in the same directory as our
+		// resource, then we can directly fetch the script's FileInfo.
+		int file_idx = EditorFileSystemDirectory::find_file_index(script_file);
+		ERR_FAIL_COND_V(file_idx < 0, "");
+		return files[file_idx]->script_class_name;
+	} else {
+		// If our resource script path is not in the same directory as our resource,
+		// we must fetch it's EditorFileSystemDirectory first, and then fetch the script's
+		// FileInfo from the newly fetched directory.
+		EditorFileSystemDirectory *dir = EditorFileSystem::get_singleton()->get_filesystem_path(script_path_base_dir);
+		int file_idx = dir->find_file_index(script_file);
+		ERR_FAIL_COND_V(file_idx < 0, "");
+		return dir->files[file_idx]->script_class_name;
+	}


(unless you were just adding temporary notes for yourself) Generally prefer not including comments unless it specifically is providing additional contextual details for how it interacts with other code elsewhere or perhaps explains why it chose a specific algorithm over another one. The code should be written in such a way that it is largely self-documenting. Overuse of comments leads to not-maintained "documentation" embedded in the code that quickly becomes stale as code is modified over time in large projects.

Because get_path() and find_file_index are member functions (i.e. methods) of the EditorFileSystemDirectory class, and we are inside of that class currently, you do not need to preface them with the class name scope; it should automatically call the local implementation. You only do this for EditorFileSystem::get_singleton() because that specifically fetches a static instance of the EditorFileSystem class and thus can only be accessed via a static getter function to begin with.

willnationsdev · 2022-04-30T06:20:40Z

editor/editor_file_system.cpp

@@ -899,6 +942,9 @@ void EditorFileSystem::_scan_new_dir(EditorFileSystemDirectory *p_dir, Ref<DirAc
 					fi->type = "TextFile";
 				}
 				fi->uid = ResourceLoader::get_resource_uid(path);
+				if (fi->type == Resource::get_class_static()) {


type can be any C++ class, so it might be more suitable to use an inheritance check here, no? We want to record the script path even if the script is attached to something more derived than the base Resource type.

willnationsdev · 2022-04-30T06:27:53Z

scene/resources/resource_format_text.cpp

+	if (res_type != "Resource") {
+		error_text = "Expected a user-defined resource file";
+		_printerr();
+		error = ERR_PARSE_ERROR;
+		return error;
+	}


Is the direct comparison to "Resource" correct/intentional? Like, does it seem like the ResourceLoaderText only applies to resources that extend the base Resource type, or could there be C++ types deriving Resource that still save as generic resource file types? I'm not sure. Just want to double-check.

Just did some testing and it looks like the res_type is stored as whatever base type your script extends from. I've only extended resources in my use cases, so this never occurred to me.

willnationsdev · 2022-04-30T06:28:04Z

scene/resources/resource_format_text.cpp

@@ -907,6 +907,84 @@ Error ResourceLoaderText::rename_dependencies(Ref<FileAccess> p_f, const String
 	return OK;
 }

+// Fetches the script attached to a custom resource, reading as few lines as possible from the resource.


This top comment is unnecessary.

willnationsdev · 2022-04-30T06:46:08Z

scene/resources/resource_format_text.cpp

+	while (true) {
+		if (next_tag.name != "ext_resource") {
+			break;
+		}


I may be misinterpreting this, but judging from the way this loop is designed, it seems to break out with the first ext_resource it finds. idk if that's actually guaranteed to refer to the script property of the resource though (is it?). Worst case scenario (and what I suspect), you'd have to jump to the serialized properties, find the one named script, identify which ID number of ExtResource(<id>) is assigned to it, and THEN work backward from there to find which ext_resource tag at the top has that ID.

Atlinx · 2022-05-15T02:28:56Z

With @willnationsdev's guidance, I've fixed up this PR. I've rebased my changes and it's ready for review. If this gets approved, we could potentially make a 3.x port, as even though I've increased filesystem_cache's version, different editor versions will end up generating their own cache file if the existing one doesn't match, so it shouldn't matter too much.

doc/classes/EditorFileSystem.xml

doc/classes/ResourceFormatLoader.xml

editor/editor_file_system.cpp

WIP new filesystem_cache fix Add custom resource icons to fs using filesystem cache Fix clang format Revert "Initial fix by adding field to FileInfo" This reverts commit 3975b5a0a03fc1f1f9b6b97a9c28bad225415b1d. 2nd attempt using FileInfo.deps Add willnationsdev's feedback - Refactored code to use ClassDB for inheritance checking. - Removed redundant writing to fi.path Custom resource icons using dependencies approach Revert "2nd attempt using FileInfo.deps" This reverts commit 43bc0f4e6bcd8d4da260cba1ad46d2b4cd11f2dd. Revert "Revert "Initial fix by adding field to FileInfo"" This reverts commit 6a810fd2cef1b1d8a7a1e96ac505c34e201acf1b. WIP using resource UID and custom FileInfo field Custom resource icons using dependencies approach Add willnationsdev's feedback - Refactored code to use ClassDB for inheritance checking. - Removed redundant writing to fi.path Attempt saving UID of script This attempt failed because there's no way of fetching the UID of a script file. I assume this is because scripts aren't treated as a resource. Revert back to script path approach Hit a tough race condition See editor_file_system.cpp:, line 1315, "TODO: Fix this workaround for a race condition." Fix race condition & optimize filesystem_changed signal

…/Atlinx/godot into feat/32706_custom_resource_icons

core/io/resource.cpp

willnationsdev · 2022-07-16T18:31:35Z

@Atlinx It took a little while, but I managed to squash and rebase all of your commits on top of the latest master branch (there were a lot of conflicts within just this branch's own commit history, for some reason). Note that I made the following changes, in accordance with what existed previously:

The _try_emit_filesystem_changed method still returns void rather than your modification for it to return bool, since the returned value is never actually used any time the method is called.
Because other changes in the history have a method overload of EditorFilesystemDock::_get_file_icon() that takes a String rather than an EditorFilesystemDirectory + int index, I switched back your renaming of the method from _get_file_icon to _get_tree_item_icon so that the method overload would still work properly.
I had to do an additional commit on top because some Map<> reference should've been HashMap<> (not sure how?).

If you would like to grab this version of the code, it is available in my gdres-filesystem branch which does not include any commits related to my exportable resources PRs.

As for whether it is fixed or not, my QuickLoad dialog now shows the UrbanTerrain resources, but not the WildTerrain resources. I think it is somehow related to the EditorFileSystem not re-scanning files properly as I've identified it not doing so when I expected it to, e.g. when the editor regains focus or when I made changes to a GDScript file's source code in the ScriptEditor. And if the scan doesn't run, then the "attached script" info won't update (my hypothesis anyway). I'm about to do a live stream of some more involved debugging.

Atlinx · 2022-07-16T19:02:14Z

As for whether it is fixed or not, my QuickLoad dialog now shows the UrbanTerrain resources, but not the WildTerrain resources. I think it is somehow related to the EditorFileSystem not re-scanning files properly as I've identified it not doing so when I expected it to, e.g. when the editor regains focus or when I made changes to a GDScript file's source code in the ScriptEditor. And if the scan doesn't run, then the "attached script" info won't update (my hypothesis anyway). I'm about to do a live stream of some more involved debugging.

Yea I was playing around in the editor and I noticed the file system doesn't rescan when a new resource is created. I wasn't sure if this was intentional or not (since rescaning might be slow) and so I didn't modify it.

willnationsdev · 2022-07-16T19:57:01Z

@Atlinx Gotcha. Then we'll need to figure out whether that's intentional or not / what the decision-making there was.

Regardless, I found another bug. With your new implementation, any resource with a global script attached to it that has an exported property containing a user-defined Resource type ends up not successfully parsing tags during the ResourceFormatLoaderText::_get_attached_script_path() method. In my case, my WildTerrain type which extends Terrain and then Resource, has an exported property of type NaturalResource which extends Resource. The NaturalResource instance shows up as a [sub_resource ...] tag when saved as a .tres. It then triggers an error when you call parse_tag here:

	while (next_tag.name != "resource") {
		error = VariantParser::parse_tag(&stream, lines, error_text, next_tag);
		if (error) {
			return "";
		}
	}

...because it starts the parsing process on the script identifier underneath the actual tag. So it just returns an empty string and thus does not associate a script resource path with the resource file.

Atlinx · 2022-07-17T08:33:18Z

So it's throwing the error because it's parsing something that's not a tag since I assume what's below [sub_resource] are assign statements?

willnationsdev · 2022-07-17T21:28:18Z

@Atlinx yes, that is correct.

Atlinx · 2022-07-25T15:23:26Z

@willnationsdev I've pushed some changes -- let me know if the latest commit works for you.

YuriSizov · 2022-09-16T15:03:37Z

So just to get those who follow this PR up to speed. We want to merge #62417 without these changes; even if performance is poor (if it's very poor, we'll prefer to just disable the relevant branches of code for the time being, but still merge the ideal solution).

The feeling about the changes present in this PR so far are that they are very invasive and touch a lot of core while being quite hacky. We can allow hackiness to a degree in the editor code, but core should be as clean as we can allow it. So this PR would probably have to wait a bit, very likely until 4.1 where we want to refactor EditorNode and EditorFileSystem cache too. I'm not sure if we would want to use this PR in some way, or if the internal changes would completely supersede it. So for now I'd keep it open, just remove as a dependency of #62417.

I think it would also be good to reopen this PR anew, so that there aren't over 100 discussion and review comments and over 50 commits. A new one, summarizing the current state of this work, with all changes neatly squashed would be easier to keep a track of. But I'd urge you to probably not work much on this for the moment until we can figure out what internal changes we can make to simplify the relevant parts of the engine and the editor.

Atlinx · 2022-09-17T23:28:30Z

Sounds good 👍, I'll just keep this open in limbo for now.

I think the most elegant solution would be adding metadata to tres files, so important data can be accessed faster without parsing the whole file. Refactoring EditorNode and EditorFileSystem could also work, but then we'd have to deal with invalidation, and it still doesn't address the issue of the initial loading of entire resource files in order to access a small portion of it.

willnationsdev

(Edit: lol, I started writing this before I saw that you had already responded and seem perfectly at ease with the feedback, but anyway, just in case, I hope you find this explanation helpful, haha)

I wanna thank you again for all the work you put into this PR. Even if it isn't accepted in its current state, it definitely gives a better starting point from which to create a more effective solution.

I've been in past situations where I did a bunch of work for a PR and received a similar response of my changes being too "invasive" or "hacky". At the time, I was quite disheartened and frustrated. This may or may not be the case for you, but if it is, I'm re-reviewing these changes through this lens to consider in what ways others may find fault with them. I hope my feedback can provide insight into what could be improved here and enrich the quality of your future work should you choose to continue making contributions.

For things labeled as "comment bloat":

Generally, Godot's source code only contains comments when it is meant to explain why the chosen code was selected vs. another approach since it usually is a contextual matter specific to Godot's codebase/behavior. That is, one needs to take into account other code in order to know why the current code is doing what it does.

However, if the current code is self-explanatory or can be discerned simply by reading the code within the class/function and putting the pieces together yourself, then explanatory comments are avoided since they are a maintenance hazard; they can easily become outdated by future contributors' work, and they unnecessarily space out the code making it harder to read as much content within a single view.
For things labeled as "uncertain/questionable core bloat*" or "core bloat?*":

This is a pull request that relates to improving the behavior of things at the editor level. As such, the effect it should have on the engine's core should be kept to a minimum. However, in the case of this specific problem, it ultimately has to be resolved by extracting serialized data from resources which, in turn, must involve making changes to the way in which resources are serialized (a piece of core logic). So you can't really avoid making some core changes to implement a solution (I think). If there IS a way to do that, then we need to investigate that possibility and re-work the solution to support that. If not though, I don't personally see any issue with the way you've added the extra behavior to the ResourceFormatLoader(s). Rather, the more pertinent issue is probably...(next point)
For things labeled as "core bloat**":

Variant is an extremely core class since it basically touches every piece of the engine, so any changes made to it are done so with great care. Nonetheless, there are a variety of new methods you've added to its parser which I'm not entirely sure should be necessary just for the sake of implementing this feature.

Furthermore, the way in which the type information is extracted using the tags is inherently "hacky" since it revolves around figuring out where the list of property tags are, iterating through all of the properties, and identifying which one corresponds to the object's "script" property. This in turn makes the logic entirely dependent on the structural makeup of the tag tree, as well as whether the object in question has a "script" property. Should a later alteration in the engine choose to modify how script information is associated with an object, the VariantParser, of all things, would now suddenly be broken (which would not be directly related to Object-handling code and thus not obvious).

In addition, the algorithm introduces a linear-time operation over the number of properties the object serializes. This contrasts with how, in concept at least, the operation is not at all requesting aggregate data related to the object's properties. It would make more sense for it to be constant-time operation.

None of this is to suggest that you didn't have reasons for doing things the way you did. I honestly probably would've first thought to do something similar to what you did (which is probably why I didn't question it the first time I reviewed it). That is, don't make any major changes to the way data is tracked and just work with what is already there to accomplish what minimum changes are necessary to make the logic further down the pipeline work properly.

In theory, you are attempting to avoid making significant changes to the core. In practice though, given the lengths your implementation has to go through to get things to work as needed, a worrying number of changes are introduced.

In this PR, you introduce changes to the core variant logic merely to support this one higher-level feature. If left alone and accepted, such single-purpose solutions culminate in a codebase that has many small pieces in the core that are not actually used by or relevant to the core at all, thus leading to code bloat. It would be preferred if we could make the minimum number of changes possible to the core to expose whatever information is necessary for higher-level pieces of the codebase to implement the required logic. This shifts the bulk of the changes to later layers of the architecture and results in a simpler and more direct approach to achieving the same goal.

I appreciate you going through all the effort of writing and maintaining this branch. It will certainly be a big help in guiding the direction of the next attempt at fixing this issue. Great work!

willnationsdev · 2022-09-17T22:28:43Z

core/io/resource.cpp

@@ -320,6 +320,11 @@ void Resource::notify_change_to_owners() {
 	}
 }

+// We assume that only Resources can have attached scripts (excluding Scripts and PackedScenes, which still extend Resource).


Comment bloat

willnationsdev · 2022-09-17T22:29:09Z

core/io/resource.cpp

+bool Resource::is_script_extendable_resource(const StringName &p_class) {
+	return ClassDB::is_parent_class(p_class, Resource::get_class_static()) && !ClassDB::is_parent_class(p_class, Script::get_class_static()) && !ClassDB::is_parent_class(p_class, "PackedScene");
+}
+


Possibly considered core bloat, but also, this seems like it could actually be reasonable since it's short and centralizes logic that would be used not just in core, but also in scene and extensions/modules.

willnationsdev · 2022-09-17T22:31:32Z

core/io/resource_format_binary.cpp

+String ResourceLoaderBinary::get_attached_script_path(Ref<FileAccess> p_f) {
+	open(p_f, false, true);
+	if (error) {
+		return "";
+	}
+	int main_resource_idx = internal_resources.size() - 1;
+
+	uint64_t offset = internal_resources[main_resource_idx].offset;
+
+	f->seek(offset);
+
+	String t = get_unicode_string();
+
+	int pc = f->get_32();
+	for (int j = 0; j < pc; j++) {
+		StringName name = _get_string();
+
+		if (name == StringName()) {
+			error = ERR_FILE_CORRUPT;
+			ERR_FAIL_V("");
+		}
+
+		Variant value;
+		uint32_t type = f->get_32();
+		// Note that OBJECT_EXTERNAL_RESOURCE is old resource file format
+		if (name != StringName("script") || type != VARIANT_OBJECT) {
+			return "";
+		}
+
+		uint32_t objtype = f->get_32();
+		if (objtype != OBJECT_EXTERNAL_RESOURCE_INDEX) {
+			return "";
+		}
+
+		int erindex = f->get_32();
+		return external_resources[erindex].path;
+	}
+
+	return "";
+}
+


Core bloat?*

willnationsdev · 2022-09-17T22:31:51Z

core/io/resource_format_binary.cpp

+String ResourceFormatLoaderBinary::get_attached_script_path(const String &p_path) const {
+	String type = get_resource_type(p_path);
+	if (!Resource::is_script_extendable_resource(type)) {
+		return "";
+	}
+
+	Ref<FileAccess> f = FileAccess::open(p_path, FileAccess::READ);
+	ERR_FAIL_COND_V_MSG(f.is_null(), "", "Cannot open file '" + p_path + "'.");
+
+	ResourceLoaderBinary loader;
+	loader.local_path = ProjectSettings::get_singleton()->localize_path(p_path);
+	loader.res_path = loader.local_path;
+	return loader.get_attached_script_path(f);
+}
+


Core bloat?*

willnationsdev · 2022-09-17T22:32:09Z

core/io/resource_format_binary.h

@@ -100,6 +100,7 @@ class ResourceLoaderBinary {
 	void set_remaps(const HashMap<String, String> &p_remaps) { remaps = p_remaps; }
 	void open(Ref<FileAccess> p_f, bool p_no_resources = false, bool p_keep_uuid_paths = false);
 	String recognize(Ref<FileAccess> p_f);
+	String get_attached_script_path(Ref<FileAccess> p_f);


Core bloat?*

willnationsdev · 2022-09-17T23:52:56Z

editor/filesystem_dock.cpp

+		// We only want to refresh the FileSystemDock AFTER script classes are updated, as
+		// that will update the custom class icons we use for our tree and list items.


Comment bloat.

willnationsdev · 2022-09-17T23:53:10Z

editor/filesystem_dock.h

+	Ref<Texture2D> _get_file_icon(bool p_is_valid, String p_file_type, String p_file_path);
+	Ref<Texture2D> _get_file_icon(bool p_is_valid, String p_file_type, EditorFileSystemDirectory *p_dir, int p_file_index);


Same, keep method name.

willnationsdev · 2022-09-17T23:54:22Z

scene/resources/resource_format_text.cpp

@@ -1544,6 +1545,133 @@ ResourceUID::ID ResourceLoaderText::get_uid(Ref<FileAccess> p_f) {
 	return ResourceUID::INVALID_ID;
 }

+Error ResourceLoaderText::_parse_only_ext_resource_value(VariantParser::Token &token, Variant &value, VariantParser::Stream *p_stream, int &line, String &r_err_str, VariantParser::ResourceParser *p_res_parser = nullptr) {


Related to uncertain/questionable core bloat*.

willnationsdev · 2022-09-17T23:55:09Z

scene/resources/resource_format_text.cpp

+		error = VariantParser::parse_tag(&stream, lines, error_text, next_tag);
+
+		if (error) {
+			_printerr();
+		}
+
+		resource_current++;
+	}
+
+	// Find resource tag to begin parsing properties
+	while (next_tag.name != "resource") {
+		error = VariantParser::skip_until_tag(&stream, lines, error_text, next_tag);
+		if (error) {
+			return "";
+		}
+	}
+
+	resource_current++;
+
+	while (true) {
+		String assign;
+		Variant value;
+
+		error = VariantParser::parse_tag_assign_with_value_func_eof(&stream, lines, error_text, next_tag, assign, value, &rp, false, _parse_only_ext_resource_value);


Related to core bloat**.

willnationsdev · 2022-09-17T23:59:46Z

scene/resources/resource_format_text.cpp

+	}
+
+	ResourceLoaderText loader;
+	return loader.get_attached_script_path(f);


Related to uncertain/questionable core bloat*.

Atlinx · 2023-02-24T20:33:59Z

Superceded by #73611

Atlinx requested review from a team as code owners April 29, 2022 01:18

timothyqiu added bug topic:editor labels Apr 29, 2022

YeldhamDev added this to the 4.0 milestone Apr 29, 2022

Atlinx force-pushed the feat/32706_custom_resource_icons branch from 66c4a4e to 3975b5a Compare April 30, 2022 00:07

willnationsdev suggested changes Apr 30, 2022

View reviewed changes

willnationsdev reviewed Apr 30, 2022

View reviewed changes

willnationsdev mentioned this pull request May 1, 2022

Allow exporting custom resources from/to GDScript, VisualScript, C#, and PluginScript. #48201

Closed

Atlinx requested a review from a team as a code owner May 15, 2022 02:09

Atlinx force-pushed the feat/32706_custom_resource_icons branch from 9b8f38c to 455ffcf Compare May 15, 2022 02:16

Calinou reviewed May 21, 2022

View reviewed changes

doc/classes/EditorFileSystem.xml Outdated Show resolved Hide resolved

Calinou reviewed May 21, 2022

View reviewed changes

doc/classes/ResourceFormatLoader.xml Outdated Show resolved Hide resolved

Calinou reviewed May 21, 2022

View reviewed changes

editor/editor_file_system.cpp Outdated Show resolved Hide resolved

Calinou reviewed May 21, 2022

View reviewed changes

editor/editor_file_system.cpp Outdated Show resolved Hide resolved

Atlinx added 3 commits May 21, 2022 19:04

Remove comments and update docs

2139a6c

Remove some comments & unecessary include

6555f22

Atlinx added 4 commits July 15, 2022 04:06

Change resource parsing to explicitly look for script property

23e8af4

Change Map to HashMap

4151bd8

Remove old code

aee71b9

Merge branch 'feat/32706_custom_resource_icons' of https://github.com…

dcee398

…/Atlinx/godot into feat/32706_custom_resource_icons

akien-mga reviewed Jul 15, 2022

View reviewed changes

core/io/resource.cpp Outdated Show resolved Hide resolved

Remove dependency on scene and remove unecessary resource.h changes

20b421a

Add skip until tag parsing

a8da40f

Atlinx added 2 commits July 25, 2022 11:25

Merge branch 'master' into feat/32706_custom_resource_icons

3b4734d

Fix missing description tag in doc

dc65e34

willnationsdev mentioned this pull request Aug 1, 2022

Add top-level @export to separate class_name. #63758

Open

Atlinx added 3 commits August 11, 2022 00:34

Fix indentation to tabs in docs

8c35031

Merge branch 'godotengine:master' into feat/32706_custom_resource_icons

9abeb02

Fix docs

f0aba98

YuriSizov added enhancement performance labels Sep 16, 2022

willnationsdev reviewed Sep 18, 2022

View reviewed changes

YuriSizov modified the milestones: 4.0, 4.x Feb 10, 2023

Atlinx closed this Feb 24, 2023

KoBeWi added the archived label Feb 24, 2023

aaronfranke removed this from the 4.x milestone Feb 28, 2023

		// We only want to refresh the FileSystemDock AFTER script classes are updated, as
		// that will update the custom class icons we use for our tree and list items.

		Ref<Texture2D> _get_file_icon(bool p_is_valid, String p_file_type, String p_file_path);
		Ref<Texture2D> _get_file_icon(bool p_is_valid, String p_file_type, EditorFileSystemDirectory *p_dir, int p_file_index);

Fix custom resource icons not appearing in file system #60606

Fix custom resource icons not appearing in file system #60606

Conversation

Atlinx commented Apr 29, 2022 • edited Loading

Atlinx commented Apr 29, 2022 • edited Loading

Atlinx commented Apr 30, 2022 • edited Loading

willnationsdev left a comment • edited Loading

Choose a reason for hiding this comment

willnationsdev Apr 30, 2022 • edited Loading

Choose a reason for hiding this comment

Atlinx Apr 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Atlinx Apr 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willnationsdev Apr 30, 2022 • edited Loading

Choose a reason for hiding this comment

willnationsdev commented Apr 30, 2022

willnationsdev commented Apr 30, 2022 • edited Loading

Atlinx commented Apr 30, 2022 • edited Loading

willnationsdev commented Apr 30, 2022 • edited Loading

Atlinx commented Apr 30, 2022 • edited Loading

willnationsdev commented Apr 30, 2022 • edited Loading

willnationsdev commented Apr 30, 2022

Atlinx commented Apr 30, 2022 • edited Loading

willnationsdev commented Apr 30, 2022

willnationsdev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Atlinx commented May 15, 2022

willnationsdev commented Jul 16, 2022

Atlinx commented Jul 16, 2022

willnationsdev commented Jul 16, 2022

Atlinx commented Jul 17, 2022 • edited Loading

willnationsdev commented Jul 17, 2022

Atlinx commented Jul 25, 2022

YuriSizov commented Sep 16, 2022

Atlinx commented Sep 17, 2022 • edited Loading

willnationsdev left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Atlinx commented Feb 24, 2023

Atlinx commented Apr 29, 2022 •

edited

Loading

Atlinx commented Apr 29, 2022 •

edited

Loading

Atlinx commented Apr 30, 2022 •

edited

Loading

willnationsdev left a comment •

edited

Loading

willnationsdev Apr 30, 2022 •

edited

Loading

Atlinx Apr 30, 2022 •

edited

Loading

Atlinx Apr 30, 2022 •

edited

Loading

willnationsdev Apr 30, 2022 •

edited

Loading

willnationsdev commented Apr 30, 2022 •

edited

Loading

Atlinx commented Apr 30, 2022 •

edited

Loading

willnationsdev commented Apr 30, 2022 •

edited

Loading

Atlinx commented Apr 30, 2022 •

edited

Loading

willnationsdev commented Apr 30, 2022 •

edited

Loading

Atlinx commented Apr 30, 2022 •

edited

Loading

Atlinx commented Jul 17, 2022 •

edited

Loading

Atlinx commented Sep 17, 2022 •

edited

Loading

willnationsdev left a comment •

edited

Loading