-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Fix custom resource icons in FileSystem #77932
Fix custom resource icons in FileSystem #77932
Conversation
9a111ff
to
a52b220
Compare
a52b220
to
8d85da0
Compare
Yeah, the branch name is very fitting 🙃 This approach is going to read and parse the file for each stored resources every time we need to figure out its icon. Even if this information doesn't change, we would still parse the file, at least partially, to fetch the type information. This definitely needs some cache, and that's what reduz wanted as well, I believe. To utilize the editor file system somehow, and only update the information when the underlying data, like the type or the script, changes. This way we would only have to run this expensive operation on change rather than on fetch. I believe that the PR this one is supposed to supersede was going this route. After a lot of discussion they've hooked into the preview generator process, since it already needs to run for a changed resource. Maybe it's not the best place to hook into, but at least it creates an accessible cache for us to use on the file tree refreshes. So in the end, I would not merge this in its current state and look for a more optimal approach using some sort of cache. |
I think the icons can be cached locally, we already do this with scene tree (see #60789). I'll look into that, I thought |
8d85da0
to
476d200
Compare
Ok rebased and updated. I used EditorFileSystem's cache to get resource icon, but it has caveats:
I added a temporary HashMap that stores cache for each script path, so it shouldn't be too slow, but testing appreciated. If it's slow, it will affect file tree updates. EDIT: |
476d200
to
a62ccf3
Compare
const String &script_path = deps[0]; // Assuming the first dependency is a script. | ||
if (script_path.is_empty() || !ClassDB::is_parent_class(ResourceLoader::get_resource_type(script_path), SNAME("Script"))) { | ||
return String(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe loop through the dependencies for the first script?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be slower though 🤔
I'd change this only if it proves unreliable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than my comment, everything seems fine! I tested it and it works.
Thanks @KoBeWi !
Thanks! |
Fixes #32706
Supersedes #73611