Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C#: Fix signature of generated signal callbacks #67023

Merged

Conversation

raulsntos
Copy link
Member

@raulsntos raulsntos commented Oct 7, 2022

- Use `long` and `double` types since signals currently only support 64-bit types.
- Fix bug for checking if the type name is a class registered in ClassDB.
@raulsntos raulsntos added this to the 4.0 milestone Oct 7, 2022
@raulsntos raulsntos requested a review from a team as a code owner October 7, 2022 10:45
@akien-mga
Copy link
Member

I guess it can close it fully, the confusing "Method not found" part can be tracked in #35910.

Probably also needed for 3.x?

@raulsntos
Copy link
Member Author

Probably also needed for 3.x?

The ClassDB check should be backported, but I believe Godot 3.x uses 32-bit types for scalar Variants.

@akien-mga
Copy link
Member

I believe Godot 3.x uses 32-bit types for scalar Variants.

Variant::INT and Variant::REAL are int64_t and double respectively:

godot/core/variant.h

Lines 155 to 156 in ec21ac5

int64_t _int;
double _real;

The other vector and poolarray types are 32-bit though.

@raulsntos
Copy link
Member Author

raulsntos commented Oct 7, 2022

Then now I'm wondering why we marshal them as 32-bit types in variant_to_mono_object:

case Variant::INT: {
int32_t val = p_var.operator signed int();
return BOX_INT32(val);
}
case Variant::REAL: {
#ifdef REAL_T_IS_DOUBLE
double val = p_var.operator double();
return BOX_DOUBLE(val);
#else
float val = p_var.operator float();
return BOX_FLOAT(val);
#endif
}

But it's likely we don't want to make this type of change in 3.x (see #39629 (comment)).

@neikeq
Copy link
Contributor

neikeq commented Dec 2, 2022

Then now I'm wondering why we marshal them as 32-bit types in variant_to_mono_object:

case Variant::INT: {
int32_t val = p_var.operator signed int();
return BOX_INT32(val);
}
case Variant::REAL: {
#ifdef REAL_T_IS_DOUBLE
double val = p_var.operator double();
return BOX_DOUBLE(val);
#else
float val = p_var.operator float();
return BOX_FLOAT(val);
#endif
}

But it's likely we don't want to make this type of change in 3.x (see #39629 (comment)).

That would break compat in 3.x, and I'd prefer not to do that.

@neikeq neikeq merged commit bcc061e into godotengine:master Dec 2, 2022
@neikeq
Copy link
Contributor

neikeq commented Dec 2, 2022

I guess it can close it fully, the confusing "Method not found" part can be tracked in #35910.

Probably also needed for 3.x?

Is this error still a thing on latest Godot 4? My guess is that, as long as the parameter count matches, the method will be called and the Variant will be adapted to the parameter type, similar to the way it works in C++.

@raulsntos raulsntos deleted the dotnet/fix-signal-callback-generation branch December 2, 2022 16:46
@raulsntos
Copy link
Member Author

My guess is that, as long as the parameter count matches, the method will be called and the Variant will be adapted to the parameter type

That is correct. But I think the issue is that having a parameter count mismatch and getting a "Method not found" error, while accurate, is kind of vague and may be difficult to understand for users.

See, for example, this issue that was recently opened #69519. From the error message, the user thought that there was a bug in Godot because it couldn't find the method even though the method "exists" (the user doesn't see a method with a different signature as a different method).

If instead we were using the error that is used in GDScript, the error would be much friendlier since it explains that the issue is that the parameters don't match:

ERROR: Error calling method from CallbackTweener: 'Node3D(Node3D.gd)::foo': Method expected 1 arguments, but called with 0

@neikeq
Copy link
Contributor

neikeq commented Dec 3, 2022

If instead we were using the error that is used in GDScript, the error would be much friendlier since it explains that the issue is that the parameters don't match:

ERROR: Error calling method from CallbackTweener: 'Node3D(Node3D.gd)::foo': Method expected 1 arguments, but called with 0

GDScript can easily do that because it doesn't allow method overloading. In our case it's harder and it may add overhead.

@raulsntos
Copy link
Member Author

It'd most likely add overhead, but maybe we can do it only in Debug. Not sure if it'd be a good idea to print different errors in Release than in Debug though.

Either way, I was just trying to explain the issue (#35910), it's not about using the wrong parameter type but about the message being confusing to users when a method does exist but the parameter count does not match.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

C#: Area3D node error calling from signal "input_event" - Method not found
3 participants