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

Add @export_tool_button annotation for easily creating inspector buttons. #96290

Merged
merged 1 commit into from
Sep 28, 2024

Conversation

Macksaur
Copy link
Contributor

@Macksaur Macksaur commented Aug 29, 2024

Supersedes: #78355
Supersedes: #59289

image

Bugsquad edit: The syntax below is not the one that was merged in the end, see #96290 (comment) for updated examples.

image

@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()

export action callable

@Mickeon
Copy link
Contributor

Mickeon commented Aug 29, 2024

If I may recommend, get_undo_redo() could be exposed in a separate PR to keep things more focused. Somewhat related to #90130 .

@Macksaur
Copy link
Contributor Author

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.

@KoBeWi
Copy link
Member

KoBeWi commented Aug 29, 2024

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.

@Mickeon Mickeon modified the milestones: 4.x, 4.4 Aug 29, 2024
@Mickeon
Copy link
Contributor

Mickeon commented Aug 29, 2024

Putting this by 4.4 because the prior PR was by 4.4.

@Macksaur
Copy link
Contributor Author

Ok the references to EditorInterface.get_undo_redo() have all been dropped and the documentation made a little clearer:

image

@KoBeWi

This comment was marked as resolved.

@Macksaur Macksaur force-pushed the export-action-callable branch 2 times, most recently from 607244e to b06e404 Compare August 29, 2024 23:06
@Macksaur
Copy link
Contributor Author

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!):

image

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.

@KoBeWi
Copy link
Member

KoBeWi commented Aug 30, 2024

"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.

@Macksaur Macksaur force-pushed the export-action-callable branch 4 times, most recently from 7f4b4bf to 19cf6d9 Compare September 26, 2024 01:39
@Macksaur
Copy link
Contributor Author

Macksaur commented Sep 26, 2024

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:

image

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?

image


Either way this PR is shaping up pretty nice! Thanks!

@dalexeev
Copy link
Member

dalexeev commented Sep 26, 2024

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:

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.

Copy link
Member

@dalexeev dalexeev left a comment

Choose a reason for hiding this comment

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

GDScript changes look good to me. But I have a question about the core/editor part. Why is it implemented as EditorInspectorPlugin and not a new PROPERTY_HINT_TOOL_BUTTON handled in editor_properties.cpp? In my opinion, an explicit property hint is better than implicit handling of properties with Callable type and empty value. It would allow to document hint format properly, for example for dynamic addition of tool buttons via _get_property_list().

Also, we could not limit tool buttons to GDScript, but add them to core, GDExtension and C# as well. Yes, it is possible with implicit Callable handling, but explicit and documented property hint looks better.

#define ADD_TOOL_BUTTON(m_method, m_label, m_icon) ::ClassDB::add_property(get_class_static(), PropertyInfo(Variant::NIL, "", PROPERTY_HINT_TOOL_BUTTON, String(m_method) + String(m_label) + String(m_icon), PROPERTY_USAGE_EDITOR | PROPERTY_USAGE_INTERNAL), StringName(), StringName())

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.

Alternatively, we could implement this as a more verbose option:

@export_tool_button("Click me") var click_button: Callable = click_action

func click_action():
    print("Clicked!")

This would allow you to use lambdas:

@export_tool_button("Click me") var click_button: Callable = func ():
    print("Clicked!")

@Macksaur Macksaur requested a review from a team as a code owner September 26, 2024 16:39
@Macksaur
Copy link
Contributor Author

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.

image

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")

@dalexeev
Copy link
Member

dalexeev commented Sep 26, 2024

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)

@Macksaur
Copy link
Contributor Author

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.

image

@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)

@dalexeev
Copy link
Member

dalexeev commented Sep 27, 2024

@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 dalexeev@0827064 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)

@Macksaur Macksaur force-pushed the export-action-callable branch 2 times, most recently from 2c422d5 to 645a230 Compare September 27, 2024 20:51
@Macksaur Macksaur changed the title Add @tool_button annotation for easily creating inspector buttons. Add @export_tool_button annotation for easily creating inspector buttons. Sep 27, 2024
Copy link
Member

@dalexeev dalexeev left a comment

Choose a reason for hiding this comment

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

Looks good to me. @Macksaur Thanks again for your work and patience!

editor/plugins/tool_button_editor_plugin.h Outdated Show resolved Hide resolved
…ttons

Co-authored-by: jordi <creptthrust@gmail.com>
Co-authored-by: K. S. Ernest (iFire) Lee <ernest.lee@chibifire.com>
Co-authored-by: Mack <86566939+Macksaur@users.noreply.github.com>
@akien-mga akien-mga merged commit a11f970 into godotengine:master Sep 28, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks @Macksaur, @jordi-star, @fire, @dalexeev, and everyone involved in the thorough review and testing!

@paulloz
Copy link
Member

paulloz commented Oct 3, 2024

I'm currently looking into bringing this to C# ✌️

@Kartopod
Copy link

Kartopod commented Oct 4, 2024

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

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.

Add a way to export clickable actions to the inspector