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

Cache Object Icons #36999

Closed

Conversation

nathanfranke
Copy link
Contributor

@nathanfranke nathanfranke commented Mar 11, 2020

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.

@volzhs
Copy link
Contributor

volzhs commented Mar 12, 2020

is there a way to update cached icon when custom icon is changed?

@nathanfranke
Copy link
Contributor Author

nathanfranke commented Mar 12, 2020

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.

@akien-mga akien-mga added this to the 4.0 milestone Mar 12, 2020
@akien-mga akien-mga added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Mar 12, 2020
@nathanfranke
Copy link
Contributor Author

nathanfranke commented Jul 1, 2020

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.
Edit: Working on this right now

@nathanfranke
Copy link
Contributor Author

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

editor/editor_node.cpp Outdated Show resolved Hide resolved
editor/editor_node.cpp Outdated Show resolved Hide resolved
editor/editor_node.cpp Outdated Show resolved Hide resolved
editor/editor_data.cpp Outdated Show resolved Hide resolved
editor/editor_node.cpp Outdated Show resolved Hide resolved
editor/editor_node.cpp Outdated Show resolved Hide resolved
editor/editor_node.cpp Outdated Show resolved Hide resolved
@nathanfranke
Copy link
Contributor Author

nathanfranke commented Jul 2, 2020

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 type_hint, but it doesn't seem to work for me, even in GDScript.

print(ResourceLoader.load("res://icon.png", "Image"))

[StreamTexture:1180]

Edit: Found my other comment here

@nathanfranke nathanfranke marked this pull request as ready for review December 21, 2020 17:34
@nathanfranke nathanfranke marked this pull request as draft April 12, 2021 09:16
@nathanfranke
Copy link
Contributor Author

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;
Copy link
Member

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?

Copy link
Contributor Author

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.

@YuriSizov YuriSizov modified the milestones: 4.0, 4.x Sep 8, 2022
@YuriSizov
Copy link
Contributor

Since this doesn't break compat and can even be potentially ported back to 3.x, I'm assigning it to 4.x to indicate that this is not blocking the release.

@YuriSizov
Copy link
Contributor

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!

@YuriSizov YuriSizov closed this Apr 11, 2023
@YuriSizov YuriSizov added archived and removed cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Apr 11, 2023
@YuriSizov YuriSizov removed this from the 4.x milestone Dec 2, 2023
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.

Scene with many nodes and 512px script icon causes bad lag spikes
6 participants