Make the C# tool script reloading process easier to work with

[ershn]

Describe the project you are working on

A colony survival/resource management game in C# with a lot of custom tooling

Describe the problem or limitation you are having in your project

The way data is reloaded into C# tool scripts after an assembly reload (e.g. after a recompilation) currently causes a lot of small annoyances that need to be worked around. Also, the specifics of data reloading are not documented which makes the behavior hard to predict unless you read the Godot source code.

I'll describe the problems I've experienced one by one.

1. The parameter-less constructor of tool scripts is called after every compilation

Every time you recompile your project, already instantiated tool scripts' parameter-less constructor is called. This is ok if your script doesn't do anything in its constructor or just sets initial data that doesn't rely on anything. However, if your constructor has side-effects, e.g. like adding a child node, this becomes a problem.

[Tool]
public partial class MyToolScript : Node
{
    public MyToolScript()
    {
        // This will be executed after every recompilation, probably not what you want/expect
        AddChild(new Node());
    }
}

The solution here is to never have side-effects in your parameter-less constructor. However, it can be limiting and it's easy to forget.

2. Tool scripts that don't have a parameter-less constructor can't be reloaded

If your tool script doesn't have a parameter-less constructor, e.g. you added a constructor that takes parameters to the class, Godot won't be able to reload it after a recompilation (currently causing an editor crash, see https://github.com/godotengine/godot/issues/87147).

[Tool]
public partial class MyToolScript : Node
{
    // If this is your only constructor, Godot won't be able to reload your script after a recompilation
    public MyToolScript(string foo) { }
}

The solution here is to add a (possibly private) parameter-less constructor that does nothing. However, if you use nullable contexts this can lead to warnings that need to be dealt with.

3. Property setters are called on every recompilation

Every time you recompile, your property setters are called. It doesn't matter if the property is exported or not. This is ok if your setter is auto-implemented or very simple. However, if it has side effects you will run into the same problems as for parameter-less constructors.

public bool Enabled
{
    get => _enabled;
    set
    {
        // This will be printed after every recompilation
        GD.Print("Enabled");
        _enabled = value;
    }
}

The solution here is to convert the setter to an individual method and make the property readonly (no setter). This works but goes against C#'s design.

4. Fields are reloaded after property setters are called

The data held in fields is restored after property setters are called. If your property relies on fields to be set beforehand, you will most likely end up with an exception. This situation happens very often.

[Export]
Label _label;

public string Message
{
    get => _label.Text;
    // This will cause an exception on every recompilation
    set => _label.Text = value;
}

The solution here is to do one of the following:

• Check if the fields are set in the property setter. However this can be cumbersome.
• Convert the setter to an individual method and make the property readonly (no setter). This works but goes against C#'s design.

On a lesser note, since properties are often backed by user defined fields, it's common for the same field to be set 2 times on each reload. Maybe this doesn't cause any visible problems, but it doesn't feel clean.

5. ISerializationListener.OnAfterDeserialize() is called before all tool scripts are reloaded

You can implement the ISerializationListener interface and use OnAfterDeserialize() to execute code only when your script is reloaded after a recompilation. However, this method isn't called on each script after all scripts have been reloaded. It is called on each script when the script is reloaded. This means that when OnAfterDeserialize() is called, other scripts might not have been reloaded yet, which means you cannot expect them to be ready to use. The order in which scripts are reloaded also isn't fixed.

[Export]
MyOtherToolScript _otherScript;

public void OnAfterDeserialize()
{
    // If MyMethod() expects _otherScript to be completely reloaded,
    // the call below will work some times and fail other times
    _otherScript.MyMethod();
}

The solution here is to use CallDeferred to always execute your code after all scripts have been reloaded.

6. All the above problems are non-obvious/not documented

There is currently no documentation describing the behavior/limitations that cause the problems above. It's also very hard to understand all of this experimentally. If you want to create tool scripts that don't break on recompilation for seemingly unknown reasons, your only option is to read the Godot source code, make a memo, and refer to it every time an error occurs when you recompile your project (that's what I ended up doing).

I'll leave my memo below in case you are interested.

Assembly reloading flow for C# tool scripts

Assembly reloading flow for C# tool scripts

• for each script, serialize its data

  • ISerializationListener.OnBeforeSerialize() is called
  • properties are get
  • fields are read
  • signal delegates are read

• assemblies are reloaded

• for each script, create a class instance

  • the (non-)public parameter-less constructor is called

• for each script, deserialize its data

  • properties are set
  • fields are set
  • signal delegates are set
  • ISerializationListener.OnAfterDeserialize() is called

Notes

• scripts are updated in inheritance dependency order
  • when there is no inheritance relationship, scripts are updated in random order
• only properties and fields with godot compatible types are saved/restored
• only non-indexer get&set properties are saved/restored
  • properties without a getter are ignored
  • properties without a setter are ignored
  • properties with an init-only setter are ignored
  • properties that implement interfaces explicitly are ignored
  • indexers are ignored
  • static properties are ignored
• only non-readonly fields are saved/restored
  • readonly fields are ignored
  • implicitly declared fields are ignored
  • static fields are ignored
• only signal delegates with compatible signatures are saved/restored
  • methods with a parameter/return type that is not godot compatible are ignored
  • methods with pass by reference parameters are ignored
  • generic methods are ignored

Godot compatible C# types

• primitive types
• string
• enums
• variant types
• C# arrays of compatible types and of one dimension
• Godot.Collections.{Array,Dictionary}
• C# classes that derive (in)directly from GodotObject

Describe the feature / enhancement and how it helps to overcome the problem or limitation

I think the root problem here is that the current reloading flow can result in too many unwanted side-effects. Also, the order in which some operations happen puts too many constraints on the user.

IMO the solution here is to reduce the amount of side-effects that can occur after a reload to a minimum. Also, the order of some operations should be changed to allow for more flexibility on the user's side.

To be more specific:

1. Only save and restore fields, ignore properties altogether

Don't save and restore properties. Only save and restore fields, including the backing fields of auto-implemented properties. You can access the backing field of a property through reflection. Since all instance data is stored in fields, this won't have an effect on what is saved or not.

Only saving/restoring fields will remove all side-effects related to properties, since AFAIK you cannot execute code when a field is get/set. This allows you to do whatever you want with properties.

If you want to execute code after a reload, you can still use ISerializationListener.

2. Auto-generate a constructor that will only be called on script reloading

Don't use the parameter-less constructor of scripts to recreate class instances after a reload. Instead, use source generators to create a constructor that will only be called after a reload (make it receive the serialized data as a parameter).

In scenarios where this would result in the parameter-less constructor to not be generated by the compiler anymore (e.g. the user didn't define a constructor), also generate a parameter-less constructor that does nothing.

Doing this will remove side-effects related to parameter-less constructors, since after this change no user defined code will unexpectedly get called on instance creation. Also, this allows scripts that don't have a parameter-less constructor to be reloaded after a recompilation, removing a limitation.

In the future, this will also allow restoring readonly fields after a reload without using reflection.

If you want to execute code after a reload, you can still use ISerializationListener.

3. Call ISerializationListener.OnAfterDeserialize() only after all scripts have been reloaded

Only call ISerializationListener.OnAfterDeserialize() on individual scripts after all scripts have been reloaded. This way, you won't ever have to use CallDeferred in OnAfterDeserialize() anymore.

Implementation-wise this is just a matter of breaking one loop into two. Should be pretty simple.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

After the changes, the reloading process will be as follows:

• for each script, serialize its data

  • ISerializationListener.OnBeforeSerialize() is called
  • signal delegates are read (doing this before field reading to be the exact opposite of the reload behavior)
  • fields are read (including auto-implemented properties' backing fields)

• assemblies are reloaded

• for each script, create a class instance

  • the deserialization-only autogenerated constructor is called
    • fields are set (including auto-implemented properties' backing fields)
    • signal delegates are set (not sure if this should be done here or in a separate loop)

• for each script

  • ISerializationListener.OnAfterDeserialize() is called

Implementing these changes will also reduce the cognitive load script reloading has on users, since there is less stuff to remember and less exceptions to take care of.

If this enhancement will not be used often, can it be worked around with a few lines of script?

No. You can't change the current behavior without changing the Godot's source code.

Is there a reason why this should be core and not an add-on in the asset library?

The relevant processing is core.

[ttencate]

Additionally, it seems that some properties and fields are silently skipped. In particular, classes not deriving from GodotObject, as well as user-defined structs, are not (de)serialized at all. This probably has good technical reasons, but should be documented somewhere.

The full logic is in https://github.com/godotengine/godot/blob/master/modules/mono/editor/Godot.NET.Sdk/Godot.SourceGenerators/MarshalUtils.cs.

[ZeromaXHe]

This issue is very useful. Thank you for your effort! Without your conclusions in this issue, I can't really use [Tool] in C# safely. The same logic is simple in GDScript @tool.

I can't understand, after nearly one year, why is this issue not answered? And maybe changing code is hard or there are some reasons we don't know, but why is there still no document about this mechanism? That makes the [Tool] hardly usable if involving setter, especially comparing to GDScript. No document causes lots of unpredictable bugs.

I think there should be some sum-up documents about this in the Godot Docs, MANUAL - Scripting - C#/.NET or MANUAL - Plugins - Running code in the editor. Or maybe these implemention details are not suitable for docs, then at least C# [Tool] should be mentioned to be carefully treated and recommend users to read source code and understand its difference from GDScript @tool. (Rather than just doing nothing about it)

[Lennart-Bours]

edit: this seems to be fixed in 4.4, see the comment below. That's great!

There is no direct way of calling a function of a [Tool] script in the editor in Godot. The community has solved this problem by using the setter of an exported bool to call the desired function, as an imitation of a button. However, since the setters of C# [Tool] scripts are called on recompilation, you cannot have code execute only on demand. Thus you are very limited in what code you could possibly execute with these 'buttons'.

This, together with the errors around in-editor signals, is making it very hard to set-up controlled in-editor functionality in C#. And to add insult to injury you cannot run editor script when using an external editor.

[ttencate]

@Lennart-Bours In 4.4, that particular use case is covered by [ExportToolButton].

[mashimaro9659]

Thank you for the article. This issue should have been documented, or at least a warning should have been included in the documentation. I discovered other behaviors through trial and error, but for a year since I started making games with Godot, I was unaware of ISerializationListener, which is undoubtedly extremely useful for many cases involving editor scripts. It's unfortunate that I could have saved a tremendous amount of time if I had found this article sooner.

[Lennart-Bours]

@mashimaro9659 Note that in the documentation issue on ISerializationListener, I got the following response:

The ISerializationListener interface is intentionally undocumented because we don't want to encourage its usage. It likely should have been internal from the beginning.

In the engine code we use it to maintain state across assembly reloads. The process of reloading the .NET assemblies is indeed quite complicated and ideally not something users should need to worry about.

We'll likely change the reloading process in the future and we don't want to limit ourselves to the current ISerializationListener design, we may want to change it.

see here: https://github.com/godotengine/godot-docs/issues/10615#issuecomment-2635063398

[JustASquid]

Issue 4 - Fields are reloaded after property setters are called - is a real killer for me.

It basically means that we can't Export properties that have side-effects, even though this is a very common pattern in Godot tool scripting.

e.g. this pattern referenced in the issue is very commonly used and I think should be properly supported.

Label _label;

[Export]
public string Message
{
    get => _label.Text;
    // This will cause an exception on every recompilation
    set => _label.Text = value;
}

The suggested workaround falls apart pretty quickly too. Quite often I want to use a pattern like

private string _foo;

[Export]
public string Message
{
    get => _foo;
    // This may or may not cause an exception depending on what `Init()` does
    set => {
        _foo = value;
        Init();
    }
}

Now the body of Init() needs to check if the object is in a valid state or not - really nasty.

I think at the very least solution 1 (Don't reload properties, only fields) should be implemented. I would be happy to have a crack at it, but I'd want the OK from the team that it's worth spending time on or if it's not due to the upcoming changes to C# integration - any thoughts @raulsntos ?