C#: Generators ignore _Ready method in partial classes from another generators

Maclay74 commented 2 years ago

Godot version

4.0 beta 1

System information

Windows 11

Issue description

This one is kinda tricky. We have source generators, they analyze classes and list all the methods they find so if there is no _Ready or _Process methods, they don't call it.

The problem is, generators don't know about each other, so if there is another generator that populates _Ready method, Godot's generator won't see it and, therefore, it's never get called.

The simplest solution is "just don't generate _Ready method". But it adds extra restrictions for developers when it comes to initialization.

We don't have an access to constructor, so _Ready is the only place we can do initial logic and its generation is crucial for some popular plugins like https://github.com/31/GodotOnReady.

As an another example, generation of _Ready method would be a very tidy and clean way to implement Dependency Injection. In Godot 3.5 I had to hook IL call to _Ready method and patch it, but since Godot 4 use Collectible Assemblies, it's not possible anymore. I implemented it using source generation, but it doesn't work, because Godot doesn't call my generated _Ready method, as I described above.

I know that it's not possible for generator to see another one, but probably we could think of a solution for this cases. I can think of using reflections to check _Ready method presence and cache it somehow.

Thanks.

Steps to reproduce

Create a project
Generate _Ready method using C# source generators or add https://github.com/31/GodotOnReady

Minimal reproduction project

No response

raulsntos commented 2 years ago

Does it work if the method is declared as partial?

public partial class MyNode : Godot.Node
{
    public override partial void _Ready();
}

Maclay74 commented 2 years ago

That's actually a good idea. And yes, it works, I see that _Ready method appears in generated code and its implementation gets called.

Is it a workaround or we can require developer to add it somehow? Interface? Error from the generator?

raulsntos commented 2 years ago

When the game developer doesn't implement the engine callback methods (e.g.: _Ready, _Process, etc.) they are not called, this is by design to avoid the cost of interop with C# for a no-op.

To know which methods are implemented, Godot's source generators generate a method that is called once to retrieve the list of implemented methods that are callable/interoperable with the engine (engine callbacks, methods that only use supported marshable types).

Because of this, we don't want developers to implement a _Ready method unless they are really using it, an empty implementation would add some cost.

I think the best option would probably be to document this behavior and recommended source generator developers to use partial with these engine callback methods; then, if the method hasn't been declared, the source generator can report an error diagnostic.

Zireael07 commented 2 years ago

I think the best option would probably be to document this behavior

Definitely.

Maclay74 commented 2 years ago

Godot's source generators generate a method that is called once to retrieve the list of implemented methods

When I was trying to check whether my _Ready method is there, I saw it in generated code. A method called once would notice presence of _Ready method. Can you please elaborate on that?

raulsntos commented 2 years ago

I was referring to the generated GetGodotMethodList method but I was wrong, this is only used for the other engine callbacks (not _Ready). In the case of _Ready, the method that gets called is the generated InvokeGodotClassMethod method.

Either way, because these methods are generated and there is no guarantee on the order of execution of source generators, we can't rely on checking generated code to check if the _Ready method is implemented to add it to the generated InvokeGodotClassMethod method implementation. We can only check non-generated sources, so the only way to make sure _Ready is called in the InvokeGodotClassMethod would be to add it to your class (with the partial modifier if you want to implement it somewhere else, like using a source generator).

To explain further, the core implementation of Node checks if the engine callbacks are implemented and calls the _Ready method in here:

https://github.com/godotengine/godot/blob/67961d875d518b565bc1fa923772af56fb227063/scene/main/node.cpp#L130-L155

The implementation of GDVIRTUAL_IS_OVERRIDDEN ends up calling the GetGodotMethodList[^1] method to check if the method is implemented and the implementation of GDVIRTUAL_CALL ends up calling InvokeGodotClassMethod[^2] to call the method (returning false if the method was not found, which for us means it's not implemented by the script).

[^1]: GDVIRTUAL_IS_OVERRIDDEN checks if there is a script attached to the node and only then it calls the ScriptInstance's has_method method which in the case of the script implementation of C# (CSharpScript) it checks its methods field which has been filled previously in the update_script_class_info method which calls ScriptManagerBridge's UpdateScriptClassInfo which calls GetMethodListForType which calls the generated GetGodotMethodList method. [^2]: GDVIRTUAL_CALL checks if there is a script attached to the node and only then it calls the ScriptInstance's callp method which in the case of the script implementation of C# (CSharpScript) it calls CSharpInstanceBridge's Call method which calls the generated InvokeGodotClassMethod method.

Maclay74 commented 2 years ago

Thanks a lot for such a brilliant explanation!

jolexxa commented 1 year ago

Third Party Source Generation Proposal

I'd like to propose a potential solution. I believe this solution could solve the following problems:

Allows multiple third-party source generators to observe node lifecycles.
Requires no more than 1 line of easy-to-write boilerplate in each script file that uses a third-party generator.
Only generates lifecycle observation events when applied to a specific node script type, rather than affecting the performance of the whole project.
Provides an implementation that's flexible enough to expose other features to third party source generators as they become available.

While this may not be the best solution, I believe any adopted solution should solve all of the problems above. Hopefully this proposal will help spur future ideas, if nothing else.

Explanation

We know that a node can observe all lifecycle events by implementing void _Notification(int what).

To make a third-party generator that observes lifecycle events, generator authors can generate a partial implementation for a node script class that contains a method that should be called from _Notification (but won't actually be named _Notification itself).

// Potential output from a third-party generator...

public partial class SomeNode : Node {
  // generated method handler that should be called back.
  public void SomeGeneratedNotificationHandler(int what) {
    // this will get called on _Notification lifecycle events
    // make your generator do whatever it needs to in response
    // to lifecycle changes
  }
}

An official Godot source generator can inject an attribute named [Lifecycle] into each project (or expose one through the Godot SDK). This attribute can be applied to node script classes that want to use a third-party generator that responds to lifecycle events.

The [Lifecycle] attribute will receive a const array of strings via a params property that represent the names of method callbacks which should be invoked when a node receives a lifecycle event (traditionally via _Notification).

Imagine in the future that @31 has ported their GodotOnReady generator to 4.x using this technique and that you would like to use it in one of your game's scripts.

// Your node script class that uses a third-party generator

[Lifecycle("GodotOnReady")]
public partial class MyNode: Node {
  // ... your node's code. no other boilerplate except the attribute above.
}

The [Lifecycle("GodotOnReady")] attribute applied to the script class would be sufficient for the official Godot Source generators to know that they must generate a method handler for _Notification. The generated method handler would call each method that was listed in the script's [Lifecycle] attribute (as well as the node script's _Notification(int what) method, if it was defined).

Of course, the hypothetical updated GodotOnReady generator would itself have to generate a method named GodotOnReady that should be called from the official Godot source generator's notification handler.

This approach allows multiple source generators to be leveraged that each respond to lifecycle events in a node.

// Your node script class that uses third-party generators

[Lifecycle("GodotOnReady", "SomeOtherGenerator")]
public partial class MyNode: Node {
  // ... your node's code. no other boilerplate except the attribute above.
}

The script above would benefit from two generators that each add methods that respond to the node's lifecycle. All of the third party generator methods would be called in the order they were listed in the [Lifecycle] attribute above the main node script. Lastly, the script's own _Notification method, if defined, would be called.

By convention, developers would be incentivized to name their lifecycle observation method the same name as their source generator. We can document this convention by adding a page to the C# section of the Godot docs that describe useful tips for building third-party source generators.

Naturally, this proposal will require changes to be made to the Godot Source generators. I don't think the changes are too terribly difficult, however.

Feel free to rip this apart. If you have any questions or I didn't do a good job explaining, let me know and I'll try to clarify.

Credits

@31 for patiently explaining the current limits of third party source generators to me.
@raulsntos for patiently explaining why my other ideas wouldn't work at all.
The https://github.com/LokiMidgard/PartialMixins library for providing some inspiration.

--

EDIT: I actually ended up creating a source generator that does most of what I described above, plus additional features that essentially allow you to create node mixins. You can find it here: https://github.com/chickensoft-games/SuperNodes

hawkerm commented 1 year ago

Worth keeping an eye on the base .NET issue for dependent source generator support: https://github.com/dotnet/roslyn/issues/57239

Note, I would assume any solutions here will be related to IIncrementalGenerator and not ISourceGenerator. Also see https://github.com/godotengine/godot-proposals/discussions/7106

thetawave commented 3 months ago

I'm more interested in seeing Godot's method dispatcher code changed than leaning into chained source generators. Maybe there's a more intelligent way to conditionally dispatch method calls from C++ instead of the InvokeGodotClassMethod in godot .NET. If the Ready method were defined in a .NET Interface then we'd be able to more naturally delegate Ready dependencies to User and/or Generated code in .NET.

godotengine / godot