31 / godotonready Goto Github PK

I don't know yet if GenerateDataSelectorEnum will actually be useful, but if it is, should consider adding a way to configure the underlying type.

Right now [OnReadyPath] generates GetNode, [OnReadyLoad] generates GD.Load.

Instead, there could be just one attribute that calls the right function depending on the type of the annotated field/property. If a subclass of Node, GetNode, if a subclass of Resource, GD.Load.

[OnReadyGet]?

This makes is more obvious how to combine these code paths, and could make it easier to implement some shared features like Private.

This could also combine with [OnReady], however none of the properties of [OnReadyPath] apply to methods so this makes less sense.

_Ready should call the base _Ready, so base class [OnReadyGet] props are set up and [OnReady] methods called.

(I worked on porting a Unity game to Godot using GodotOnReady to try to explore whether GodotOnReady is useful, and naturally that doesn't use any node inheritance, so I didn't notice this yet. 😛)

I believe Godot 4.0 now lets you [Export] public Node n;. Providing [OnReadyGet] public Node n; is the main reason I wrote this library, so I'm thinking it might now be obsolete.

https://godotengine.org/article/dev-snapshot-godot-4-0-alpha-11
Exporting Node pointers as NodePaths (@export var some_node: Node)

I've still got to get my hands on the 4.0 beta to confirm that works how I expect, and see if there's other feature gaps I'm forgetting that I want to use GodotOnReady to plug. GodotOnReady has evolved to support more cases (like defining a default NodePath in the attribute that can be overridden in the editor) but personally I'm not sure if those would justify using the library in my own projects.

Now that Godot has built-in source generators and analyzers, ideally any new features can be added to Godot directly through a proposal rather than a library like GodotOnReady.

Opening this issue for comments in case anyone else has some thoughts. 🙂

I haven't found a way to get references working yet, so I've just copied the attributes file over to the code generator csproj. Here's the extension method I use to convert Rosyln source code attributes to instances of regular C# attributes.

        public static T MapToType<T>(this AttributeData attributeData) where T : Attribute
        {
            T attribute;
            if (attributeData.AttributeConstructor != null && attributeData.ConstructorArguments.Length > 0)
            {
                attribute = (T)Activator.CreateInstance(typeof(T), attributeData.GetActualConstuctorParams().ToArray());
            }
            else
            {
                attribute = (T)Activator.CreateInstance(typeof(T));
            }
            foreach (var p in attributeData.NamedArguments)
            {
                var field = typeof(T).GetField(p.Key);
                if (field != null)
                    field.SetValue(attribute, p.Value.Value);
                else
                    typeof(T).GetProperty(p.Key).SetValue(attribute, p.Value.Value);
            }
            return attribute;
        }

I'm writing this issue so others can find the solution easier. Potentially a similar text could be included in the README.md

When making builds inside docker images they sometimes don't include .NET by default (e.g. abarichello/godot-ci#80), but Godot still resolves NuGet packages. This causes the build pipeline to succeed, as all the code is valid, but the actual build will exhibit strange behavior as methods are not called and nodes are not fetched.

The solution, if using a docker image based on Debian 10, is to add

wget https://packages.microsoft.com/config/debian/10/packages-microsoft-prod.deb -O packages-microsoft-prod.deb
dpkg -i packages-microsoft-prod.deb
rm packages-microsoft-prod.deb
apt-get update -qq
apt-get install -y apt-transport-https
apt-get update -qq
apt-get install -y dotnet-sdk-6.0

to your build pipeline before running the build command.

The generator currently makes something like this:

[OnReadyGet("Face")] private Spatial _face;
...
if (FacePath != null)
{
	_face = GetNode<global::Godot.Spatial>(FacePath);
}
if (_face == null)
{
	throw new NullReferenceException($"'{((Resource)GetScript()).ResourcePath}' member '_face' is unexpectedly null in '{GetPath()}', '{this}'. Ensure 'FacePath' is set correctly, or set [OnReadyGet(OrNull = true)] to allow null.");
}

The GetNode<global::Godot.Spatial>(FacePath) can throw, and it doesn't include much info. I think it would be useful to use GetNodeOrNull(FacePath) and do the checks on our side, so we can throw a detailed exception if it doesn't work.

A specific situation I want to improve is when the target node is the wrong type. It can be hard to think of this as the root cause, sometimes, and more details would help.

(This is what I was attempting to do with the generated throw, but it only helps in very specific situations.)

Thanks

Currently, for an OnReadyGet like this:

[OnReadyGet(Export = true)]
private Texture MyTexture { get; set; } = null!;

the generated code looks like that:

[Export] public global::Godot.Texture MyTextureResource
{
    get => MyTexture;
    set { _hasBeenSetMyTexture = true; MyTexture = value; }
}
private bool _hasBeenSetMyTexture;

The code responsible for that is this here:

But _hasBeenSetMyTexture is never used. (I noticed this because I got several CS0414 warnings when compiling.)
It would only be used if the generator had to generate the assignment by itself as in here:

I couldn't find any documentation regarding the possible usage of _hasBeenSet outside the generated code (i.e. by the user) and cannot imagine a sensible use case.
Therefore I suggest to only generate _hasBeenSet if IsGeneratingAssignment is set, so that it is only generated if it is actually used.

On 2021/01/14, the cookbook got some instructions on how to unit test:
https://github.com/dotnet/roslyn/blob/main/docs/features/source-generators.cookbook.md#unit-testing-of-generators
dotnet/roslyn#44075

That would definitely be better than what I do now. (Open a project and build, then navigate to generated source to see if it looks right.)

This isn't really OnReady functionality... but I've seen this asked about twice before and it ties into the Godot limitation where the only way you can export a set of options to show up in the editor is via enum (or I suppose an editor plugin).

From the data definitions:

[GenerateDataSelectorEnum("Tag")]
public partial class TagData
{
    private static readonly TagData A = new() { Name = "Table" };
    private static readonly TagData B = new() { Name = "Chair" };
    private static readonly TagData C = new() { Name = "Rug" };

    public string Name { get; set; }
}

Generate a lookup and enum:

public enum Tag { A, B, C }

public class TagData
{
    public static TagData Get(Tag tag)
    {
        switch (tag)
        {
            case Tag.A: return A;
            case Tag.B: return B;
            case Tag.C: return C;
        }
        return null;
    }
}

This way you can [Export] Tag Foo to give the scene editor a choice between several options, with an easy (and efficient) way to attach more data to each of those options.

When using C# 8 Non-nullable types with GodotOnReady, Godot shows warnings similar to

<project folder>\GodotOnReady.Generator\GodotOnReady.Generator.GodotOnReadySourceGenerator\Partial__IdleState.cs(7,27): warning CS8618: Non-nullable property 'PlayerDetectionAreaPath' must contain a non-null value when exiting constructor. Consider declaring the property as nullable. [<project folder>\ActionRPG.csproj]

This is a warning, so it doesn't beak any code, although it is still somewhat off-putting, specially as the script count increases.

Normally, one would write null! in the property declaration to give it a null value until _Ready() is called.

[OnReadyGet] private DetectionArea playerDetectionArea = null!;

This way, declaring the property as nullable is not necessary, and as that's the only time it will be null, it is perfectly safe.

A solution could be adding that to the PlayerDetectionAreaPath property if C# 8 is being used and non-nullable types are enabled.
Making the property nullable would also work; as that property is only intented to be used by the generated source code (afaik), it wouldn't result in developers having to write pointless null checks.

The following code will not compile using GodotOnReady. The generated partial class is going to be public, raising CS0262.

internal partial class Foo : Node
{
    [OnReadyGet("Foo")]
    private Node foo;
}

[OnReadyLoad("res://somewhere/i/have/a/scene.tscn")] private PackedScene Scene { get; set; }

GDScript lets you do this:

export var scene : PackedScene = preload("res://ExportPackedScene/ExternalScene.tscn")

which shows up like this:

The default shows up nicely when you freshly attach it to a new node, and you can set it to something else.

This C# code does the same thing as far as I can tell:

[Export] public PackedScene Scene
{
    get => _scene;
    set
    {
        _scene = value;
        _sceneSet = true;
    }
}

private const string _sceneDefault = "res://ExportPackedScene/ExternalScene.tscn";
private PackedScene _scene;
private bool _sceneSet;

public ExportPackedScene()
{
    // Set Scene in the constructor when in the editor so when Godot instantiates the node, it can
    // tell what the default should be.
    if (Engine.EditorHint)
    {
        Scene = GD.Load<PackedScene>(_sceneDefault);
    }
    GD.Print($"End of constructor: {Scene}");
}

public override void _Ready()
{
    // At runtime, only load the scene once we're in _Ready. Godot will construct this object and
    // populate the property with the custom value if non-default. Note that we can't just check
    // if Scene is null, because it may have intentionally be set to null in the editor.
    if (!Engine.EditorHint && !_sceneSet)
    {
        Scene = GD.Load<PackedScene>(_sceneDefault);
    }
    GD.Print($"End of ready: {Scene}");
}

Godot shows a generic exception when the node path isn't assigned:

Unhandled Exception:
System.NullReferenceException: The instance of type NodePath is null.
  at Godot.NodePath.GetPtr (Godot.NodePath instance) [0x00009] in /root/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/NodePath.cs:15
  at Godot.Node.GetNode (Godot.NodePath path) [0x00001] in /root/godot/modules/mono/glue/GodotSharp/GodotSharp/Generated/GodotObjects/Node.cs:617
  at Godot.Node.GetNode[T] (Godot.NodePath path) [0x00001] in /root/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Extensions/NodeExtensions.cs:7
  at MyThing._Ready () [0x00013] in D:\proj\MyThing\src\GodotOnReady.Generator\GodotOnReady.Generator.GodotOnReadySourceGenerator\Partial__MyThing.cs:20

The ready method should detect null and throw an exception that includes script name, node name/path, and field/prop name.

GetNode and GetNode<T> return null when the node path points to nothing. This should only be allowed when OrNull=true.

It would be useful if we could use OnReadyGet to initialize an interface field with a reference node. Game engines like Unity include this feature in their GetComponent<T>() function.

Using .NET Standard 2.0 or 2.1 will lead to an exporting error.
But .Net framework 4.7.2 or 4.8 work just fine.
godot mono 3.5 beta4

If the attributes are used in generic classes, Godot throws the following error:

<project folder>\GodotOnReady.Generator\GodotOnReady.Generator.GodotOnReadySourceGenerator\Partial__StateMachine.cs(9,23): 'StateMachine._Ready()': no suitable method found to override

StateMachine.cs

using Godot;
using GodotOnReady.Attributes;
using System.Collections.Generic;

public class StateMachine<T> : Node where T : Node
{
    [OnReadyGet] private State<T> currentState = null!;

    public void SetState(State<T> newState)
    {
        currentState.Exit();
        newState.Enter();
        currentState = newState;
    }

    public bool SetStateIf(Dictionary<State<T>, bool> transitionConditions)
    {
        foreach (var kv in transitionConditions)
        {
            if (kv.Value)
            {
                SetState(kv.Key);
                return true;
            }
        }

        return false;
    }

    public override void _Process(float delta)
    {
        base._Process(delta);

        currentState.StateProcess(delta);
    }

    public override void _PhysicsProcess(float delta)
    {
        base._PhysicsProcess(delta);

        currentState.StatePhysicsProcess(delta);
    }
}

Generated output (Partial__StateMachine.cs)

using Godot;
using System;

public partial class StateMachine
{

	[Export] public NodePath CurrentStatePath { get; set; }

	public override void _Ready()
	{
		base._Ready();

		if (CurrentStatePath != null)
		{
			currentState = GetNode<global::State<T>>(CurrentStatePath);
		}
		if (currentState == null)
		{
			throw new NullReferenceException($"'{((Resource)GetScript()).ResourcePath}' member 'currentState' is unexpectedly null in '{GetPath()}', '{this}'. Ensure 'CurrentStatePath' is set correctly, or set [OnReadyGet(OrNull = true)] to allow null.");
		}
	}
}

/*
---- END OF GENERATED SOURCE TEXT ----
Roslyn doesn't clear the file when writing debug output for
EmitCompilerGeneratedFiles, so I'm writing this message to
make it more obvious what's going on when that happens.
*/

Would be nice if we could also do something like (I'm including the optional parameters for reference):

[OnReadyFind("SomeNodeName"), Recursive = true, Owned = true] HBoxContainer _myContainer;

Which will essentially generate:

HBoxContainer _myContainer;
_myContainer = (HBoxContainer)FindNode("SomeNodeName", true, true)

Would be useful for example for AnimationTree, something like this:

[OnReadyGet("AnimationTree", Property = "parameters/playback")]
private AnimationNodeStateMachinePlayback _playback;

Please tell me what you think, if you wan't I can also have a go at implementing this and submitting a PR.

Hey 👋, thank you for this awesome plugin.

One of the nice feature of using [OnReadyGet] is that it allows dependency inversion. Making the code a lot more cleaner and way more maintainable.

Right now I have the following issue: If a scene was not created yet, it cannot have NodePaths applied to its parents objects, as it was not added as a child yet. This makes it awkward to inject dependencies into scenes, because no node paths can be set. To solve this, I am using FindParent("...") in the child scenes, so that it can then use the parents public interface to access the required dependencies. Example:

    public override void _Ready()
    {
        Arena arena = (Arena)FindParent("Arena");
        eventHub = arena.EventHub;
        playersInfo = arena.PlayersInfo;
    }

It would be great if there was something like OnReadyFindParent("...") that could automatically inject the required dependencies on startup. Example:

//Good
[OnReadyFindParent("Arena")]
private Arena arena;

//Better
[OnReadyFindParent("Arena", Property = "EventHub")]
private EventHub eventHub;

//Best
[OnReadyFindParent("Arena", Property = typeof(EventHub))]
private EventHub eventHub;

This feature would be greatly appriciated!

When splitting partial classes into multiple files generated code get's duplicated:

Example_1.cs

using Godot;
using GodotOnReady.Attributes;

public partial class Example : Node
{
    [OnReadyGet]
    public Node example;
}

Example_2.cs

using Godot;

public partial class Example : Node
{
}

Results in the following generated code which then fails to compile:

using Godot;
using System;

public partial class Example
{

	[Export] public NodePath ExamplePath { get; set; }

	[Export] public NodePath ExamplePath { get; set; }

	public override void _Ready()
	{
		base._Ready();

		if (ExamplePath != null)
		{
			example = GetNode<global::Godot.Node>(ExamplePath);
		}
		if (example == null)
		{
			throw new NullReferenceException($"'{((Resource)GetScript()).ResourcePath}' member 'example' is unexpectedly null in '{GetPath()}', '{this}'. Ensure 'ExamplePath' is set correctly, or set [OnReadyGet(OrNull = true)] to allow null.");
		}

		if (ExamplePath != null)
		{
			example = GetNode<global::Godot.Node>(ExamplePath);
		}
		if (example == null)
		{
			throw new NullReferenceException($"'{((Resource)GetScript()).ResourcePath}' member 'example' is unexpectedly null in '{GetPath()}', '{this}'. Ensure 'ExamplePath' is set correctly, or set [OnReadyGet(OrNull = true)] to allow null.");
		}
	}
}

/*
---- END OF GENERATED SOURCE TEXT ----
Roslyn doesn't clear the file when writing debug output for
EmitCompilerGeneratedFiles, so I'm writing this message to
make it more obvious what's going on when that happens.
*/

Hello, I encountered the following problems when using the godotonready you wrote. A member with the same parameter type named "_Ready" has been defined. This problem is related to the troubleshooting you listed, but I did not understand how to change my code. Could you please tell me in detail, thank you

Here is my code
public override void _Ready()