godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
90.46k stars 21.07k forks source link

@export_tool_button breaks when changing the script contents #97834

Open caphindsight opened 2 weeks ago

caphindsight commented 2 weeks ago

Tested versions

System information

I'm running MacOS, Firefox, web editor

Issue description

After adding a new @export_tool_button to a script, all tool buttons break. Seems to be fixable only by reloading the entire project.

Steps to reproduce

My script:

@tool
extends Node

@export_tool_button("Hello world")
var hello_world := func():
    print("Hello world")

The button appears in the UI. When I click it, the error message appears:

The value of property "hello_world" is Nil, but Callable was expected.

"Soft reload tool script" doesn't help.

After reloading the whole project though, it works as expected:

Hello world

But add another tool button, and both of them break again:

@tool
extends Node

@export_tool_button("Hello world")
var hello_world := func():
    print("Hello world")

@export_tool_button("Hello world2")
var hello_world2 := func():
    print("Hello world2")

Now the first one errors with

Tool button action "<invalid lambda>" is an invalid callable.

and the second one errors with

The value of property "hello_world2" is Nil, but Callable was expected.

Reloading the whole project again, again, fixes both problems.

Minimal reproduction project (MRP)

Sorry I couldn't figure out how to attach the web project, and I running out of time right now. It's basically the default project with just one scene and one script, that I pasted above. I'm confident this will show up in any project.

caphindsight commented 2 weeks ago

Makes me wonder: was it really a good idea to serialize a callable here? IMO just marking a function name as a tool button would be enough and simpler

dalexeev commented 2 weeks ago

@caphindsight See https://github.com/godotengine/godot-proposals/issues/2149#issuecomment-2394937456.

dalexeev commented 1 week ago

I tested the issue, it has several parts.

1. Hot reloading only breaks when adding new lambdas. It does not break if you add a new variable (even an exported one) that does not contain a lambda. The class variable initializers are compiled in a single implicit_initializer function, and we have the following limitation:

https://github.com/godotengine/godot/blob/4c4e67334412f73c9deba5e5d29afa8651418af2/modules/gdscript/gdscript_compiler.cpp#L3203-L3206

If you swap two variables, then after saving the script and reselecting the node, the buttons will swap, but will perform actions in the old order. CC @rune-scape

2. Newly added properties are null by default until you restart the scene/editor, because on hot reload the implicit initializer and constructor are not called again. I'll see what we can do about that, though it might be a bit counterintuitive due to the nature of hot reloading.

I have an idea about default values, and I noticed a bit of refactoring in that area would be desirable (member_default_values ​​and member_default_values_cache, GDScriptCompiler::convert_to_initializer_type() and GDScriptAnalyzer::make_variable_default_value() look like duplicates, GDScript::get_property_default_value() doesn't always return the correct value even in editor builds).

3. Regarding the use of lambda callables, the documentation already warns that it is not advisable for RefCounted classes, we could expand the note.

Note: Avoid storing lambda callables in member variables of RefCounted-based classes (e.g. resources), as this can lead to memory leaks. Use only method callables and optionally Callable.bind or Callable.unbind.

If you use method callables, you will not have problem 1, only 2 (but this can be fixed by restarting the scene/editor).

Alternatively, you can use a getter instead of assigning a lambda to a variable. This is similar to the feature that will be implemented for C# (see #97894), but a bit more verbose since GDScript does not have the => syntactic sugar.

@export_tool_button("Hello world")
var hello_world:
    get:
        return func (): print("Hello world")

With this proposal we could reduce the boilerplate:

@export_tool_button("Hello world")
var hello_world => func (): print("Hello world")

But I'm not sure that's a good enough justification, since the problem is limited to hot reloading and lambda callables; method callables seem pretty robust. Tool scripts may not be comfortable to develop and debug, but once you're done there shouldn't be any problems.

caphindsight commented 1 week ago

Sorry, what's the recommended way to use @export_tool_button that doesn't have any of the problems you listed? Lambda doesn't work, function name still doesn't work because of 2, does getter work always? Are there plans to fix function name or lambda?

rune-scape commented 1 week ago

i have an idea on how to fix 1. by naming lambdas if they are assigned to a variable or member and then hotswapping uniquely named lambdas in the same script

lambda hot swapping is mostly a guessing game for unnamed lambdas, as it too hard to reliably tell where the text has been moved, if it even makes sense to replace, and the new function might not even be compatible

hotswapping lambdas is a delicate process, as there are some internal properties (captures and use_self and others) that can prevent the function pointers from being swapped out inside a callable. this doesnt apply to the getter version, bc the lambda callable is newly created with the correct reloaded function every time