Add a script method to get its class icon

[YuriSizov]

This is more of a proof-of-concept built on top of #75472, but perhaps we can polish it into something mergeable? The goal here is to extract the icon for built-in scripts, so we can use it in the editor like we do with regular script classes.

This exposes an editor-only method to fetch an icon path of the script's class. The reason why I'm still skeptical here, is because this doesn't allow for multiple icons per one script (in case it contains multiple classes). Perhaps the API should be generalized somehow, maybe by passing a class name to this method.

But maybe it's fine as is, and we only accept one icon per file. This is totally acceptable for GDScript anyway, at least for now.

[KoBeWi]

The reason why I'm still skeptical here, is because this doesn't allow for multiple icons per one script (in case it contains multiple classes).

How would that even work? Only one icon can be displayed.

This looks interesting, but the method technically only exists to fetch a built-in script icon path. Adding it to Script class is overkill, because only GDScript can be built-in (and VisualScript, but it's irrelevant). I think you could bind this method only in GDScript and use call().

[YuriSizov]

How would that even work? Only one icon can be displayed.

For built-in scripts it's unlikely to be useful, but for discrete scripts it's not impossible that in the future we allow to add icons to sub-classes as well.

the method technically only exists to fetch a built-in script icon path

So far, yeah. But there is nothing inherently limiting it to built-in scripts. So I preferred to make a general-purpose API. It also allows us to avoid binding EditorData and the GDScript module together. (Well, I haven't checked, maybe we already do, but that's not great if so.)

I think you could bind this method only in GDScript and use call().

Ah, yes, that could work. If the server-level change is rejected, this is a good fallback option.

[KoBeWi]

but for discrete scripts it's not impossible that in the future we allow to add icons to sub-classes as well.

But that's still one icon per Create Dialog/Scene Tree/Inspector/Editor Help entry.
Built-in scripts also allow sub-classes. And sub-classes are technically separate scripts. So "multiple icons per script" does not make much sense.

But there is nothing inherently limiting it to built-in scripts.

Well, use-cases are limited. We already have methods for fetching script icons. It would be more useful if you refactored the existing code to use get_class_icon_path().

it's not impossible that in the future
So far, yeah

https://docs.godotengine.org/en/stable/contributing/development/best_practices_for_engine_contributors.html#to-solve-the-problem-it-has-to-exist-in-the-first-place
:v

[YuriSizov]

Well, use-cases are limited. We already have methods for fetching script icons. It would be more useful if you refactored the existing code to use get_class_icon_path().

Do you propose I refactor the entire EditorData caching system just to justify this API? I'm not against the idea of moving all this cache to the ScriptServer side, but for the purposes of this feature it's an overkill.

But that's still one icon per Create Dialog/Scene Tree/Inspector/Editor Help entry.
Built-in scripts also allow sub-classes. And sub-classes are technically separate scripts. So "multiple icons per script" does not make much sense.

The idea is that sub-classes will be registered independently and will have their own icons. GDScript does kind of treat them as separate scripts, but to the engine it's all one resource, and all under the same path in any cache we have.

https://docs.godotengine.org/en/stable/contributing/development/best_practices_for_engine_contributors.html#to-solve-the-problem-it-has-to-exist-in-the-first-place
:v

I personally don't read this as "Add hacks and then maybe refactor later" 🙃 Besides, I'm not adding any new functionality. We do have a ScriptLanguage method that allows us to retrieve a class icon already (get_global_class_name). It's just bundled together with a few other features into a single method that does 3 different things, and is only available to non-built-in scripts.

[YuriSizov]

PS. All this talk about subclasses is only relevant if we want to future-proof the API. But I'd be okay with it as is too. It's a PoC anyway. We can discard it altogether.

[anvilfolk]

PS. All this talk about subclasses is only relevant if we want to future-proof the API. But I'd be okay with it as is too. It's a PoC anyway. We can discard it altogether.

For what it's worth, I'm going to try to make sure that inspector tooltips are working with subclasses also. Who knows if that'll work, but worth a try?

[dalexeev]

Please remove the outdated note in the @icon docs (I'm just reminding you):

The script must be registered as a global class using the class_name keyword for this to have a visible effect.

[YuriSizov]

Removed the note and adjusted the condition to handle any unnamed script, not just built-ins.

[anvilfolk]

PS. All this talk about subclasses is only relevant if we want to future-proof the API. But I'd be okay with it as is too. It's a PoC anyway. We can discard it altogether.

For what it's worth, I'm going to try to make sure that inspector tooltips are working with subclasses also. Who knows if that'll work, but worth a try?

I also just remembered that I looked into this but there was actually no way to get inner classes to show up in the inspector (unless you got into remote inspect mode when the app is running, but then no docstrings actually show up and everything has weird names), so that shouldn't be a consideration anymore either :)

[KoBeWi]

Needs description.

[YuriSizov]

I agree, but in this case the entire class needs documenting, and this method is pretty much self-explanatory. Someone should do a pass to add descriptions to all of them, in the same consistent style. So I think it's not important for this PR.

[KoBeWi]

I'm for enforcing writing descriptions for every added documentation field, regardless if the class is documented overall or not, and whether anyone can write it. Leaving empty fields like that is the reason why we have so much undocumented stuff.

in the same consistent style

You mean the Godot documentation style used everywhere?

[YuriSizov]

Judging by the number of "overhaul" documentation PRs we don't have a consistent style. 🙃
Okay, if you insist, I'll first make a PR documenting this class and then we can consider this feature.

[akien-mga]

Currently we haven't been writing any docs for *Extension classes, where their methods are exposed only for the purpose of being overridden from GDExtension, but we don't have in-depth documentation on how to implement a Script language with GDExtension. It's expected that users who want to do that (which is a very advanced use case) will read the source code of the engine to know what to do (and refer to the general Script documentation to see what's the contract for each of the function they're defining).

Having docs for extension classes would definitely be nice, but it's definitely outside the scope of this PR.

[akien-mga]

That's not what this method does though, it's what it should do in your implementation :)

[KoBeWi]

Yes. In Control virtual methods there is also "Virtual method to be implemented by the user.", but here all methods are virtual. I think it should be in the class description that the methods are supposed to be overridden.

[dsnopek]

I'm not sure the ScriptExtension class should even be exposed? You should never interact with it from GDScript or C#, and even GDExtension dosen't directly interact with it (from GDExtension you register some callbacks via a function in gdextension_interface.h which get used by ScriptExtension).

If we edit register_engine_classes.cpp and remove the line:

ClassDB::register_engine_class<ScriptExtension>();

... I think (although, I didn't test) that it should still work fine for it's purpose, except as an "unexposed" class.

And then I think (although, again I didn't test) that godot --doctool shouldn't even try to generate docs for it.

[KoBeWi]

Having it exposed is the only way to document the methods that need to be implemented. For people creating their own script extensions it would be useful to know what these methods are supposed to do.

[dsnopek]

Er, sorry, I got myself mixed up! I was thinking of ScriptInstanceExtension which GDExtension developers don't directly interact with. For ScriptExtension, the GDExtension developer does directly interact with the class, so this should remain exposed.

[YuriSizov]

Rebased and tested again. Seems to work as before, and doesn't introduce any regression with mismatched types (fixed by #79203) for built-in scripts (the "Control" here has a script that extends Node):

[dalexeev]

I tested it a bit, it looks like it works fine.

Potentially this should work for inner classes as well? (However, you cannot attach an inner script in the editor, only create it via code.) I tried to do this, but it doesn't work. Not so important, just a note for possible future PR.

My try diff

diff --git a/modules/gdscript/gdscript_parser.cpp b/modules/gdscript/gdscript_parser.cpp
index debc85ebbf..e568592299 100644
--- a/modules/gdscript/gdscript_parser.cpp
+++ b/modules/gdscript/gdscript_parser.cpp
@@ -84,7 +84,7 @@ GDScriptParser::GDScriptParser() {
 	// Register valid annotations.
 	// TODO: Should this be static?
 	register_annotation(MethodInfo("@tool"), AnnotationInfo::SCRIPT, &GDScriptParser::tool_annotation);
-	register_annotation(MethodInfo("@icon", PropertyInfo(Variant::STRING, "icon_path")), AnnotationInfo::SCRIPT, &GDScriptParser::icon_annotation);
+	register_annotation(MethodInfo("@icon", PropertyInfo(Variant::STRING, "icon_path")), AnnotationInfo::SCRIPT | AnnotationInfo::CLASS, &GDScriptParser::icon_annotation);
 	register_annotation(MethodInfo("@static_unload"), AnnotationInfo::SCRIPT, &GDScriptParser::static_unload_annotation);
 
 	register_annotation(MethodInfo("@onready"), AnnotationInfo::VARIABLE, &GDScriptParser::onready_annotation);
@@ -762,6 +762,13 @@ void GDScriptParser::parse_class_member(T *(GDScriptParser::*p_parse_function)(b
 	}
 
 	for (AnnotationNode *&annotation : annotations) {
+		if constexpr (std::is_same_v<T, ClassNode>) {
+			if (annotation->name == SNAME("@icon")) {
+				// `@icon` needs to be applied in the parser.
+				annotation->apply(this, member);
+				continue;
+			}
+		}
 		member->annotations.push_back(annotation);
 	}

[raulsntos]

Tested with C# and it works as expected, but the icons don't update until the scene is reopened (same with GDScript).

[YuriSizov]

Added a GDScript-side cache per @dalexeev's suggestion and made the script extension method non-required for compatibility.

At this point, unless there are further reviews, only the docs requirement is remaining. As I mentioned, I would prefer for it to be done in a separate PR, because no docs are present for the documented class yet. If it's a hard requirement, I'd prefer to do it myself as a prerequisite of this PR, otherwise I'd leave it for someone to cover in a follow-up.

[dalexeev]

Looks good to me. Since the GDScript changes are made by me, it would be nice to get a double check from @vnen or @adamscott, so that it doesn't feel like self-approve. But even if we find a bug later, we can always fix it, it doesn't look like a critical part that is worth the long hesitation. Let's give it a green light!

[akien-mga]

Looks good to me.

If there's no objection / requested changes, this should be mergeable for 4.2-dev5.

[akien-mga]

Thanks!