C#: Deprecate accessor methods and generate correct int and float types #28423

neikeq · 2019-04-25T18:31:59Z

Methods that act as accessors for properties in the same class (like GetPosition and SetPosition are for Position) are now marked as obsolete. They will be made private in the future.
The C# bindings generator correct integer and floating point types (no more int and float everything). Closes randi() of RandomGenerator returns signed int instead of unsigned int #28393

- Normal log messages are no longer warnings. - BindingsGenerator is no longer a singleton. - Added a log function.

- Methods that act as accessors for properties in the same class (like `GetPosition` and `SetPosition` are for `Position`) are now marked as obsolete. They will be made private in the future.

neikeq · 2019-04-25T18:36:20Z

core/object.h

+		METADATA_INT_IS_UI64,
+		METADATA_REAL_IS_FLOAT,
+		METADATA_REAL_IS_DOUBLE
+	};


@reduz Are you fine with this?

Nope, please do the following:

use a property usage for unsigned

use another property usage for 64 bits

There is no point at all in supporting 8 and 16 bits. I prefer this rather than adding yet another field to property info (which is used everywhere), which is just bloat for something so core, and confusing to anyone reading the code.

I know it may bother you to not have the only 4 functions in the whole bindings (which are very rarely used) exposed exactly as they are, but please understand it bothers me a lot more to have to add an extra field to PropertyInfo.

I can see the point in avoiding the bloat of adding a field to a struct that is used everywhere. What if we surround it with #ifdef DEBUG_METHODS_ENABLED?
I can't take the second statement seriously. This code couldn't be simpler. I can't imagine anyone getting confused by it, let alone annoyed.

I'm very serious about it, a PR like this won't go through. My first priority will always be to keep bloat out of core and this is an extremely core class. If for you this inconsistency is unacceptable, I prefer to remove the 4 occurrences of the < 32 bits arguments so there are no longer any functions using them.

Does that mean surrounding #ifdef DEBUG_METHODS_ENABLED is not an option? If not, I'm thinking of an alternative to having the field in PropertyInfo.

No, please let's not add hack to that. Definitely go the property usage way and let's get rid of 8/16 bit access if you need them.

Even modern processors don't really support this type of access, it's a thing of the past.

neikeq · 2019-04-25T18:41:09Z

@karroffel The int/float commit may be of use for GDNative.

reduz · 2019-04-25T21:54:29Z

core/object.h

+		METADATA_INT_IS_UI64,
+		METADATA_REAL_IS_FLOAT,
+		METADATA_REAL_IS_DOUBLE
+	};


Nope, please do the following:

use a property usage for unsigned

use another property usage for 64 bits

There is no point at all in supporting 8 and 16 bits. I prefer this rather than adding yet another field to property info (which is used everywhere), which is just bloat for something so core, and confusing to anyone reading the code.

I know it may bother you to not have the only 4 functions in the whole bindings (which are very rarely used) exposed exactly as they are, but please understand it bothers me a lot more to have to add an extra field to PropertyInfo.

reduz · 2019-04-26T04:15:10Z

core/object.h

+		METADATA_INT_IS_UI64,
+		METADATA_REAL_IS_FLOAT,
+		METADATA_REAL_IS_DOUBLE
+	};


I'm very serious about it, a PR like this won't go through. My first priority will always be to keep bloat out of core and this is an extremely core class. If for you this inconsistency is unacceptable, I prefer to remove the 4 occurrences of the < 32 bits arguments so there are no longer any functions using them.

neikeq · 2019-04-27T02:10:36Z

Tried it in a different way as a last attempt. Let me know if you don't like this one either.

reduz · 2019-04-27T14:36:04Z

I really still think you are over complicating it. That will make it more difficult for those writing binders to understand what it is, and it will add strings to everything that has bindings (I know they are stringnames so it's just a pointer, but still.). The doc generators or code completion may also get confused (or need to take this into consideration) with this given they also use it.

I keep insisting on the approach I proposed and drop 8 and 16 bits. I can remove them from the API if you want, as they are useless.

neikeq · 2019-04-27T19:36:34Z

That will make it more difficult for those writing binders to understand what it is

This change doesn't affect any existing binders. If they don't decide to use it, nothing changes for them.

and it will add strings to everything that has bindings (I know they are stringnames so it's just a pointer, but still.).

I don't understand what you mean. This code is not using String nor StringName at all. All the code in this commit that uses StringName is in the C# bindings generator.

The doc generators or code completion may also get confused (or need to take this into consideration) with this given they also use it.

The doc generators and code completion won't use this AFAIK. GDScript only supports int (64bit integer) so there is no point in using this, and as I said this doesn't break any existing code, it's a completely opt-in thing.

neikeq · 2019-04-27T19:39:28Z

I would rather do it this way, but I don't really mind doing it the way you suggested if you ultimately decide so (there are only a few places using byte and short any way). I just don't understand these points.

reduz · 2019-04-28T12:29:23Z

Nevermind, I misread the PR. I guess this is OK.

akien-mga · 2019-04-29T14:56:29Z

Thanks!

karroffel · 2019-05-03T10:33:45Z

This does not change the way ptrcall receives arguments, right? I'm a bit confused what this does then apart from documentation purposes?

neikeq · 2019-05-03T14:57:17Z

@karroffel It does not change anything in ptrcall. This is metadata you can optionally use when generating bindings for a language.

If a C# method has a parameter of type short, this parameter has to be stored in a variable of the correct type in order to pass it to ptrcall. In the case of any integral type that would be int64_t, as defined in core/method_ptrcall.h.

neikeq added enhancement topic:dotnet labels Apr 25, 2019

neikeq added this to the 3.2 milestone Apr 25, 2019

neikeq requested a review from reduz as a code owner April 25, 2019 18:32

neikeq added 2 commits April 25, 2019 20:34

C# bindings generator cleanup

c20a382

- Normal log messages are no longer warnings. - BindingsGenerator is no longer a singleton. - Added a log function.

C#: Mark generated property accessor methods as Obsolete

3402527

- Methods that act as accessors for properties in the same class (like `GetPosition` and `SetPosition` are for `Position`) are now marked as obsolete. They will be made private in the future.

neikeq force-pushed the dont-forget-to-think-a-name-for-this-branch branch from a92bfad to f2b35eb Compare April 25, 2019 18:35

neikeq commented Apr 25, 2019

View reviewed changes

reduz requested changes Apr 25, 2019

View reviewed changes

reduz requested changes Apr 26, 2019

View reviewed changes

neikeq force-pushed the dont-forget-to-think-a-name-for-this-branch branch 3 times, most recently from e044b17 to 49bd240 Compare April 26, 2019 22:47

C#: Generate the correct integer and floating point types

3380565

neikeq force-pushed the dont-forget-to-think-a-name-for-this-branch branch from 49bd240 to 3380565 Compare April 26, 2019 23:49

akien-mga merged commit 554c0ea into godotengine:master Apr 29, 2019

karroffel mentioned this pull request May 3, 2019

Implement new numeric-type distinction in API JSON in GDExtension #28645

Closed

neikeq deleted the dont-forget-to-think-a-name-for-this-branch branch May 13, 2021 23:46

raulsntos mentioned this pull request Jan 16, 2023

C#: Make property accessors internal #71516

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C#: Deprecate accessor methods and generate correct int and float types #28423

C#: Deprecate accessor methods and generate correct int and float types #28423

neikeq commented Apr 25, 2019 •

edited

Loading

neikeq Apr 25, 2019

reduz Apr 25, 2019 •

edited

Loading

neikeq Apr 25, 2019

reduz Apr 26, 2019

neikeq Apr 26, 2019

reduz Apr 26, 2019

reduz Apr 26, 2019

neikeq commented Apr 25, 2019

reduz Apr 25, 2019 •

edited

Loading

reduz Apr 26, 2019

neikeq commented Apr 27, 2019

reduz commented Apr 27, 2019

neikeq commented Apr 27, 2019

neikeq commented Apr 27, 2019

reduz commented Apr 28, 2019

akien-mga commented Apr 29, 2019

karroffel commented May 3, 2019

neikeq commented May 3, 2019

C#: Deprecate accessor methods and generate correct int and float types #28423

C#: Deprecate accessor methods and generate correct int and float types #28423

Conversation

neikeq commented Apr 25, 2019 • edited Loading

neikeq Apr 25, 2019

Choose a reason for hiding this comment

reduz Apr 25, 2019 • edited Loading

Choose a reason for hiding this comment

neikeq Apr 25, 2019

Choose a reason for hiding this comment

reduz Apr 26, 2019

Choose a reason for hiding this comment

neikeq Apr 26, 2019

Choose a reason for hiding this comment

reduz Apr 26, 2019

Choose a reason for hiding this comment

reduz Apr 26, 2019

Choose a reason for hiding this comment

neikeq commented Apr 25, 2019

reduz Apr 25, 2019 • edited Loading

Choose a reason for hiding this comment

reduz Apr 26, 2019

Choose a reason for hiding this comment

neikeq commented Apr 27, 2019

reduz commented Apr 27, 2019

neikeq commented Apr 27, 2019

neikeq commented Apr 27, 2019

reduz commented Apr 28, 2019

akien-mga commented Apr 29, 2019

karroffel commented May 3, 2019

neikeq commented May 3, 2019

neikeq commented Apr 25, 2019 •

edited

Loading

reduz Apr 25, 2019 •

edited

Loading

reduz Apr 25, 2019 •

edited

Loading