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

@export_tool_button breaks when changing the script contents (lambda hot-reloading) #97834

Open
caphindsight opened this issue Oct 5, 2024 · 13 comments · May be fixed by #98221
Open

@export_tool_button breaks when changing the script contents (lambda hot-reloading) #97834

caphindsight opened this issue Oct 5, 2024 · 13 comments · May be fixed by #98221

Comments

@caphindsight
Copy link

Tested versions

  • Reproducible in 4.4dev3, the web editor

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.

@dalexeev dalexeev added this to the 4.4 milestone Oct 5, 2024
@github-project-automation github-project-automation bot moved this to For team assessment in GDScript Issue Triage Oct 5, 2024
@dalexeev dalexeev moved this from For team assessment to In progress / Assigned in GDScript Issue Triage Oct 5, 2024
@dalexeev dalexeev self-assigned this Oct 5, 2024
@caphindsight
Copy link
Author

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
Copy link
Member

dalexeev commented Oct 6, 2024

@dalexeev
Copy link
Member

dalexeev commented Oct 10, 2024

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:

if (p_new_infos != nullptr && p_new_infos->size() == p_old_infos.size()) {
// For now only attempt if the size is the same.
new_info = &p_new_infos->get(i);
}

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
Copy link
Author

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
Copy link
Contributor

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

@Burloe
Copy link

Burloe commented Feb 23, 2025

I'm getting similar issues when trying to use tool buttons on 4.4.beta4. I always get an error message until I restart/reload the project.

Image

@caphindsight
Copy link
Author

caphindsight commented Feb 23, 2025

I'm getting similar issues when trying to use tool buttons on 4.4.beta4. I always get an error message until I restart/reload the project.

Image

Please file a release blocking bug report, otherwise it's likely to ship broken with 4.4.

@caphindsight
Copy link
Author

@akien-mga this bug makes tool buttons unusable, should we roll back tool buttons in 4.4 if this is 4.5?

@akien-mga
Copy link
Member

Since reloading the project seems to fix the issue, I think the feature is still worth having, even if hot reloading is broken.

Hot reloading for tool scripts has never been very reliable, especially with lambdas.

I get that the issue is a bit frustrating but it's something that will mostly be a hassle while developing custom tooling. Once the custom tooling is made, the whole team can use it just fine.

@dalexeev
Copy link
Member

dalexeev commented Mar 3, 2025

This is one of the reasons why it is hard for us to fix this, let alone hot reload lambdas. So using member function callable or variable getter is more reliable than lambda. Maybe we should introduce short getter syntax, see #99199.

@akien-mga akien-mga changed the title @export_tool_button breaks when changing the script contents @export_tool_button breaks when changing the script contents (lambda hot-reloading) Mar 5, 2025
@pranabekka
Copy link

The faster workaround for me is to start and quit the game, instead of reloading the project. Could we put this in the documentation? Also, this suggests that there's something in the code path for starting/stopping that reloads the tool script and is faster than reloading the whole project.

@rune-scape
Copy link
Contributor

i would like to mention that using the lambda-in-getter syntax:

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

this avoids the issue completely! even lambda hot-reloading ones. and its not too much more to type, even if it isn't the thing i'd immediately jump to

the intuitive one using a lambda as initializer:

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

has 2 issues:

  • the member is null after adding a new member or renaming the member then hot reloading
  • reordering 2 members initialized with unnamed lambdas can get them mixed up after hot reloading

and i have a PR that solves the latter issue, #98221

@ChrisJamesonGroves
Copy link

Workaround which might help some people:

I found the same issue when adding the method name to the export_tool_button like this:
@export_tool_button("Regenerate") var test = generate_map
This was working, after reloading the project. But then when I changed the method name it was calling 'generate_world' i found it was still calling the original method. I'm sure a reload would fix it.

My workaround, and what I now do whenever I use export_tool_button, is to keep the method name the same, then change what that method in turn calls. Something like this, which works every time:

@export_tool_button("Regenerate") var test = onGenerateButtonClicked

func onGenerateButtonClicked();
  callSomeOtherMethod() # I can then just update this each time I want to call different code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Bad
Status: In progress / Assigned
Development

Successfully merging a pull request may close this issue.

7 participants