Add `@tool_button` annotation for easily creating inspector buttons.
Supersedes: https://github.com/godotengine/godot/pull/78355 Supersedes: https://github.com/godotengine/godot/pull/59289
@tool
extends Sprite2D
@tool_button("Toot!")
func toot():
print("toot!")
@tool_button("Randomize Color", "ColorRect")
func randomize_color(undo_redo:EditorUndoRedoManager):
#var also_undo_redo := EditorInterface.get_undo_redo()
undo_redo.create_action("Randomize the Sprite2D's color")
undo_redo.add_do_property(self, &"self_modulate", Color(randf(), randf(), randf()))
undo_redo.add_undo_property(self, &"self_modulate", self_modulate)
undo_redo.commit_action()
- Bugsquad edit, closes: https://github.com/godotengine/godot-proposals/issues/2149
If I may recommend, get_undo_redo() could be exposed in a separate PR to keep things more focused. Somewhat related to https://github.com/godotengine/godot/pull/90130 .
get_undo_redo()could be exposed in a separate PR
I wanted to provide all avenues of approach on the first draft of this as I wasn't 100% sure how acceptable the parameterization would be.
I'm happy to drop it from this PR, just say when.
I think both exposing undoredo and passing it as argument is a bit redundant.
Though it's a bit tricky, because referring to EditorUndoRedoManager or EditorInterface in your script will break it once the project is exported. If you want a game script with helper tool button, your undoredo needs to be untyped. Which I guess would be a valid reason to pass it as argument 🤔
From what I see, the argument is optional, so it needs to be better documented.
I'm happy to drop it from this PR, just say when.
Well, since #90130 exists, and will likely get merged sooner, I think you can remove the new method and write the docs as if the other PR was merged already.
Putting this by 4.4 because the prior PR was by 4.4.
Ok the references to EditorInterface.get_undo_redo() have all been dropped and the documentation made a little clearer:
I think the name argument can be optional. I noticed that it gets capitalized, but if it gets capitalized anyway, it could use the method name, no?
I'd make it optional, and if not provided, it could use the capitalized method name. If a name is provided, it should display as is.
Thanks for the review @KoBeWi! I believe I tackled all of your comments, let me know if any of the changes missed the mark.
Here is the updated documentation (it grows!):
I think that tidies up the convoluted word order while still being at the correct verbosity to make sure people are aware of the caveats of exporting a scene with a @tool_button annotation.
Interestingly the way I plan to use this is actually to automatically remove the offending nodes (that are in the editor_only group) from the exported project's .pck with the use of an EditorExportPlugin utilizing _customize_scene. Perhaps something like this workflow should be supported by Godot natively in some fashion?
Maybe there are other ways to handle resolving editor only doodads. For now I think this solution suits me and is worth pushing.
"non-tools build" is a confusing term. Exported project is always "non-tools".
Perhaps something like this workflow should be supported by Godot natively in some fashion?
My idea for this problem was recognizing "editor scripts" and warning the user if they are referenced outside addons folder. Automatically removing nodes is rather unexpected behavior.
Automatically removing nodes is rather unexpected behavior.
No, not like that. Automatically removing nodes that are specifically marked by the user for removal as being editor_only. I do that by placing them in a node group to filter them from the .pck. The process is automated because I don't want to make mistakes. I use it to have all kinds of debug and editor only things that are removed for me on export. It really helps my development workflow and now it has more uses.
My point was that more users are perhaps going to start looking for this kind of feature to be provided by Godot instead of a plugin if they encounter class name resolution errors and the like.
I wish if the icon could have been two ways, built in theme icons or custom icons
if you change the function name and save the scene (and the script alongside it), the button breaks until...
Good find! I'm not sure how to fix that, perhaps a quirk of the engine. Is that because of the way the tool scripts are being handled by the engine that there's only ever one current copy kept around?
I suggest not having any icon instead in this particular situation
In this case, having no icon results in the button looking lopsided like this (which I find aesthetically unacceptable) and requires style changes throughout the editor to support (also not great). The padding and spacing of editor buttons require that they all have icons.
https://github.com/godotengine/godot/blob/88ed6af1e6844908293aa1599421b40870be513c/scene/gui/button.cpp#L347
h_separation seems to contain most of the padding/spacing and the icon does the rest of the work. Without it, it gets weird. I am hesitant to make any changes here. Can follow up PRs tackle this if it is desired?
using EditorUndoRedoManager without create_action() causes the editor to print an empty line
If undo-redo is too difficult for users then they wont use it, I don't know what to do there.
Good find! I'm not sure how to fix that, perhaps a quirk of the engine. Is that because of the way the tool scripts are being handled by the engine that there's only ever one current copy kept around?
That's a problem with exported properties too. When you rename a property, it won't appear in the inspector and the setter is non-functional until you re-select the node. The issue is that inspector does not refresh when script's exposed property list changes. It's a pre-existing bug.
In this case, having no icon results in the button looking lopsided like this (which I find aesthetically unacceptable) and requires style changes throughout the editor to support (also not great).
This could be worked around by committing an empty 16×16 editor icon and using that if an empty string is passed.
Here's such an icon (add it to editor/icons and recompile): Empty.svg.zip
@Calinou I tried some combinations, I can't get any of them to work and I feel uncomfortable about this 😆
I will describe what I've tried taking into account X, where X is the width of [X][Text][Right Padding] when looking at the alignment of a button.
no icon (padding + no h_separation, text fills the full LHS and has correct padding on the right):
16x16 icon (padding + icon width + h_separation + padding, text is pushed right):
0x16 icon or 0x0 icon (padding + h_separation + padding, text is still pushed right ~6px, double padding?):
I even tried a "negative" width icon just to see what would happen, it's clamped to 0 width and looks like the above 0x16.
So a rough measure puts it at 41px left, 35px right with a 0px icon. It's close but infuriatingly not centered.
The editor theming here wasn't designed to have buttons like this with no icon and trying to making it behave with tricks is a little too sneaky feeling. I would be much happier if anyone's distraught enough to fix it properly, with carefully considered changes to either the Button class or the editor theming.
I kind of vibe the icon requirement, the Callable icon has grown on me.
Thanks for the PR to implement the long awaited button feature in tools. 👍
I recently created an addon to have similar tool buttons with actual functionality (bool checkbox). It simply converts particular checkboxes into buttons. https://godotengine.org/asset-library/asset/3313
I just tested your implementation. It works as expected. But I have some comments and ideas. Maybe something for future improvements:
-
I would not show an icon by default. Or allow null or empty string. Btw. can I use custom icons? And where can I find the available build-in icons? Icon only, without text would be also interessting. Centered icon. Bigger icons could increase the button height.
-
Add the option to set a color for the button. I thinking about colors like Danger (red), Success (green), Warning (yellow), Info (blue) and no color. Or custom color by hex color string. Or use the current editor theme variables.
-
Allow null as arguments to fallback to the default. For example, if you only want to change the icon or color which are the 2nd or 3rd arguments.
tool_button(null, null, "Danger")or use dict liketool_button({ "color": "Danger" })(same for icon). -
Use full width for buttons. I think it looks better if multiple buttons are in column. But not tested. Maybe matter of taste.
-
I wonder if we can put e.g. 2 buttons in row? But anyway... I would be happy if I had buttons at all. That's something for future implementations for sure. Just an idea.
-
The inspector panel depends on the width of the button. You can test this by setting a long button text. Not sure if we can wrap the text. Maybe not, because the button width is defined by the text content.
-
You always have to Scene > Reload Saved Scene to update the tool. But this has nothing to do with this implementation. I'm not sure if this is intended, but this is the known behavior when working on a tool script. Maybe it's because preventing executing the script on save. Otherwise it will call e.g. _ready on every save. But sure if this is a problem. But I have to reload the scene on every script export change (inspector UI). It's ok, if you know it.
Just some spontaneous loud thinking. It's great to officially have tool buttons in the next Godot 4. Thanks again. 🙂
Here is the full patch on top of your current changes. Now you can control the export order. I would suggest renaming the annotation to @export_tool_button or @export_button for consistency.
Patch
diff --git a/modules/gdscript/doc_classes/@GDScript.xml b/modules/gdscript/doc_classes/@GDScript.xml
index c28ec2c58b..5cf811145b 100644
--- a/modules/gdscript/doc_classes/@GDScript.xml
+++ b/modules/gdscript/doc_classes/@GDScript.xml
@@ -737,22 +737,24 @@
</annotation>
<annotation name="@tool_button">
<return type="void" />
- <param index="0" name="text" type="String" default="""" />
- <param index="1" name="icon" type="StringName" default="""" />
+ <param index="0" name="method" type="StringName" />
+ <param index="1" name="text" type="String" default="""" />
+ <param index="2" name="icon" type="StringName" default="""" />
<description>
- Mark a function to be displayed as a clickable button in the Inspector when running the editor.
- If [param text] is specified it is used as the label text of the button. If [param text] is omitted, the label text is derived from the name of the annotated function and automatically capitalized.
+ Add a clickable button in the Inspector when running the editor. When the button is pressed, the specified [param method] is called.
+ If [param text] is specified it is used as the label text of the button. If [param text] is omitted, the label text is derived from the [param method] name and automatically capitalized.
If [param icon] is specified, it is used to fetch an icon for the button via [method Control.get_theme_icon], from the [code]"EditorIcons"[/code] theme type. If [param icon] is omitted, the default [code]"Callable"[/code] icon is used instead.
- The first argument of the annotated function is optional and is the [EditorUndoRedoManager]. Consider using the optionally passed [EditorUndoRedoManager] to allow the action to be reverted safely. The code snippet below demonstrates this:
+ The first argument of the function [param method] is optional and is the [EditorUndoRedoManager]. Consider using the optionally passed [EditorUndoRedoManager] to allow the action to be reverted safely. The code snippet below demonstrates this:
[codeblock]
@tool
extends Sprite2D
- @tool_button
+ @tool_button(&"hello")
+ @tool_button(&"randomize_color", "Randomize the color!", &"ColorRect")
+
func hello():
print("Hello world!")
- @tool_button("Randomize the color!", "ColorRect")
func randomize_color(undo_redo):
undo_redo.create_action("Randomized Sprite2D Color")
undo_redo.add_do_property(self, "self_modulate", Color(randf(), randf(), randf()))
diff --git a/modules/gdscript/gdscript.cpp b/modules/gdscript/gdscript.cpp
index 3e70bab0b9..ec42992aec 100644
--- a/modules/gdscript/gdscript.cpp
+++ b/modules/gdscript/gdscript.cpp
@@ -567,8 +567,7 @@ bool GDScript::_update_exports(bool *r_err, bool p_recursive_call, PlaceHolderSc
case GDScriptParser::ClassNode::Member::SIGNAL: {
_signals[member.signal->identifier->name] = member.signal->method_info;
} break;
- case GDScriptParser::ClassNode::Member::GROUP:
- case GDScriptParser::ClassNode::Member::TOOL_BUTTON: {
+ case GDScriptParser::ClassNode::Member::META: {
members_cache.push_back(member.annotation->export_info);
} break;
default:
diff --git a/modules/gdscript/gdscript_analyzer.cpp b/modules/gdscript/gdscript_analyzer.cpp
index 58feb98f6b..9b07c7d4a0 100644
--- a/modules/gdscript/gdscript_analyzer.cpp
+++ b/modules/gdscript/gdscript_analyzer.cpp
@@ -1203,8 +1203,7 @@ void GDScriptAnalyzer::resolve_class_member(GDScriptParser::ClassNode *p_class,
resolve_class_inheritance(member.m_class, p_source);
}
break;
- case GDScriptParser::ClassNode::Member::GROUP:
- case GDScriptParser::ClassNode::Member::TOOL_BUTTON:
+ case GDScriptParser::ClassNode::Member::META:
// No-op, but needed to silence warnings.
break;
case GDScriptParser::ClassNode::Member::UNDEFINED:
@@ -1381,8 +1380,8 @@ void GDScriptAnalyzer::resolve_class_body(GDScriptParser::ClassNode *p_class, co
resolve_function_body(member.variable->setter);
}
}
- } else if (member.type == GDScriptParser::ClassNode::Member::GROUP) {
- // Apply annotation (`@export_{category,group,subgroup}`).
+ } else if (member.type == GDScriptParser::ClassNode::Member::META) {
+ // Apply annotation (`@export_{category,group,subgroup}`, `@tool_button`).
resolve_annotation(member.annotation);
member.annotation->apply(parser, nullptr, p_class);
}
diff --git a/modules/gdscript/gdscript_compiler.cpp b/modules/gdscript/gdscript_compiler.cpp
index b6503aad92..9d152dd194 100644
--- a/modules/gdscript/gdscript_compiler.cpp
+++ b/modules/gdscript/gdscript_compiler.cpp
@@ -2859,11 +2859,10 @@ Error GDScriptCompiler::_prepare_compilation(GDScript *p_script, const GDScriptP
p_script->constants.insert(name, enum_n->dictionary);
} break;
- case GDScriptParser::ClassNode::Member::GROUP:
- case GDScriptParser::ClassNode::Member::TOOL_BUTTON: {
+ case GDScriptParser::ClassNode::Member::META: {
const GDScriptParser::AnnotationNode *annotation = member.annotation;
// Avoid name conflict. See GH-78252.
- StringName name = vformat("@group_%d_%s", p_script->members.size(), annotation->export_info.name);
+ StringName name = vformat("@meta_%d_%s", p_script->members.size(), annotation->export_info.name);
// This is not a normal member, but we need this to keep indices in order.
GDScript::MemberInfo minfo;
diff --git a/modules/gdscript/gdscript_editor.cpp b/modules/gdscript/gdscript_editor.cpp
index 3ee76b2451..fb169ce50d 100644
--- a/modules/gdscript/gdscript_editor.cpp
+++ b/modules/gdscript/gdscript_editor.cpp
@@ -1126,8 +1126,7 @@ static void _find_identifiers_in_class(const GDScriptParser::ClassNode *p_class,
}
option = ScriptLanguage::CodeCompletionOption(member.signal->identifier->name, ScriptLanguage::CODE_COMPLETION_KIND_SIGNAL, location);
break;
- case GDScriptParser::ClassNode::Member::GROUP:
- case GDScriptParser::ClassNode::Member::TOOL_BUTTON:
+ case GDScriptParser::ClassNode::Member::META:
break; // No-op, but silences warnings.
case GDScriptParser::ClassNode::Member::UNDEFINED:
break;
@@ -2428,8 +2427,7 @@ static bool _guess_identifier_type_from_base(GDScriptParser::CompletionContext &
r_type.type.class_type = member.m_class;
r_type.type.is_meta_type = true;
return true;
- case GDScriptParser::ClassNode::Member::GROUP:
- case GDScriptParser::ClassNode::Member::TOOL_BUTTON:
+ case GDScriptParser::ClassNode::Member::META:
return false; // No-op, but silences warnings.
case GDScriptParser::ClassNode::Member::UNDEFINED:
return false; // Unreachable.
diff --git a/modules/gdscript/gdscript_parser.cpp b/modules/gdscript/gdscript_parser.cpp
index e50d249347..4be1821c45 100644
--- a/modules/gdscript/gdscript_parser.cpp
+++ b/modules/gdscript/gdscript_parser.cpp
@@ -100,7 +100,7 @@ GDScriptParser::GDScriptParser() {
register_annotation(MethodInfo("@static_unload"), AnnotationInfo::SCRIPT, &GDScriptParser::static_unload_annotation);
// Member annotations.
register_annotation(MethodInfo("@onready"), AnnotationInfo::VARIABLE, &GDScriptParser::onready_annotation);
- register_annotation(MethodInfo("@tool_button", PropertyInfo(Variant::STRING, "text"), PropertyInfo(Variant::STRING_NAME, "icon")), AnnotationInfo::FUNCTION, &GDScriptParser::tool_button_annotation, varray("", ""));
+ register_annotation(MethodInfo("@tool_button", PropertyInfo(Variant::STRING_NAME, "method"), PropertyInfo(Variant::STRING, "text"), PropertyInfo(Variant::STRING_NAME, "icon")), AnnotationInfo::STANDALONE, &GDScriptParser::tool_button_annotation, varray("", ""));
// Export annotations.
register_annotation(MethodInfo("@export"), AnnotationInfo::VARIABLE, &GDScriptParser::export_annotations<PROPERTY_HINT_NONE, Variant::NIL>);
register_annotation(MethodInfo("@export_enum", PropertyInfo(Variant::STRING, "names")), AnnotationInfo::VARIABLE, &GDScriptParser::export_annotations<PROPERTY_HINT_ENUM, Variant::NIL>, varray(), true);
@@ -631,14 +631,14 @@ void GDScriptParser::parse_program() {
if (previous.type != GDScriptTokenizer::Token::NEWLINE) {
push_error(R"(Expected newline after a standalone annotation.)");
}
- if (annotation->name == SNAME("@export_category") || annotation->name == SNAME("@export_group") || annotation->name == SNAME("@export_subgroup")) {
- head->add_member_group(annotation);
+ if (annotation->name == SNAME("@export_category") || annotation->name == SNAME("@export_group") || annotation->name == SNAME("@export_subgroup") || annotation->name == SNAME("@tool_button")) {
+ head->add_meta_member(annotation);
// This annotation must appear after script-level annotations and `class_name`/`extends`,
// so we stop looking for script-level stuff.
can_have_class_or_extends = false;
break;
} else {
- // For potential non-group standalone annotations.
+ // For potential other standalone annotations.
push_error(R"(Unexpected standalone annotation.)");
}
} else {
@@ -1025,10 +1025,10 @@ void GDScriptParser::parse_class_body(bool p_is_multiline) {
if (previous.type != GDScriptTokenizer::Token::NEWLINE) {
push_error(R"(Expected newline after a standalone annotation.)");
}
- if (annotation->name == SNAME("@export_category") || annotation->name == SNAME("@export_group") || annotation->name == SNAME("@export_subgroup")) {
- current_class->add_member_group(annotation);
+ if (annotation->name == SNAME("@export_category") || annotation->name == SNAME("@export_group") || annotation->name == SNAME("@export_subgroup") || annotation->name == SNAME("@tool_button")) {
+ current_class->add_meta_member(annotation);
} else {
- // For potential non-group standalone annotations.
+ // For potential other standalone annotations.
push_error(R"(Unexpected standalone annotation.)");
}
} else { // `AnnotationInfo::CLASS_LEVEL`.
@@ -4175,41 +4175,55 @@ bool GDScriptParser::onready_annotation(AnnotationNode *p_annotation, Node *p_ta
bool GDScriptParser::tool_button_annotation(AnnotationNode *p_annotation, Node *p_target, ClassNode *p_class) {
#ifdef TOOLS_ENABLED
- ERR_FAIL_COND_V_MSG(p_target->type != Node::FUNCTION, false, R"("@tool_button" annotation can only be applied to functions.)");
-
- FunctionNode *func = static_cast<FunctionNode *>(p_target);
+ ERR_FAIL_COND_V(p_annotation->resolved_arguments.is_empty(), false);
if (!is_tool()) {
- push_error("Tool buttons can only be used in tool scripts (add `@tool` to the top of the script).", p_annotation);
+ push_error(R"(Tool buttons can only be used in tool scripts (add "@tool" to the top of the script).)", p_annotation);
+ return false;
+ }
+
+ const StringName method = p_annotation->resolved_arguments[0];
+
+ if (method == StringName()) {
+ push_error(R"(Expected function name for "@tool_button" method.)", p_annotation);
return false;
}
- if (func->has_tool_button) {
- push_error(vformat(R"("%s" annotation can only be used once per function.)", p_annotation->name), p_annotation);
+ if (!current_class->has_member(method)) {
+ push_error(vformat(R"(The function "%s" is not found for "@tool_button" method.)", method), p_annotation);
+ return false;
+ }
+
+ const ClassNode::Member &member = current_class->get_member(method);
+
+ if (member.type != ClassNode::Member::FUNCTION) {
+ push_error(vformat(R"(The %s "%s" cannot be used as "@tool_button" method.)", member.get_type_name(), method), p_annotation);
return false;
}
+ const FunctionNode *func = member.function;
+
p_annotation->export_info.name = "@tool_button_" + func->identifier->name;
p_annotation->export_info.type = Variant::Type::CALLABLE;
p_annotation->export_info.usage = PROPERTY_USAGE_EDITOR | PROPERTY_USAGE_INTERNAL;
- if (func->parameters.size() >= 1 && func->parameters[0]->datatype_specifier != nullptr && func->parameters[0]->datatype_specifier->type_chain[0]->name != "EditorUndoRedoManager") {
- push_error("Tool button methods should have their first argument typed as an EditorUndoRedoManager.", func);
- return false;
+ if (!func->parameters.is_empty() && func->parameters[0]->datatype_specifier != nullptr) {
+ const DataType arg_type = func->parameters[0]->get_datatype();
+ if (!arg_type.is_variant() && (arg_type.kind != DataType::NATIVE || arg_type.native_type != "EditorUndoRedoManager")) {
+ push_error(R"(Tool button function must have first argument of type "EditorUndoRedoManager".)", p_annotation);
+ return false;
+ }
}
- // Build the hint string (format: "<method>","<text>","[icon]")
- String hint_string = func->identifier->name;
- if (p_annotation->resolved_arguments.size() > 0) {
- hint_string += "," + p_annotation->resolved_arguments[0].operator String(); // Button text.
- }
+ // Build the hint string (format: `<method>[,<text>[,<icon>]]`).
+ String hint_string = method;
if (p_annotation->resolved_arguments.size() > 1) {
- hint_string += "," + p_annotation->resolved_arguments[1].operator String(); // Button icon.
+ hint_string += "," + p_annotation->resolved_arguments[1].operator String(); // Button text.
+ }
+ if (p_annotation->resolved_arguments.size() > 2) {
+ hint_string += "," + p_annotation->resolved_arguments[2].operator String(); // Button icon.
}
p_annotation->export_info.hint_string = hint_string;
-
- current_class->add_tool_button_member(p_annotation);
- func->has_tool_button = true;
#endif // TOOLS_ENABLED
return true; // Only available in editor.
@@ -4714,9 +4728,7 @@ bool GDScriptParser::export_custom_annotation(AnnotationNode *p_annotation, Node
template <PropertyUsageFlags t_usage>
bool GDScriptParser::export_group_annotations(AnnotationNode *p_annotation, Node *p_target, ClassNode *p_class) {
- if (p_annotation->resolved_arguments.is_empty()) {
- return false;
- }
+ ERR_FAIL_COND_V(p_annotation->resolved_arguments.is_empty(), false);
p_annotation->export_info.name = p_annotation->resolved_arguments[0];
@@ -5544,9 +5556,8 @@ void GDScriptParser::TreePrinter::print_class(ClassNode *p_class) {
break;
case ClassNode::Member::ENUM_VALUE:
break; // Nothing. Will be printed by enum.
- case ClassNode::Member::GROUP:
- case ClassNode::Member::TOOL_BUTTON:
- break; // Nothing. Groups are only used by inspector.
+ case ClassNode::Member::META:
+ break; // Nothing. Meta properties are only used by inspector.
case ClassNode::Member::UNDEFINED:
push_line("<unknown member>");
break;
diff --git a/modules/gdscript/gdscript_parser.h b/modules/gdscript/gdscript_parser.h
index 657aa09d2a..3d47f1d477 100644
--- a/modules/gdscript/gdscript_parser.h
+++ b/modules/gdscript/gdscript_parser.h
@@ -563,8 +563,7 @@ public:
VARIABLE,
ENUM,
ENUM_VALUE, // For unnamed enums.
- GROUP, // For member grouping.
- TOOL_BUTTON,
+ META, // For exporting groups and tool buttons.
};
Type type = UNDEFINED;
@@ -600,8 +599,7 @@ public:
return m_enum->identifier->name;
case ENUM_VALUE:
return enum_value.identifier->name;
- case GROUP:
- case TOOL_BUTTON:
+ case META:
return annotation->export_info.name;
}
return "";
@@ -625,10 +623,8 @@ public:
return "enum";
case ENUM_VALUE:
return "enum value";
- case GROUP:
- return "group";
- case TOOL_BUTTON:
- return "tool button";
+ case META:
+ return "meta property";
}
return "";
}
@@ -649,8 +645,7 @@ public:
return m_enum->start_line;
case SIGNAL:
return signal->start_line;
- case GROUP:
- case TOOL_BUTTON:
+ case META:
return annotation->start_line;
case UNDEFINED:
ERR_FAIL_V_MSG(-1, "Reaching undefined member type.");
@@ -674,8 +669,7 @@ public:
return enum_value.identifier->get_datatype();
case SIGNAL:
return signal->get_datatype();
- case GROUP:
- case TOOL_BUTTON:
+ case META:
return DataType();
case UNDEFINED:
return DataType();
@@ -699,8 +693,7 @@ public:
return enum_value.identifier;
case SIGNAL:
return signal;
- case GROUP:
- case TOOL_BUTTON:
+ case META:
return annotation;
case UNDEFINED:
return nullptr;
@@ -738,8 +731,8 @@ public:
type = ENUM_VALUE;
enum_value = p_enum_value;
}
- Member(AnnotationNode *p_annotation, Member::Type p_type) {
- type = p_type;
+ Member(AnnotationNode *p_annotation) {
+ type = META;
annotation = p_annotation;
}
};
@@ -793,15 +786,11 @@ public:
members_indices[p_enum_value.identifier->name] = members.size();
members.push_back(Member(p_enum_value));
}
- void add_member_group(AnnotationNode *p_annotation_node) {
+ void add_meta_member(AnnotationNode *p_annotation_node) {
// Avoid name conflict. See GH-78252.
- StringName name = vformat("@group_%d_%s", members.size(), p_annotation_node->export_info.name);
+ StringName name = vformat("@meta_%d_%s", members.size(), p_annotation_node->export_info.name);
members_indices[name] = members.size();
- members.push_back(Member(p_annotation_node, Member::Type::GROUP));
- }
- void add_tool_button_member(AnnotationNode *p_annotation_node) {
- members_indices[p_annotation_node->export_info.name] = members.size();
- members.push_back(Member(p_annotation_node, Member::Type::TOOL_BUTTON));
+ members.push_back(Member(p_annotation_node));
}
ClassNode() {
@@ -869,7 +858,6 @@ public:
Vector<Variant> default_arg_values;
#ifdef TOOLS_ENABLED
MemberDocData doc_data;
- bool has_tool_button = false;
#endif // TOOLS_ENABLED
bool resolved_signature = false;
diff --git a/modules/gdscript/language_server/gdscript_extend_parser.cpp b/modules/gdscript/language_server/gdscript_extend_parser.cpp
index 88f0816e4d..25f87b70cd 100644
--- a/modules/gdscript/language_server/gdscript_extend_parser.cpp
+++ b/modules/gdscript/language_server/gdscript_extend_parser.cpp
@@ -454,8 +454,7 @@ void ExtendGDScriptParser::parse_class_symbol(const GDScriptParser::ClassNode *p
parse_class_symbol(m.m_class, symbol);
r_symbol.children.push_back(symbol);
} break;
- case ClassNode::Member::GROUP:
- case ClassNode::Member::TOOL_BUTTON:
+ case ClassNode::Member::META:
break; // No-op, but silences warnings.
case ClassNode::Member::UNDEFINED:
break; // Unreachable.
@@ -989,8 +988,7 @@ Dictionary ExtendGDScriptParser::dump_class_api(const GDScriptParser::ClassNode
methods.append(dump_function_api(m.function));
}
} break;
- case ClassNode::Member::GROUP:
- case ClassNode::Member::TOOL_BUTTON:
+ case ClassNode::Member::META:
break; // No-op, but silences warnings.
case ClassNode::Member::UNDEFINED:
break; // Unreachable.
It would also be nice to add tests (error validation). Although you can't test the editor functionality, you can use @tool in tests, see parser/errors/duplicate_tool.gd.
Thanks for the review and suggestions @dalexeev! I've gone ahead and included the changes of GROUP/TOOL_BUTTON -> META, written some cursory validation tests, renamed @tool_button to @export_tool_button and went with the new behaviour.
Given these changes now force the explicit inclusion of the method name (as a part of the annotation, as a magic string) I would like to ask about how the team feel about something like this (not an exact implementation, just ideas) to help alleviate the magic string issue a little:
diff --git a/modules/gdscript/gdscript_analyzer.cpp b/modules/gdscript/gdscript_analyzer.cpp
index 8fc2d62c07..41ea62e8b3 100644
--- a/modules/gdscript/gdscript_analyzer.cpp
+++ b/modules/gdscript/gdscript_analyzer.cpp
@@ -1603,13 +1603,18 @@ void GDScriptAnalyzer::resolve_annotation(GDScriptParser::AnnotationNode *p_anno
reduce_expression(argument);
- if (!argument->is_constant) {
+ Variant value;
+
+ if (argument->type == GDScriptParser::Node::IDENTIFIER) {
+ GDScriptParser::IdentifierNode *identifier = static_cast<GDScriptParser::IdentifierNode*>(p_annotation->arguments[i]);
+ value = identifier->name;
+ } else if (argument->is_constant) {
+ value = argument->reduced_value;
+ } else {
push_error(vformat(R"(Argument %d of annotation "%s" isn't a constant expression.)", i + 1, p_annotation->name), argument);
return;
}
- Variant value = argument->reduced_value;
-
if (value.get_type() != argument_info.type) {
#ifdef DEBUG_ENABLED
if (argument_info.type == Variant::INT && value.get_type() == Variant::FLOAT) {
to change, and permit, either of the following to now compile:
Without this change the analyzer complains that the name of a function is not a constant expression, which I don't think is quite true. Perhaps this error, in the case of an identifier name, is unwarranted?
Either way this PR is shaping up pretty nice! Thanks!
Given these changes now force the explicit inclusion of the method name (as a part of the annotation, as a magic string) I would like to ask about how the team feel about something like this
This has been discussed before. Annotations support constant expressions, so treating an identifier as a quoteless string would be inconsistent and confusing. See:
- #71634
- #76512
Actually, it doesn't matter if it's a string literal or an "identifier". In both cases it's a string. As long as there are static checks, you shouldn't worry, it's just an aesthetic issue.
Without this change the analyzer complains that the name of a function is not a constant expression, which I don't think is quite true
It's correct. my_func is syntactic sugar for Callable(self, &"my_func"), self is not a constant expression. There are no function/method pointers in GDScript, so you cannot represent a method without an instance as a first-class value, only as String/StringName. Because of this, there is no point in using Callable as an annotation argument type, only global utility functions are treated as constant expressions.
Note that to fully dynamically add tool buttons we would need to be able to pass an argument from hint string to the function, since there is no _call() to dynamically dispatch calls.
It would be good to somehow bind arguments to the called function, especially in the dynamic case to permit multiple functions to hit the same end-point. I am not sure how to do this in a satisfying fashion. In your comment, what argument is unable to be passed here normally?
Alternatively, we could implement this as a more verbose option
Annotating a callable does appeal to me more than an otherwise magic method name string even if it is a little wordy. I might try changing it to do that instead.
@dalexeev Is this what you meant by using property hints, in b53967b? My original use case was quite minimal, I just need to be able to trigger something cleanly in the editor, so when I salvaged this I just got it working to the point that I could use it for myself. I do like this, not using a property hint feels like an oversight.
Code from the picture
@tool
extends Node2D
@export var first:int = 123
@export_tool_button("test")
@export_tool_button("test_disabled")
@export var last:int = 42
func _validate_property(property: Dictionary) -> void:
if property.name == "@tool_button_test": # hide the test button
property.usage = property.usage & ~PROPERTY_USAGE_EDITOR
if property.name == "@tool_button_test_disabled":
property.usage = property.usage | PROPERTY_USAGE_READ_ONLY
func _get_property_list() -> Array[Dictionary]:
return [{
"name": "cool_dynamic_tool_button",
"type": TYPE_NIL,
"hint": PROPERTY_HINT_TOOL_BUTTON,
"hint_string": "dynamic_button",
}]
func test():
print("toot")
func test_disabled():
print("can't touch this")
func dynamic_button():
print("dynamic button")
This would allow you to use lambdas:
@export_tool_button("Click me") var click_button: Callable = func (): print("Clicked!")
Unfortunately, I realized that this could be problematic. GDScriptLambdaSelfCallable holds a reference to self, which will leak memory for Resources if the self-lambda is assigned to a class member. This is not a problem for Node, GDScriptLambdaCallable, and the standard Callable, so we could document the pitfall of self-lambdas on RefCounted objects. But it still makes the idea less appealing.
It would be good to somehow bind arguments to the called function, especially in the dynamic case to permit multiple functions to hit the same end-point. I am not sure how to do this in a satisfying fashion. In your comment, what argument is unable to be passed here normally?
I think something like this would work, although it doesn't allow you to bind arbitrary arguments, unlike Callable:
@tool_button(method: StringName, text: String = "", icon: StringName = "", arg: String = "")
# hint string format: <method>[,<text>[,<icon>[,<arg>]]]
@export_tool_button(&"test", "Click me", &"", "123")
func test(_undo_redo, arg):
prints("Hello world!", arg)
GDScriptLambdaSelfCallable holds a reference to self, which will leak memory
There are too many caveats to annotate Callables at present, it's very fiddly. What is here otherwise works very well so far. If self-referencing ever becomes a non-issue it would be a nice change to consider.
I think I'm done with the changes necessary for property hint backing and dynamic addition of tool buttons! With the addition of the arg parameter you can literally just call existing setters/methods, it's very nice (see: @export_tool_button(&"set_modulate", "Make Green", &"ColorRect", "Color(0, 1, 0, 1)")).
This now more than meets my own needs, let me know if I've missed/broken anything.
@tool
extends Sprite2D
@export var first:int = 123
@export_tool_button(&"test_hidden")
@export_tool_button(&"test_disabled", "Disabled", &"Stop")
@export_tool_button(&"test_undoredo", "", &"UndoRedo")
@export_tool_button(&"set_modulate", "Make Green", &"ColorRect", "Color(0, 1, 0, 1)")
@export_tool_button(&"set_modulate", "Clear Modulation", &"Clear", "Color(1, 1, 1, 1)")
@export var last:int = 42
func _validate_property(property: Dictionary) -> void:
if property.name == "@tool_button_test_hidden": # hide the test button
property.usage = property.usage & ~PROPERTY_USAGE_EDITOR
if property.name == "@tool_button_test_disabled":
property.usage = property.usage | PROPERTY_USAGE_READ_ONLY
func _get_property_list() -> Array[Dictionary]:
var properties:Array[Dictionary]
for i in 3:
properties.append({
"name": "cool_dynamic_tool_button_%d" % i,
"type": TYPE_NIL,
"hint": PROPERTY_HINT_TOOL_BUTTON,
"hint_string": "test_dynamic,%s,Variant,Vector2(123, %d)" % ["Dynamic Button %d" % i, i],
})
return properties
func test_hidden():
print("toot")
func test_disabled():
print("can't touch this")
func test_undoredo(undo_redo:EditorUndoRedoManager):
prints("undoredo", undo_redo)
func test_dynamic(what:Variant):
prints("dynamic button", type_string(typeof(what)), typeof(what), what)
@Macksaur Thank you for your patience! I'm sorry to make you do so much work, but I think the current version is overcomplicated, especially using str_to_var() to decode argument.
I tried to change it to be as simple and functional as possible by removing unnecessary arguments and checks (since Godot is quite dynamic with callables). Please see https://github.com/dalexeev/godot/commit/082706409b3fa8e1eb010cd513fc12c5d12e9f02 and if you agree with this approach, feel free to rebase your PR branch using the commit.
Updated testing script
@tool
extends Sprite2D
@export var first: int = 123
@export_tool_button("Hidden") var hidden_action = test_hidden
@export_tool_button("Disabled", "Stop") var stop_action = test_disabled
@export_tool_button("UndoRedo", "UndoRedo") var undoredo_action = test_undoredo
@export_tool_button("Make Green", "ColorRect")
var make_green_action = set_self_modulate.bind(Color.GREEN)
@export_tool_button("Clear Modulation", "Clear")
var clear_modulation_action = set_self_modulate.bind(Color.WHITE)
@export var last: int = 42
func _validate_property(property: Dictionary) -> void:
if property.name == "hidden_action": # hide the test button
property.usage = property.usage & ~PROPERTY_USAGE_EDITOR
if property.name == "stop_action":
property.usage = property.usage | PROPERTY_USAGE_READ_ONLY
func _get_property_list() -> Array[Dictionary]:
var properties:Array[Dictionary]
for i in 3:
properties.append({
"name": "cool_dynamic_tool_button_%d" % i,
"type": TYPE_CALLABLE,
"hint": PROPERTY_HINT_TOOL_BUTTON,
"hint_string": "Dynamic Button %d" % i,
"usage": PROPERTY_USAGE_EDITOR,
})
return properties
func _get(property: StringName) -> Variant:
if property.begins_with("cool_dynamic_tool_button_"):
return test_dynamic.bind(property.trim_prefix("cool_dynamic_tool_button_"))
return null
func test_hidden():
print("toot")
func test_disabled():
print("can't touch this")
func test_undoredo():
prints("undoredo", EditorInterface.get_editor_undo_redo())
func test_dynamic(what):
prints("dynamic button", type_string(typeof(what)), typeof(what), what)
Another example
@tool
extends Node
@export_tool_button("Test") var test_action = test.bind(1)
func test(x):
print(x)
test_action = test.bind(x + 1)
Thanks @Macksaur, @jordi-star, @fire, @dalexeev, and everyone involved in the thorough review and testing!
I'm currently looking into bringing this to C# :v:
Not sure how the workflow for documenting new feature works, but in case it wasn't considered I wanted to mention that these pages need to be updated with information about the new annotation:
GDScript exported properties - https://docs.godotengine.org/en/latest/tutorials/scripting/gdscript/gdscript_exports.html GDScript reference (Annotations heading) - https://docs.godotengine.org/en/stable/tutorials/scripting/gdscript/gdscript_basics.html#annotations
This feature is extremely useful! But I couldn't get it to work in beta 4, even following an example from the documentation.
Try reloading the scene.
@Kartopod Thanks for the reminder to add docs. I think the process for ensuring that manual docs are added for new PRs is currently a little inconsistent. In this case I've made an issue to track it: https://github.com/godotengine/godot-docs/issues/10303, which anyone can contribute. If it doesn't get a volunteer contributor before 4.4, I'll make the changes myself