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

Reduce the amount of casting required for floating points in C# #5403

Open
antonWetzel opened this issue Sep 11, 2022 · 9 comments
Open

Reduce the amount of casting required for floating points in C# #5403

antonWetzel opened this issue Sep 11, 2022 · 9 comments

Comments

@antonWetzel
Copy link

Describe the project you are working on

Experiments with the master branch and CSharp

Describe the problem or limitation you are having in your project

Old behaviour: every floating point is float
New behaviour: methods use double and everything else uses float

Old

//old behaviour
class TestOld : Node2D {
	[Export] float speed;
	[Export] float acceleration;
	[Export] float turnSpeed;

	public override void _Process(float delta) {
		speed += acceleration * delta;
		Position += new Vector2(speed, 0f) * delta;
		Rotation += Mathf.RadToDeg(turnSpeed) * delta;
	}
}

Variant 1: my code uses double

//every interaction with build in members needs a cast
class TestDouble : Node2D {
	[Export] double speed;
	[Export] double acceleration;
	[Export] double turnSpeed;

	public override void _Process(double delta) {
		speed += acceleration * delta;
		Position += new Vector2((float)speed, 0f) * (float)delta;
		Rotation += Mathf.RadToDeg((float)turnSpeed) * (float)delta;
	}
}

Variant 2: my code uses float

//if only the parameters use double there are only casted to float and never used as doubles
class TestFloat : Node2D {
	[Export] float speed;
	[Export] float acceleration;
	[Export] float turnSpeed;

	public override void _Process(double delta) {
		speed += acceleration * (float)delta;
		Position += new Vector2(speed, 0f) * (float)delta;
		Rotation += Mathf.RadToDeg(turnSpeed) * (float)delta;
	}
}

No matter what type I use I need to cast alot, which does not feel nice and pollutes the code.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Solutions

  • (use double as real_t solves the problem but changes a lot more)
  • Implicit conversion from double to real_t (prefered)
  • revert methods parameters back to float (not prefered)

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

New

//new behaviour
class TestNew : Node2D {
	[Export] double speed;
	[Export] double acceleration;
	[Export] double turnSpeed;

	public override void _Process(double delta) {
		speed += acceleration * delta;
		Position += new Vector2(speed, 0f) * delta;
		Rotation += Mathf.RadToDeg(turnSpeed) * delta;
	}
}

Problem

Casting from double to float can't be added to csharp, so real_t must be an opaque struct or every constructur/member includes a copy with double

If this enhancement will not be used often, can it be worked around with a few lines of script?

Not possible

Is there a reason why this should be core and not an add-on in the asset library?

Not possible

@Calinou
Copy link
Member

Calinou commented Sep 12, 2022

cc @neikeq

@raulsntos
Copy link
Member

For context this behavior was changed by godotengine/godot#65168 in order to fix issue godotengine/godot#65139.

Related issues:

The new behavior is correct since it uses the right types as defined by core and this is also how godot-cpp generates its bindings (see https://github.com/godotengine/godot-cpp/blob/24f97739a189c6edef5ba2516b29abe60f97b004/binding_generator.py#L1801-L1829).

Let me address some of your suggestions:

use double as real_t

This would make vector types use double which is incorrect, in core scalars use 64-bit types but vectors use 32-bit types (unless the engine is built with --float=64).

Implicit conversion from double to real_t

real_t is not a type, it gets replaced by double or float on compilation depending on if you build with --float=32 or --float=64 (the default is 32). When real_t is float, this means you are asking for us to implement an implicit conversion from double to float which we can't implement because we don't own these types but even if we could it wouldn't be a good idea because it's a lossy conversion.

To quote the .NET guidelines on conversion operators:

❌ DO NOT provide an implicit conversion operator if the conversion is potentially lossy.

For example, there should not be an implicit conversion from Double to Int32 because Double has a wider range than Int32. An explicit conversion operator can be provided even if the conversion is potentially lossy.

revert methods parameters back to float

This would be essentially the same as implicitly converting the double we get from core to a float, which is again a lossy conversion and would be a bad idea as explained above. The only difference with the previous suggestions is the conversion is done by the engine and not on C#'s side but that doesn't change the fact that you lose precision.


My two cents, there is nothing wrong with using explicit conversion when you know the loss of precision is acceptable and being explicit is much better because otherwise you might accidentally and unintentionally lose precision without realizing it.

You might have a point about Mathf though, in core the math functions are defined with overloads for both double and float (see https://github.com/godotengine/godot/blob/7535c6c1636d37a76d79361092887f7dd13283df/core/math/math_funcs.h#L235-L236). In the BCL we have System.Math and System.MathF, the reason why there's two separate classes is there used to be only Math with doubles and in order to implement float overloads they had to do so in a separate class (MathF) to avoid breaking compatibility (see dotnet/runtime#14353 (comment)).

@YuriSizov YuriSizov changed the title Csharp and floating point: to many casts Reduce the amount of casting required for floating points in C# Sep 12, 2022
@aaronfranke
Copy link
Member

It might be worth undoing the recent changes in godotengine/godot#65168, so that _Process etc use real_t, for the sake of usability for C# users. If users care about precision, they can compile Godot with doubles.

@neikeq
Copy link

neikeq commented Sep 15, 2022

The problem about reverting that PR is that there are other APIs that can be broken with int or float.

I plan to make it possible to specify size and sign for virtual methods and signals, just as it's done with normal methods and properties. That should solve some issues, but not _Process because it's made to use double both in the C++ virtual method bind and in the get_process_delta_time method.

BTW you cast once into a field, to avoid repeating:

public override void _Process(double delta)
{
    float deltaf = (float)delta;
    speed += acceleration * deltaf;
    Position += new Vector2(speed, 0f) * deltaf;
    Rotation += Mathf.RadToDeg(turnSpeed) * deltaf;
}

@ashelleyPurdue
Copy link

ashelleyPurdue commented Dec 22, 2022

This would make vector types use double which is incorrect, in core scalars use 64-bit types but vectors use 32-bit types (unless the engine is built with --float=64).

Well, there's the real problem! Why do scalars and vectors use different precisions in the first place? That's pretty counter-intuitive.

My two cents, there is nothing wrong with using explicit conversion when you know the loss of precision is acceptable and being explicit is much better because otherwise you might accidentally and unintentionally lose precision without realizing it.

Is there really any precision being lost, though? If you're multiplying a vector by a scalar, it's the vector that's getting changed, not the scalar. The vector isn't losing any precision, because it was already limited to 32 bits. The only reason this would catch someone by surprise is if they assumed Vector2 uses the same precision as delta. Which, again, brings me back to my previous point. Why aren't they the same?

I plan to make it possible to specify size and sign for virtual methods and signals, just as it's done with normal methods and properties. That should solve some issues, but not _Process because it's made to use double both in the C++ virtual method bind and in the get_process_delta_time method.

If it doesn't affect _Process, then it doesn't help with this problem at all. Multiplying a velocity vector by a delta time is probably the most common thing you'd ever do in a _Process() or _PhysicsProcess() method, so it really should be ergonomic.

IMO, Godot 4 can't be considered ready for release until this is fixed. It's an extremely common use case that has seen a very obvious regression. One of these things needs to happen:

  • Add overloads for _Process() and _PhysicsProcess() that accept float instead of double
  • Change Vector2 and Vector3 to support multiplying by doubles, even if it incurs precision loss
  • Change Vector2 and Vector3 to use doubles internally instead of floats, to be consistent with scalars
  • Change the core engine to use floats for scalars, to be consistent with vectors

Those last two options are probably out of the question, given that they'd have consequence that I can't fully comprehend. I seriously suggest giving options 1 and 2 a real look, though. I think they would be an excellent compromise.

@neikeq
Copy link

neikeq commented Dec 23, 2022

  • Add overloads for _Process() and _PhysicsProcess() that accept float instead of double

That's a solution I'm considering. One thing that worries me is that it may cause confusion about the call order if for some reason a class overrides the double overload while its base class overrides the float overload. It may also cause annoyances with code-completion when trying to override the method, as the overload selected by default may not be the one desired.

@aaronfranke
Copy link
Member

  • Change Vector2 and Vector3 to support multiplying by doubles, even if it incurs precision loss

It's not quite that simple. If they supported only multiplying by doubles, then that would be a lot of unnecessary casting. If they supported both types in the operators, then other numeric types will not work because the cast will be ambiguous (ex: you wouldn't be able to do myVector * 5 because 5 is an int and it's ambiguous if that should be implicitly converted to float or double).

  • Change Vector2 and Vector3 to use doubles internally instead of floats, to be consistent with scalars

This can be done by compiling Godot with double-precision support. Aside from this proposal, this is the preferred option for games that need precise calculations or to handle large scales, so you may want to use this anyway.

  • Change the core engine to use floats for scalars, to be consistent with vectors

This won't be done. However, note that in Godot 3.x, this is how it was exposed in C#, with the process method using float in C# despite it using double internally.

@Hamster5295
Copy link

  • Add overloads for _Process() and _PhysicsProcess() that accept float instead of double

That's a solution I'm considering. One thing that worries me is that it may cause confusion about the call order if for some reason a class overrides the double overload while its base class overrides the float overload. It may also cause annoyances with code-completion when trying to override the method, as the overload selected by default may not be the one desired.

IMO there can be different names for the override, like _ProcessF(float) or _ProcessD(double). (May not be correctly named. Just an example :D ). This way, the call order can be fixed and be noted at the docs, and the problems with code completion can be avoided.

Since C# users have long been using _Process(float) and indeed float is much more convenient when multiplied with Vectors, which is quite common in game deveopment, I'd prefer a _ProcessD(double) and a _Process(float) as the solution. (_ProcessD(double) can be named with something else. It's just an example 🐱 )

@Artromskiy
Copy link

Not familiar with Godot, but wouldn't this be a problem in the future or a redundant feature in the engine? It seems to me that it would be much better to make it possible to perform vector operations on both float and double scalars, so precision will not be lost if the engine uses double precision for vectors, and you do not have to pull another method like _Process

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

Successfully merging a pull request may close this issue.

8 participants