Don't grab theme icons for scripts

[KoBeWi]

When a node is extending a different class from it's actual type, the scene tree dock will show the script class instead. Extending a different class is a common use-case, e.g. you might have a Container-based node, but make the script extend Control to avoid problems with changing type etc. Scene tree displaying script type means that you don't know what Container type this node represents unless you hover it and check the tooltip. It's confusing and annoying.

This PR gets rid of this behavior. Custom types are unaffected.

Before

After

EDIT:
Fixes #76983

[akien-mga]

Fixes #76983 ?

[YuriSizov]

This is fine as a temporary fix, but...

This was a part of the old get_class_icon method (but not of get_object_icon). I guess I might've been greedy when unifying those two methods. When we try to get a class icon for a script, it makes sense to fall back on the first built-in base class of the script. Whereas when we try to get an icon for an object, we should respect that object.

The method that you've changed deals with the script, so I think it should still do the look up for the base class in the theme. The discrepancy must be handled at a higher level, in EditorNode::_get_class_or_script_icon (unlikely, too general as well) or in EditorNode::get_object_icon. Can also be done in EditorNode::_get_class_or_script_icon but put behind a flag that only EditorNode::get_object_icon would set.

[KoBeWi]

What unwanted side-effects would my change cause?
It's easy to ignore native class icons if you know the object's extended type, but this information is only available and cached in get_script_icon(). In any higher method there's a risk of slow lookups.

Note that _get_class_or_script_icon() already fallbacks to theme icon.

[YuriSizov]

What unwanted side-effects would my change cause?

I suppose, scripts without custom icons will display a generic icon rather than an icon corresponding to their base type. And I mean scripts themselves rather than nodes with these scripts attached.

In any higher method there's a risk of slow lookups.

We could cache base types in EditorData just like we cache icons 👀

[KoBeWi]

Do we even display custom icons for Script resources? I think it's always based on the language only.
Also, this method is only called for objects with scripts attached, and Script can't have a script.

[YuriSizov]

Also, this method is only called for objects with scripts attached, and Script can't have a script.

get_class_icon is called for strings with class names. Only get_object_icon is called for objects. That's why they are different methods. We kind of need both.

[HolonProduction]

Just wanted to point out, that I have a PR which addresses the same issue: #76993.
If this gets merged the other one should be closed.

[KoBeWi]

I looked at the code again, and just as I noted in a previous comment, _get_class_or_script_icon() already falls back to theme icon:

godot/editor/editor_node.cpp

Lines 4308 to 4318 in 8f175a8

	// Look up the class name or the fallback name in the editor theme.
	// This is only relevant for built-in classes.
	if (gui_base) {
	if (gui_base->has_theme_icon(p_class, SNAME("EditorIcons"))) {
	return gui_base->get_theme_icon(p_class, SNAME("EditorIcons"));
	}
	if (p_fallback.length() && gui_base->has_theme_icon(p_fallback, SNAME("EditorIcons"))) {
	return gui_base->get_theme_icon(p_fallback, SNAME("EditorIcons"));
	}
	}

And this part of code actually checks if the returned script icon is valid and tries further if not:

godot/editor/editor_node.cpp

Lines 4285 to 4290 in 8f175a8

	if (p_script.is_valid()) {
	Ref<Texture2D> script_icon = ed.get_script_icon(p_script);
	if (script_icon.is_valid()) {
	return script_icon;
	}
	}

So not sure what should I change. Am I missing something?

[YuriSizov]

So not sure what should I change. Am I missing something?

The bit that you removed looks if the base type of the script has an icon in the theme. That's the only place where we try to find an icon for the base type. The snippets that you've highlighted don't use the base type of the script, they use the class name passed to the function.

This means that after your changes if you call get_class_icon("MyCustomType") and your custom type has no icon itself, you will get the fallback icon returned. Before I unified get_class_icon and get_object_icon this worked differently. get_class_icon would always check the base type of the script and use its icon. get_object_icon didn't do that.

When I unified them, I used a combination of checks from both methods. That's how we have this check that you're removing in there. I took the code from the old get_class_icon which had no equivalent in the old get_object_icon. That was my mistake and the discrepancy was intentional. We don't want to check the base class of the script in get_object_icon, because the object takes the priority if the script is missing a direct icon.

But if we want to preserve the old logic correctly, get_class_icon should still check the base type. So my proposal is to add a flag to _get_class_or_script_icon, where get_class_icon would pass true and get_object_icon would pass false, and we use this flag to conditionally check for the base type. Since we don't want to mess with the cache in EditorData::get_script_icon, you can probably move the check to this bit that you've highlighted:

godot/editor/editor_node.cpp

Lines 4285 to 4290 in 8f175a8

	if (p_script.is_valid()) {
	Ref<Texture2D> script_icon = ed.get_script_icon(p_script);
	if (script_icon.is_valid()) {
	return script_icon;
	}
	}

Something like

 if (p_script.is_valid()) { 
 	Ref<Texture2D> script_icon = ed.get_script_icon(p_script); 
 	if (script_icon.is_valid()) { 
 		return script_icon; 
 	} 

	// Look for the base type in the editor theme.
	// This is only relevant for built-in classes.

	String base_type;
	p_script->get_language()->get_global_class_name(p_script->get_path(), &base_type);
	if (gui_base && gui_base->has_theme_icon(base_type, SNAME("EditorIcons"))) {
		return gui_base->get_theme_icon(base_type, SNAME("EditorIcons"));
	}
 } 

[KoBeWi]

Ok I pushed something, but idk how to test it.

[YuriSizov]

I think this is good now. At the very least it preserves the original logic and reverts my incorrect changes. We can see if there are any new complaints or if we notice any issues naturally.

[YuriSizov]

Thanks!

[YuriSizov]

Cherry-picked for 4.1.2.