Attributes not working with generic classes about godotonready HOT 14 CLOSED

I am aware that I haven't declared the class as partial in the example, however doing so results in the same behaviour. Interestingly, the compiler doesn't warn about partial missing, I assume it's because it finds the other error first

from godotonready.

Thanks for the report! Fixing it.

I hadn't run into this because Godot doesn't let you attach a generic node script to an object in the editor, and I assumed this doesn't work. But, I've learned more recently that making a script based on it like Foo : StateMachine<Sprite> lets you attach it to a node just fine.

Interestingly, the compiler doesn't warn about partial missing, I assume it's because it finds the other error first

Well, when StateMachine doesn't match up with StateMachine<T>, you actually don't need partial, because these are two entirely different classes as far as C# is concerned. 😄

from godotonready.

Fixed in 1.1.2, let me know if it doesn't work out!

from godotonready.

Well, when StateMachine doesn't match up with StateMachine, you actually don't need partial, because these are two entirely different classes as far as C# is concerned. 😄

Oh I see, that makes sense.

I am still getting the following error in another class with version 1.1.2:
<project folder>\Scripts\State.cs(6,30): The type 'T' of 'State<T>.stateHolder' is not supported. Expected a Resource or Node subclass.

My guess is that it is caused by the generic type bounds not being copied to the generated class.

Partial__StateMachine.cs

using Godot;
using System;

public partial class StateMachine
	<T>
{

	[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.
*/

State.cs (original source where the error is thrown)

using Godot;
using GodotOnReady.Attributes;

public abstract partial class State<T> : Node where T : Node
{
    [OnReadyGet] protected T stateHolder = null!;
    [OnReadyGet] protected StateMachine<T> stateMachine = null!;

    public virtual void StatePhysicsProcess(float delta) { }
    public virtual void StateProcess(float delta) { }
    public virtual void Enter() { }
    public virtual void Exit() { }
}

Partial__State.cs

using Godot;
using System;

public partial class State
	<T>
{

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

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

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

from godotonready.

Actually, that swings the other way: as long as the generic type list matches, only one partial class has to have the generic constraints. They just can't conflict: https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/partial-classes-and-methods#restrictions

The following keywords on a partial-type definition are optional, but if present on one partial-type definition, cannot conflict with the keywords specified on another partial definition for the same type:

• [...]
• generic constraints

The problem is that when the source generator sees T, it isn't smart enough to look at T's constraints.

This is why State<T> works and T doesn't: the source generator knows that State<whatever> is a node because any State<T> is a node, but it can't tell that T itself is a node because it doesn't look at the constraints.

I think that fixing this means checking to see if the member type is a type parameter, then checking to see if the type parameter has constraints, and if any of the constraints is a Node. (Or a Resource. Or an interface.)

There might be a better API to use to ask Roslyn to tell me if a symbol is convertible to another, so I don't have to do all these intermediate steps myself... but I don't think I know it.

Reopening to address this.

from godotonready.

The problem is that when the source generator sees T, it isn't smart enough to look at T's constraints.

Ooh okay, well that's odd.

I'd love to contribute to solve the issue, although I'm not familiar with source generators or the quirks of c# being used in the project. Let me know if I can be of any help :D

By the way, I have other questions not related to this subject, where could I ask them?

from godotonready.

I'd love to contribute to solve the issue, although I'm not familiar with source generators or the quirks of c# being used in the project. Let me know if I can be of any help :D

No worries--for me, it's just a matter of poking around at the API and in the debugger to figure out how to get the data I need, then writing ifs around it. 😄 The only critical bit of info that's easy to miss is how to activate this:

I have a powershell window in the godot/ directory, and I run $env:DebugGodotOnReadySourceGenerator = 'true' to set the env var (for the lifetime of that particular console), then run dotnet build-server shutdown ; dotnet msbuild each time I want to start a build/debug session. (Doing this with an env var in an individual powershell window means I don't get annoying Debugger.Launch(); popups all the time while trying to build in my IDE.)

I already got this fixed, I believe (will build a new package shortly), but maybe if you hit something you want to dig into in the future or want to try your hand at a fix, you could try that.

By the way, I have other questions not related to this subject, where could I ask them?

Still related to GodotOnReady though, I assume? 😄 Feel free to file an issue.

from godotonready.

Ok, 1.1.4 is available and should have this working.

from godotonready.

Still related to GodotOnReady though, I assume? 😄 Feel free to file an issue.

Okay! I will

Ok, 1.1.4 is available and should have this working.

It is fixed for where T: Node, or node subclasses (tested with KinematicBody2D), however if I change it to where T: class (with no interface) the error is back. It's not something too useful, but if you write the code manually, it works, so I wanted to point that out.

from godotonready.

where T: class (with no interface)

Can you give an example where this is useful (to any degree)? Having a hard time imagining it.

from godotonready.

It's not something too useful, but if you write the code manually, it works, so I wanted to point that out.

I can't think of any use case either, since you can't attach a script whose class doesn't inherit from Node, but I noticed it while testing and I wanted to let you know. I think it is perfectly fine to leave it like this.

from godotonready.

Hmm, for some reason I thought GetNode<T> was constrained to T : Node, but of course it's not, and I should have realized, because I've been passing it interfaces since #30!

With that in mind, I thought of a reasonable scenario that requires GodotOnReady to accept T : class:

public partial class FetchByGeneric<T> : Node where T : class
{
	[OnReadyGet] public T F { get; set; }
}

public partial class FetchByGenericLabel : FetchByGeneric<Label>
{
	[OnReady] private void TalkAboutIt() => GD.Print("Label text is:", F.Text);
}

public partial class FetchByGenericShout : FetchByGeneric<IShout>
{
	[OnReady] private void ShoutIt() => F.Shout();
}

I'll change it. Thanks for prompting me to think about it harder. 😄

(This brings the library further down the road of assuming that things are nodes when they could be resources, but after #30, I'm ok going in that direction.)

from godotonready.

1.1.5 is now available with that change.

from godotonready.

I'll change it. Thanks for prompting me to think about it harder. 😄

No problem! I'm glad I could help with that.

Everything seems to work flawlessly under 1.1.5 :D

from godotonready.