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

[Mono] Replace float with real_t, other misc C# improvements #17134

Merged
merged 1 commit into from Mar 24, 2018

Conversation

Projects
None yet
9 participants
@aaronfranke
Contributor

aaronfranke commented Mar 1, 2018

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.

@NathanWarden

This comment has been minimized.

Contributor

NathanWarden commented Mar 1, 2018

Wow, super exciting Aaron! :)

@fire

This comment has been minimized.

Member

fire commented Mar 1, 2018

Ah, this is still [c#] only. It still requires the real_t patch I started on.

Good work on C# though.

@aaronfranke

This comment has been minimized.

Contributor

aaronfranke commented 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.

@akien-mga akien-mga added this to the 3.1 milestone Mar 1, 2018

@@ -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);

This comment has been minimized.

@akien-mga

akien-mga Mar 1, 2018

Member

Could you squash that into the original commit?

This comment has been minimized.

@aaronfranke

aaronfranke Mar 1, 2018

Contributor

I've never done that before. After looking it up, I am confused, don't you have the option to "squash and merge"?

This comment has been minimized.

This comment has been minimized.

@aaronfranke

aaronfranke Mar 2, 2018

Contributor

Thanks for that article link, it's quite helpful. I was able to figure it out, it's now just one commit.

@aaronfranke aaronfranke changed the title from Replace float with real_t, other misc C# improvements to [Mono] Replace float with real_t, other misc C# improvements Mar 2, 2018

@akien-mga

This comment has been minimized.

Member

akien-mga commented Mar 2, 2018

CC @neikeq

@aaronfranke aaronfranke referenced this pull request Mar 4, 2018

Merged

Update wrap functions #17254

@neikeq

This comment has been minimized.

Member

neikeq commented Mar 7, 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.

@neikeq

This comment has been minimized.

Member

neikeq commented Mar 16, 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.

@neikeq

This comment has been minimized.

Member

neikeq commented Mar 16, 2018

Hopefully I can test this on some projects tomorrow. I think I'll need to do some changes to the bindings generator.

@aaronfranke

This comment has been minimized.

Contributor

aaronfranke commented Mar 17, 2018

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!

@NathanWarden

This comment has been minimized.

Contributor

NathanWarden commented Mar 17, 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.

@aaronfranke

This comment has been minimized.

Contributor

aaronfranke commented 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.

@neikeq

This comment has been minimized.

Member

neikeq commented Mar 21, 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 double args when compiled with float is 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.

@aaronfranke

This comment has been minimized.

Contributor

aaronfranke commented Mar 21, 2018

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?

@neikeq

This comment has been minimized.

Member

neikeq commented Mar 21, 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.

@neikeq

This comment has been minimized.

Member

neikeq commented Mar 21, 2018

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.

@aaronfranke

This comment has been minimized.

Contributor

aaronfranke commented Mar 21, 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: arnfranke@yahoo.com and maybe also @NathanWarden

@neikeq

This comment has been minimized.

Member

neikeq commented Mar 22, 2018

@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 stores double while it actually casts the parameters to float under the hood, losing precision.
  • It's inconsistent with other methods that receive real_t as a parameter and won't be compatible either.

What do you think?

@paulloz

This comment has been minimized.

Contributor

paulloz commented Mar 22, 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.

@neikeq

This comment has been minimized.

Member

neikeq commented Mar 22, 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.

@PJB3005

This comment has been minimized.

Contributor

PJB3005 commented Mar 22, 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.

@aaronfranke

This comment has been minimized.

Contributor

aaronfranke commented Mar 23, 2018

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)

@aaronfranke

This comment has been minimized.

Contributor

aaronfranke commented 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.

Replace float with real_t, default Vectors, other misc C# improvements
Replace float with real_t in most files, defined at the top of each file via using. Objects such as Vector3 now accept doubles as inputs, and convert to real_t internally. I've added default Vectors such as Vector3.Zero. Other misc C# improvements such as Mathf.RoundToInt(). Color continues to use float only because high precision is not needed for 8-bit color math and to keep things simple. Everything seems to compile and work fine, but testing is requested, as this is the first time I've ever contributed to Godot.

@neikeq neikeq merged commit a8d8c06 into godotengine:master Mar 24, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@neikeq

This comment has been minimized.

Member

neikeq commented Mar 24, 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?

@aaronfranke

This comment has been minimized.

Contributor

aaronfranke commented Mar 27, 2018

@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)

@NathanWarden

This comment has been minimized.

Contributor

NathanWarden commented 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. :)

@hpvb

This comment has been minimized.

Member

hpvb commented Apr 14, 2018

Cherry picked into 3.0.3

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