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.

Sign up for GitHub

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

Add a script method to get its class icon #75656

Merged
merged 1 commit into from
Aug 29, 2023
Merged

Add a script method to get its class icon #75656

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions core/object/script_language.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ class Script : public Resource {

#ifdef TOOLS_ENABLED
virtual Vector<DocData::ClassDoc> get_documentation() const = 0;
virtual String get_class_icon_path() const = 0;
virtual PropertyInfo get_class_category() const;
#endif // TOOLS_ENABLED

Expand Down
1 change: 1 addition & 0 deletions core/object/script_language_extension.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ void ScriptExtension::_bind_methods() {
GDVIRTUAL_BIND(_reload, "keep_state");

GDVIRTUAL_BIND(_get_documentation);
GDVIRTUAL_BIND(_get_class_icon_path);

GDVIRTUAL_BIND(_has_method, "method");
GDVIRTUAL_BIND(_get_method_info, "method");
Expand Down
7 changes: 7 additions & 0 deletions core/object/script_language_extension.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ class ScriptExtension : public Script {
EXBIND1R(Error, reload, bool)

GDVIRTUAL0RC(TypedArray<Dictionary>, _get_documentation)
GDVIRTUAL0RC(String, _get_class_icon_path)
#ifdef TOOLS_ENABLED
virtual Vector<DocData::ClassDoc> get_documentation() const override {
TypedArray<Dictionary> doc;
Expand All @@ -89,6 +90,12 @@ class ScriptExtension : public Script {

return class_doc;
}

virtual String get_class_icon_path() const override {
String ret;
GDVIRTUAL_CALL(_get_class_icon_path, ret);
return ret;
}
#endif // TOOLS_ENABLED

EXBIND1RC(bool, has_method, const StringName &)
Expand Down
5 changes: 5 additions & 0 deletions doc/classes/ScriptExtension.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@
<description>
</description>
</method>
<method name="_get_class_icon_path" qualifiers="virtual const">
<return type="String" />
<description>
</description>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs description.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@YuriSizov YuriSizov Aug 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@KoBeWi KoBeWi Aug 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@dsnopek dsnopek Aug 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

</method>
<method name="_get_constants" qualifiers="virtual const">
<return type="Dictionary" />
<description>
Expand Down
8 changes: 7 additions & 1 deletion editor/editor_data.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1122,8 +1122,14 @@ Ref<Texture2D> EditorData::get_script_icon(const Ref<Script> &p_script) {
Ref<Script> base_scr = p_script;
while (base_scr.is_valid()) {
// Check for scripted classes.
String icon_path;
StringName class_name = script_class_get_name(base_scr->get_path());
String icon_path = script_class_get_icon_path(class_name);
if (base_scr->is_built_in() || class_name == StringName()) {
icon_path = base_scr->get_class_icon_path();
} else {
icon_path = script_class_get_icon_path(class_name);
}

Ref<Texture2D> icon = _load_script_icon(icon_path);
if (icon.is_valid()) {
_script_icon_cache[p_script] = icon;
Expand Down
2 changes: 1 addition & 1 deletion editor/editor_node.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3457,7 +3457,7 @@ void EditorNode::_remove_edited_scene(bool p_change_tab) {

void EditorNode::_remove_scene(int index, bool p_change_tab) {
// Clear icon cache in case some scripts are no longer needed.
// FIXME: Perfectly the cache should never be cleared and only updated on per-script basis, when an icon changes.
// FIXME: Ideally the cache should never be cleared and only updated on per-script basis, when an icon changes.
editor_data.clear_script_icon_cache();

if (editor_data.get_edited_scene() == index) {
Expand Down
2 changes: 1 addition & 1 deletion modules/gdscript/doc_classes/@GDScript.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -595,7 +595,7 @@
<return type="void" />
<param index="0" name="icon_path" type="String" />
<description>
Add a custom icon to the current script. The script must be registered as a global class using the [code]class_name[/code] keyword for this to have a visible effect. The icon specified at [param icon_path] is displayed in the Scene dock for every node of that class, as well as in various editor dialogs.
Add a custom icon to the current script. The icon specified at [param icon_path] is displayed in the Scene dock for every node of that class, as well as in various editor dialogs.
[codeblock]
@icon("res://path/to/class/icon.svg")
[/codeblock]
Expand Down
14 changes: 7 additions & 7 deletions modules/gdscript/gdscript.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,6 +482,10 @@ void GDScript::_clear_doc() {
docs.clear();
doc = DocData::ClassDoc();
}

String GDScript::get_class_icon_path() const {
return simplified_icon_path;
}
#endif

bool GDScript::_update_exports(bool *r_err, bool p_recursive_call, PlaceHolderScriptInstance *p_instance_to_update) {
Expand Down Expand Up @@ -2527,13 +2531,6 @@ String GDScriptLanguage::get_global_class_name(const String &p_path, String *r_b
* Before changing this function, please ask the current maintainer of EditorFileSystem.
*/

if (r_icon_path) {
if (c->icon_path.is_empty() || c->icon_path.is_absolute_path()) {
*r_icon_path = c->icon_path.simplify_path();
} else if (c->icon_path.is_relative_path()) {
*r_icon_path = p_path.get_base_dir().path_join(c->icon_path).simplify_path();
}
}
if (r_base_type) {
const GDScriptParser::ClassNode *subclass = c;
String path = p_path;
Expand Down Expand Up @@ -2601,6 +2598,9 @@ String GDScriptLanguage::get_global_class_name(const String &p_path, String *r_b
}
}
}
if (r_icon_path) {
*r_icon_path = c->simplified_icon_path;
}
return c->identifier != nullptr ? String(c->identifier->name) : String();
}

Expand Down
2 changes: 2 additions & 0 deletions modules/gdscript/gdscript.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ class GDScript : public Script {
String path;
String name;
String fully_qualified_name;
String simplified_icon_path;
SelfList<GDScript> script_list;

SelfList<GDScriptFunctionState>::List pending_func_states;
Expand Down Expand Up @@ -250,6 +251,7 @@ class GDScript : public Script {
virtual Vector<DocData::ClassDoc> get_documentation() const override {
return docs;
}
virtual String get_class_icon_path() const override;
#endif // TOOLS_ENABLED

virtual Error reload(bool p_keep_state = false) override;
Expand Down
1 change: 1 addition & 0 deletions modules/gdscript/gdscript_compiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2928,6 +2928,7 @@ void GDScriptCompiler::convert_to_initializer_type(Variant &p_variant, const GDS
void GDScriptCompiler::make_scripts(GDScript *p_script, const GDScriptParser::ClassNode *p_class, bool p_keep_state) {
p_script->fully_qualified_name = p_class->fqcn;
p_script->name = p_class->identifier ? p_class->identifier->name : "";
p_script->simplified_icon_path = p_class->simplified_icon_path;

HashMap<StringName, Ref<GDScript>> old_subclasses;

Expand Down
17 changes: 15 additions & 2 deletions modules/gdscript/gdscript_parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3836,18 +3836,31 @@ bool GDScriptParser::tool_annotation(const AnnotationNode *p_annotation, Node *p
bool GDScriptParser::icon_annotation(const AnnotationNode *p_annotation, Node *p_node) {
ERR_FAIL_COND_V_MSG(p_node->type != Node::CLASS, false, R"("@icon" annotation can only be applied to classes.)");
ERR_FAIL_COND_V(p_annotation->resolved_arguments.is_empty(), false);

ClassNode *p_class = static_cast<ClassNode *>(p_node);
String path = p_annotation->resolved_arguments[0];

#ifdef DEBUG_ENABLED
if (!p_class->icon_path.is_empty()) {
push_error(R"("@icon" annotation can only be used once.)", p_annotation);
return false;
}
if (String(p_annotation->resolved_arguments[0]).is_empty()) {
if (path.is_empty()) {
push_error(R"("@icon" annotation argument must contain the path to the icon.)", p_annotation->arguments[0]);
return false;
}
#endif // DEBUG_ENABLED
p_class->icon_path = p_annotation->resolved_arguments[0];

p_class->icon_path = path;

if (path.is_empty() || path.is_absolute_path()) {
p_class->simplified_icon_path = path.simplify_path();
} else if (path.is_relative_path()) {
p_class->simplified_icon_path = script_path.get_base_dir().path_join(path).simplify_path();
} else {
p_class->simplified_icon_path = path;
}

YuriSizov marked this conversation as resolved.
Show resolved Hide resolved
return true;
}

Expand Down
1 change: 1 addition & 0 deletions modules/gdscript/gdscript_parser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -724,6 +724,7 @@ class GDScriptParser {

IdentifierNode *identifier = nullptr;
String icon_path;
String simplified_icon_path;
Vector<Member> members;
HashMap<StringName, int> members_indices;
ClassNode *outer = nullptr;
Expand Down
3 changes: 3 additions & 0 deletions modules/mono/csharp_script.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,9 @@ class CSharpScript : public Script {
Vector<DocData::ClassDoc> docs;
return docs;
}
virtual String get_class_icon_path() const override {
return icon_path;
}
#endif // TOOLS_ENABLED

Error reload(bool p_keep_state = false) override;
Expand Down