[Mono] Replace float with real_t, other misc C# improvements
source link: https://github.com/godotengine/godot/pull/17134
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
I've replaced float with real_t in most files, defined at the top of each file via using and #if.
Note that this is a stepping stone to double-precision support in Godot C#, I don't expect it to compile with REAL_T_IS_DOUBLE yet. What this patch does do is abstract much of what used to be float to real_t. It seems to works perfectly fine when compiled with float with my test code, though I would recommend testing this patch with actual projects to make sure it works. There's not really any downside to merging this patch, assuming everything works as expected.
Color continues to use float only because high precision is not needed for color math and to keep things simple. The String-float parsing is also left as float for now.
I've added default Vectors such as Vector3.Zero etc. I used a private static readonly variable and a public static getter, following the example of the existing Quat.Identity code.
I made a few other misc C# improvements such as Mathf.RoundToInt().
Again, I would advise testing before merging. I may have missed a thing or two. If you are having issues with compiling the changes, delete ~/.local/share/godot/mono and remake the Mono glue.
Contributor
NathanWarden commented on Mar 1, 2018
Wow, super exciting Aaron! :)
Ah, this is still [c#] only. It still requires the real_t patch I started on.
Good work on C# though.
Member
Author
aaronfranke commented on Mar 1, 2018
Yes @fire so you should rebase your branch soon but as my patch doesn't add any compiler flags and doesn't break any existing functionality I decided to commit back to master.
modules/mono/glue/cs_files/Vector2.cs
Outdated
@@ -220,13 +220,13 @@ public Vector2 Tangent()
}
private static readonly Vector2 zero = new Vector2 (0, 0);
private static readonly Vector2 one = new Vector2 (1, 1);
aaronfranke on Mar 2, 2018
Author
Member
I've never done that before. After looking it up, I am confused, don't you have the option to "squash and merge"?
aaronfranke on Mar 2, 2018
Author
Member
Thanks for that article link, it's quite helpful. I was able to figure it out, it's now just one commit.
changed the title Replace float with real_t, other misc C# improvements
[Mono] Replace float with real_t, other misc C# improvements
Member
neikeq commented on Mar 8, 2018
Sorry, I'm still busy with final exams and this looks like a PR to review well before merging. I'll be back for sure on Tuesday.
Member
neikeq commented on Mar 17, 2018
I'm not sure about providing a method with float parameters and another for double. I understand why you made it this way, so the same code compiles both with REAL_T_IS_DOUBLE and without it. However, wouldn't it be confusing? What if I am compiling without REAL_T_IS_DOUBLE, and I pass a double expecting it to not lose precision?
This won't be too much of a problem if Godot's official binaries are built with REAL_T_IS_DOUBLE, which I think they should since GDScript already uses double precision.
Member
neikeq commented on Mar 17, 2018
Hopefully I can test this on some projects tomorrow. I think I'll need to do some changes to the bindings generator.
However, wouldn't it be confusing? What if I am compiling without REAL_T_IS_DOUBLE, and I pass a double expecting it to not lose precision?
I doubt this confusion would occur in the real world. It's pretty easy to check what type the number is stored as by simply checking the autocomplete (Vector3 v then v.x shows public float x).
I do believe it would be a good idea to build with double by default, once support for doubles is completed (#17134 and #12299), and if we can ensure project compatibility and high performance (@NathanWarden has shown concern about double possibly being noticeably slower in some situations)
In fact, supporting double args when compiled with float is required for project (back-)compatibility!
Contributor
NathanWarden commented on Mar 18, 2018
double would impact anyone still on a 32 bit Android phone, which knowing how Android devices are in the market probably won't be completely gone for a few more years.
The rarer case is that double would impact anyone who has a use case for SIMD instructions. But, I suppose if someone really wants to add these to the C# side of things they could just make their own wrapper code to use float instead.
If it doesn't make a mess out of the project it would be nice to be able to keep both for now, but I would completely understand if we just went all double to keep things clean.
Member
Author
aaronfranke commented on Mar 21, 2018
@neikeq I have more code to contribute, unrelated to double-precision support so therefore I'll put it in a separate PR. But I can't do that until this PR is merged since it's built on top of this PR's code.
Member
neikeq commented on Mar 22, 2018
After thinking for a bit, I don't think any changes are required in the bindings generator. We would need to make the editor build the solution with the REAL_T_IS_DOUBLE flag.
I mantain my concerns about providing two methods (for float and double).
In fact, supporting
doubleargs when compiled withfloatis required for project (back-)compatibility!
Yes, I understand the reasoning behind it. But I still think it would cause confusion. If REAL_T_IS_DOUBLE was the default, the story would be different.
Any way, everything else looks good, so I won't prevent this from being merged if the community is in favour of it.
BTW, we will need to have some unit tests soon, to make sure nothing breaks.
Purely accepting real_t would be more straightforward but less functional than dual double/float. My vote is to keep both. Let me know if you want me to change the constructors to real_t.
Otherwise, is testing the only concern preventing merging then?
Member
neikeq commented on Mar 22, 2018
We will need testing at some point, regardless of this PR.
It also bothers me to do this things without knowing what the plans are for REAL_T_IS_DOUBLE in the engine.
These do not block this PR from being merged though, since this code doesn't change anything (except the two methods for float and double thing) unless REAL_T_IS_DOUBLE is enabled. You can see the same changes already being done in the engine.
I wish there was a team or something I could tag regarding C# changes. This way we could get some quick feedback from other people about what they thing on changes like the alternative method thing in this PR.
Member
Author
aaronfranke commented on Mar 22, 2018
I see. You'll have to ask @fire about the plans for the compiler flag(s).
This PR basically not changing many things without the compiler flag enabled is exactly why I decided to create a PR directly to Godot rather than a PR to @fire 's fork. If it doesn't break anything I want to push upstream whenever possible.
A mailing list or something for C# changes sounds like a great idea, for reviewing and feedback. If you make one, feel free to add me to it: [email protected] and maybe also @NathanWarden
@PJB3005 @mysticfall @paulloz @NathanWarden Sorry to bother you. You were quite active recently on mono related issues and PRs so I would like your opinion on this matter.
This PR uses an alias named real_t to represent either float or double depending of the REAL_T_IS_DOUBLE constant. But it makes an exception with constructors, where instead of using real_t, it provides two constructors, one with a float parameter and other with a double parameter.
IINW, this is in order to make the following code compile whether REAL_T_IS_DOUBLE is enabled or not (@aaronfranke please, correct me if i'm wrong on this):
// this won't without `REAL_T_IS_DOUBLE` because the constructor expects floats, but we pass two doubles
// so we have to provide both constructors
new Vector2(13.23, 13.23d);
The reasons I don't like this are:
- It has the potential of confusing the user, when building without
REAL_T_IS_DOUBLE, making them think the type storesdoublewhile it actually casts the parameters tofloatunder the hood, losing precision. - It's inconsistent with other methods that receive
real_tas a parameter and won't be compatible either.
What do you think?
Contributor
paulloz commented on Mar 23, 2018
Hey, nothing to be sorry about, feel free to tag me if needed/wanted.
I personally share @neikeq's reluctances regarding the parameters inconsistency and the fact that (imho) when explicitly giving a double as argument, you don't expect it to be stored as a float.
I understand the reasoning behind the constructors, but (correct me if I'm wrong) C# support is still considered alpha and thus do not have to be completely back-compatible.
Sidenote, VERSION.txt should be incremented.
Member
neikeq commented on Mar 23, 2018
Sidenote, VERSION.txt should be incremented.
Ah, yes. There is another PR which already incremented it to 2. Weird that's not causing a conflict.
Contributor
PJB3005 commented on Mar 23, 2018
I don't mind being mentioned. Hell I was wondering about how Godot's support for doubles was anyways so this is nice to see. Hell sign me up for that mailing list idea if it goes anywhere.
If constructors get the overloads then regular methods need it too, probably. To avoid confusion you could use [Obsolete], while technically incorrect, to signal that the double is getting trimmed. It'd get hella spammy but you did warn them clearly.
The problem kinda is that there's no way you're gonna get code to be compatible to both double and float reasonably (without using floats everywhere, thus defeating a lot of the point) because C# doesn't allow you to typedefs that can be imported like Rust does. I'd say compiler warning for doubles if they'd be trimmed is as good idea.
Honestly, I haven't thought about the fact that the behavior is inconsistent with other methods. To me this is a bigger issue than any confusion caused by accepting double on float-compiled Godot.
Arguably there are not many precision issues if you choose to use only float for many of the methods, like Vector multiplication (as whole numbers used for scalers can be exactly represented with low-precision floats in most cases) but it would be confusing to have mixed float/double like that.
I think I'll go ahead and make a fixup commit to make constructors real_t (and increment VERSION)
Member
Author
aaronfranke commented on Mar 23, 2018
Incrementing version is causing Git to throw a fit... I'll keep it at 2 for now (it can be incremented later, or changes can be forced by deleting ~/.local/share/godot/mono)
Honestly, there are a lot of issues with checking the version in this way. We should re-think our approach. A text file storing a version number is not ideal.
Member
neikeq commented on Mar 25, 2018
Thanks!
Regarding the versioning approach, it was added as a quick fix. It can always be changed if it doesn't fit our needs. What issues did you find with it? What would you propose as an alternative?
@neikeq The biggest issue is that it hinders the ability for us to have simultaneous Mono PRs.
-
Each should logically increment the version to signal the compiler to rebuild with the changes.
-
When a PR is merged after another, each incrementing the version, if the resulting version is the same then users on bleeding versions don't see the changes (as @maikramer experienced here)
-
If a PR is merged, other PRs need to rebase and re-increment.
Furthermore, it makes changing features of C# more difficult, especially if people don't know about incrementing VERSION and wonder why code changes do nothing (as I've experienced)
Alternative: Perhaps a checksum of each file to find out of the files changed (and if so, recompile)
Contributor
NathanWarden commented on Mar 28, 2018
If it needs versioning for consistency checking then I like the checksum approach @aaronfranke mentioned, and then rebuild if that changed.
If it doesn't need versioning, then it seems to me like it could just recompile no matter what. C# builds super fast to start with and the code base for this is pretty small which makes it even faster.
But, I completely agree, the first time I tried to change anything in the C# project it was pretty confusing. It would be nice to make this not be an issue for any developers who want to help improve the C# side of things. :)
Member
hpvb commented on Apr 15, 2018
Cherry picked into 3.0.3
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK