Implement [ExportToolButton]

[paulloz]

Follow up to #96290, implementing the C# attribute.

---

✋ I leave this in draft for the following reasons:

• I need to add tests in Godot.SourceGenerators.Tests.
• Texts in the new diagnostics are placeholders. I'd appreciate if someone could take a stab at writing proper error messages, I'm quite bad at it myself.

---

I more or less reflected the GDScript annotation, with a new attribute [ExportToolButton]. It can be used on both fields, and properties of type Callable. I lifted the restriction on read-only exported properties for this specific usage. I don't think we need to do anything in ScriptPropertyDefValGenerator, unless I miss something major.

The DeterminePropertyInfo() method is becoming a bit convoluted. I'm pretty sure we could simplify it, but didn't want to refactor more than I needed. I could comment it more if it helps to navigate it, please tell me. Also, am I blind, or are the comments lines 452 and 463 wrong?

I introduced the following diagnostics:

• GD0108, reported when [ExportToolButton] marks a member defined in a class not marked with [Tool].
• GD0109, reported when [ExportToolButton] and [Export] mark the same member.
• GD0110, reported when [ExportToolButton] marks a member that is not of type Callable.

The easiest way to use this is something like the following.

[Tool]
public partial class MyNode : Node
{
    [ExportToolButton("Click me!")] // You can pass an icon as second argument if you want.
    public Callable ClickMeButton => Callable.From(ClickMe);

    public void ClickMe()
    {
        GD.Print("Hooray!");
    }
}

It is also possible to use a backing field, but it becomes more verbose, since in C# we can't assign Callable.From(...) during fields initialization.

[Tool]
public partial class MyNode : Node
{
    [ExportToolButton("Click me!")] // You can pass an icon as second argument if you want.
    private Callable _clickMeButton;
    
    public MyNode()
    {
        _clickMeButton = Callable.From(ClickMe);
    }

    public void ClickMe()
    {
        GD.Print("Hooray!");
    }
}

Cheers ✨

[paulloz]

I'm putting this in "ready for review". The last thing to do is write proper diagnostic messages, for which I'd still require some help.

[Delsin-Yu]

It would be better if we can apply this attribute directly to a method.
I see, thanks

[paulloz]

It would be better if we can apply this attribute directly to a method.

The reasons it is not the case have been discussed in the original proposal, if you're interested.

[a-johnston]

I tweaked the messages and also refactored a bit to error when using a non-marshallable type (currently if you put the attribute on for example System.Action it won't error but it also won't work in the editor, which might be confusing) here https://github.com/a-johnston/godot/tree/dotnet/export-tool-button

The comments you called out are wrong and I think the handling was sort of copied from ScriptPropertyDefValGenerator.cs, or left over from a refactor. Those diagnostic errors are currently being raised twice between the two generators, so I changed ScriptPropertiesGenerator.cs to just filter with a comment clarifying the other class is responsible for raising the error.

The branch is rebased so I could use this with some other recent merges but if you'd prefer I can open a pr against your branch rather than cherry picking the latest commit off mine to yours. Also imo the error messages are fine. I tweaked them because you called out maybe changing them, but I think they are clear enough as is.

[paulloz]

currently if you put the attribute on for example System.Action it won't error but it also won't work in the editor, which might be confusing

That's a good catch. I'd want to take a second look to maybe find a less invasive way to handle this, tho.

The comments you called out are wrong and I think the handling was sort of copied from ScriptPropertyDefValGenerator.cs, or left over from a refactor. Those diagnostic errors are currently being raised twice between the two generators, so I changed ScriptPropertiesGenerator.cs to just filter with a comment clarifying the other class is responsible for raising the error.

I don't think this should be modified in this PR. It isn't really related to the new button thing, and faxing unrelated changes make things hard to track IMHO. It's also probably going to clash against #98396.

Also imo the error messages are fine. I tweaked them because you called out maybe changing them, but I think they are clear enough as is.

TBH, that was more of a call for @raulsntos, to make sure all of our diagnostic messages are more or less worded in the same way.

[a-johnston]

That's a good catch. I'd want to take a second look to maybe find a less invasive way to handle this, tho.

That's fair; I felt like the structure of ScriptPropertiesGenerator was not conducive to adding this check and I wasn't super happy with the impl, although this seemed preferable to tacking on a helper method with duplicated code or adding tool button validation to an otherwise unrelated generator. I do have thoughts on how to better organize this code but it probably isn't worth overhauling.

I don't think this should be modified in this PR. It isn't really related to the new button thing, and faxing unrelated changes make things hard to track IMHO. It's also probably going to clash against #98396.

I'll make a separate issue/pr then for it. It looks like it will conflict with that pr but not in a substantial way.

ed: #99205 + #99206

[raulsntos]

Thanks for implementing the C# follow-up so quickly, everything looks great already.

The DeterminePropertyInfo() method is becoming a bit convoluted. I'm pretty sure we could simplify it, but didn't want to refactor more than I needed.

Not sure it's worth it since we'll replace these generators in the future, but feel free to do this in a follow-up PR if you want.

Also, am I blind, or are the comments lines 452 and 463 wrong?

You are right, I think originally these members were supposed to be filtered by WhereIsGodotCompatibleType. This method used to filter read-only and write-only members, but these checks where removed in #67304, probably also why we have duplicate diagnostic reports now (but it seems @a-johnston has already fixed this in #99206).

[paulloz]

Thanks for the review. Everything you noted should be addressed 👍

[raulsntos]

LGTM. Thanks!

[paulloz]

Squashed and ready to go 🚀

[a-johnston]

Not sure it's worth it since we'll replace these generators in the future, but feel free to do this in a follow-up PR if you want.

Are there plans/goals for overhauling the generators or is this a hypothetical long-term thing?

[paulloz]

Not sure it's worth it since we'll replace these generators in the future, but feel free to do this in a follow-up PR if you want.

Are there plans/goals for overhauling the generators or is this a hypothetical long-term thing?

Generators are very different in the GDExtension bindings.

[a-johnston]

Generators are very different in the GDExtension bindings.

In reference to https://godotengine.org/article/whats-new-in-csharp-for-godot-4-0/#gdextension-support (unsure if there are more recent links)? I think that would be welcome but if c# still stuck around as a scripting language in addition to a gdextension language, wouldn't these generators also continue being relevant, or do you anticipate them sharing code?

[Repiteo]

Thanks!

[Delsin-Yu]

In reference to https://godotengine.org/article/whats-new-in-csharp-for-godot-4-0/#gdextension-support (unsure if there are more recent links)? I think that would be welcome but if c# still stuck around as a scripting language in addition to a gdextension language, wouldn't these generators also continue being relevant, or do you anticipate them sharing code?

If you are interested in the development progress of the GDExtension-based integration, you may check this repo; please note that this project is not yet ready for formal projects.

[molingyu]

hi, I read the discussion about why we can't export methods directly.
But I don't think this is an absolute reason not to do so. In fact, we can completely hide the process of converting C# methods to godot callables with source generators.

When the method marked by ExportToolButton is checked, a xxx_exportToolButtonField of type Callable is generated. This can completely avoid manual explicit callable conversion. This also maintains the consistency of gds and c# APIs and reduces the mental burden of understanding functions.