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

Some work on double-precision support #21922

Merged
merged 1 commit into from
Aug 10, 2021
Merged

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Sep 10, 2018

Please see this proposal for the eventual goals this PR is a part of: godotengine/godot-proposals#892

This PR does a lot of work on double-precision support. I have previously split this PR up into smaller ones that are easier to review/merge and now those have been merged. It's small enough now that I don't think splitting this further would help much, the majority of the code that can be reviewed/merged separately has been reviewed/merged. Note that the rest of this PR changes things like arrays/vectors which are a bit more tricky so keep that in mind when reviewing.

In its current state things are mostly working except for a binding issue with integer vector multiplication/division that causes this PR to not compile. I previously had #44441 included in this PR's list of commits but now I've removed it so that this can be merged, and then the integer vector multiplication/division issue can be solved separately.

@fire
Copy link
Member

fire commented Sep 10, 2018

Haven't had time to look and compile the patch, but thank you for continuing with the work. Currently looking at the driver side of this which is dependent on Opengl4 or Vulkan. Since Vulkan is in the roadmap, I'm focusing on that.

This is the dmat4 type for the position.

@reduz
Copy link
Member

reduz commented Sep 12, 2018

This is great! Besides ensuring that everything is using real_t, there is some significant piece of work missing which is making sure that binary serialization uses doubles instead of floats when compiling in this mode (yet keeping compat). One of the places that needs to be modified is:

https://github.com/godotengine/godot/blob/master/core/io/marshalls.cpp

As you can see I added a tiny bit of support for encoding as 64 bits like here:
https://github.com/godotengine/godot/blob/master/core/io/marshalls.cpp#L153

but needs to be:

  • Expanded to handle other datatypes that become doubles (Vector2, Vector3, etc).
  • Make sure when engine is compiled with real_t as 64 bits, writes 64, else 32 (save for real and int because Variant always has those as 64)
  • Make sure, when reading, that this flag is always recognized, so scenes remain compatible.

Similar work needs to be done here:

https://github.com/godotengine/godot/blob/master/core/io/resource_format_binary.cpp

@fire
Copy link
Member

fire commented Sep 15, 2018

Remember the code to turn on 64bit float is in: https://github.com/godotengine/godot/blob/2d3f6945ee7de9bf37139799b15e24c34b1e5a0b/platform/windows/detect.py#L207

godot/platform/windows/detect.py

## Libs
    if env["float"] == "64":
       env.Append(CCFLAGS=['/DREAL_T_IS_DOUBLE'])

Edited: I had cascading errors when turning that on.

@aaronfranke
Copy link
Member Author

aaronfranke commented Sep 15, 2018

@fire Isn't that just for Windows? It's in the platform/windows folder.

EDIT: Done, see code here, I also made it exit if you try to compile 64-bit floats on a 32-bit CPU, even if it works it would be slow and it's not a good idea.

@fire
Copy link
Member

fire commented Sep 15, 2018

You can move it out into SConstruct, but the main point you need to

    if env["float"] == "64":
       env.Append(CCFLAGS=['/DREAL_T_IS_DOUBLE'])

So that real_t is defined as double.

@AndreaCatania
Copy link
Contributor

when compiled with double precision, is important that even bullet is compiled with double precision. btScalar is the type def of floating point variables inside bullet.

@aaronfranke aaronfranke changed the title Some work on double-precision support (float -> real_t) Some work on double-precision support (physics and build) Oct 6, 2018
@aaronfranke
Copy link
Member Author

@reduz Like this? efee149

@fire
Copy link
Member

fire commented Nov 25, 2018

I was blocked when I realize to fully convert the code, I need to touch the float in the position shader and gles3 version of our code didn't double precision matrixes.

However, not doing still gives us more precision. I'm not sure how to calculate it, but it's better converting from double to a float on render than using a float directly.

@aaronfranke
Copy link
Member Author

aaronfranke commented Nov 25, 2018

@fire Perhaps it would be best to worry about the rendering code after Vulkan is added.

Since GPUs work with floats, it would be best to use floats there. The most important thing is for all math (including physics etc) to be in doubles. I can move the world around the player so that the "global" positions of what need to be rendered are within the precision range of floats. Or, it would be really cool if this could be done automatically by the renderer (some kind of "camera-centric" rendering where all objects are position -= camera_position; and camera is at 0,0,0?)

@ghost
Copy link

ghost commented Jul 10, 2021

This change will be a game-changer for Godot! No more '#50251's!

@aaronfranke aaronfranke force-pushed the double branch 3 times, most recently from d07c125 to 46cf773 Compare August 1, 2021 20:30
@aaronfranke aaronfranke force-pushed the double branch 2 times, most recently from bba7dbd to f6f7d5b Compare August 9, 2021 21:13
@aaronfranke aaronfranke marked this pull request as ready for review August 9, 2021 21:20
@aaronfranke aaronfranke changed the title Some work on double-precision support (physics and serialization) Some work on double-precision support Aug 9, 2021
SConstruct Outdated Show resolved Hide resolved
SConstruct Outdated Show resolved Hide resolved
@aaronfranke aaronfranke force-pushed the double branch 2 times, most recently from 3afa13c to 5d198d3 Compare August 9, 2021 21:57
@akien-mga akien-mga merged commit 536950f into godotengine:master Aug 10, 2021
@akien-mga
Copy link
Member

Thanks!

@aaronfranke aaronfranke deleted the double branch August 11, 2021 01:33
@ghost
Copy link

ghost commented Aug 11, 2021

Thank you!!!!!!!!!!!!!!!

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.

8 participants