-
-
Notifications
You must be signed in to change notification settings - Fork 21k
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
Cache Object Icons #36999
Cache Object Icons #36999
Conversation
is there a way to update cached icon when custom icon is changed? |
Yes I should probably remove that element in the set call Edit: ResourceLoader would work a lot better here (caching and it watches for file changes), but then icons might need to be manually imported as Images. |
Is it possible anyone could help? I want to use ResourceLoader here to avoid making a custom cache system for the script icons, but it wouldn't work if the icon is imported as an image. |
71163ed
to
7590c6e
Compare
Using resource loader now. Added an error if the user tries to use an icon not imported as Image. (This is a problem since the default import is Texture). Let me know |
7590c6e
to
18087d0
Compare
Thanks for the reviews Akien! They all should be fixed now. The only issue now is that users are forced to Re-import their icons as Images. I thought this could be fixed by using print(ResourceLoader.load("res://icon.png", "Image"))
|
18087d0
to
70187f8
Compare
70187f8
to
1c63264
Compare
Made this a draft since #44501 needs to be fixed :) |
@@ -773,8 +773,8 @@ class EditorNode : public Node { | |||
Ref<Theme> get_editor_theme() const { return theme; } | |||
Ref<Script> get_object_custom_type_base(const Object *p_object) const; | |||
StringName get_object_custom_type_name(const Object *p_object) const; | |||
Ref<Texture2D> get_object_icon(const Object *p_object, const String &p_fallback = "Object") const; | |||
Ref<Texture2D> get_class_icon(const String &p_class, const String &p_fallback = "Object") const; |
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.
What is the purpose of removing const
? Do these methods need to modify the EditorNode instance?
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.
They use resource loader load which isn't const IIRC, but I might be able to add it back since previously I used a manual cache system.
Since this doesn't break compat and can even be potentially ported back to |
From what I understand, the "cache" part comes from using the resource loader. As such, this is already addressed in master, alongside other improvements to class icons (see #75472). The rest of the changes don't seem relevant to this problem. So I think this is safe to close as superseded. Thanks nevertheless! |
Draft: Wait for #44501 to be fixed.
Fixes #36981
Edit: This probably shouldn't be merged yet
since the cache will not update if the image data is changed. But if the path changes it will work fine.The cache now works properly but it requires the icon is imported as Image which could be annoying.