Change c++ code to work with doubles #232

SlashScreen · 2023-10-19T08:13:56Z

solves half of issue #30 - doesn't touch shader though

SlashScreen · 2023-10-19T08:14:35Z

Don't merge this until I've gone through everything

TokisanGames · 2023-10-19T08:44:04Z

You know, thinking about it I think its fine that all instances here are switched to real_t. I was thinking using set_cull_margin(real_t) is a waste of space using a double when only a float is needed. But if we're calling it from gdscript what is coming in is a double anyway.

The only concern I have is with saved variables. Say version for instance. If it's saved as a float into Storage. Then later read by a double engine, is it seamlessly converted? If it's saved as a double, can it be opened by a float engine without issue? If version and all of the other data can be read and written by both engines then it's fine to do everything as real_t.

Also include a line somewhere like __ready() that prints LOG(ERROR, "Float: ", sizeof(float), " Real_t: ", sizeof(real_t)); and include the result here so we can make sure they are different.

SlashScreen · 2023-10-19T17:20:17Z

Asked AThousandShips:

Me:
If a double (f64) value is serialized into a text file, and then opened using a single-precision compiled engine, what happens?
is it just truncated as a float, as if you were casting
And what about the reverse?

AThousandShips: 
Again most are stored as double, just some types rely on the precision setting
Like Vector2/3/4, Basis, Transform2D/3D, etc.
Most cases of float in the engine are f64, and serialisation isn't affected unless you're storing a type that depends

Shouldn't be an issue.

SlashScreen · 2023-10-19T17:25:00Z

SlugFiller:
Also, serialization into a text file truncates floating point way below the limit of single. There's an issue, and I believe, also a PR about it

TokisanGames · 2023-10-20T07:44:23Z

src/terrain_3d.cpp

@@ -75,7 +75,7 @@ void Terrain3D::_initialize() {

 void Terrain3D::__ready() {
 	_initialize();
-	LOG(WARN, "Float: ", sizeof(float), " Real_t: ", sizeof(real_t));
+	LOG(WARN, "Float: ", Variant(sizeof(float)), " Real_t: ", Variant(sizeof(real_t)));


We don't need these in the repo, I just want you to run it once to confirm that building with precision=double does something, and I'd like you to post your results in the comments.

SlashScreen · 2023-10-21T07:08:35Z

Got it to build, as I said in discord. Let me know if I need to change anything.

TokisanGames · 2023-10-21T19:38:03Z

godot-cpp

Remove from PR. I will update godot-cpp later. Current commit tag is godot-4.1.1-stable

TokisanGames · 2023-10-21T19:39:00Z

project/project.godot

@@ -12,7 +12,7 @@ config_version=5

 config/name="Terrain3D"
 run/main_scene="res://demo/Demo.tscn"
-config/features=PackedStringArray("4.1")
+config/features=PackedStringArray("4.1", "Double Precision")


Remove from PR. Users can enable this

TokisanGames · 2023-10-21T19:40:12Z

src/terrain_3d.cpp

@@ -75,6 +75,7 @@ void Terrain3D::_initialize() {

 void Terrain3D::__ready() {
 	_initialize();
+	// LOG(WARN, "Float: ", Variant(int(sizeof(float))), " Real_t: ", Variant(int(sizeof(real_t))));


TokisanGames · 2023-10-21T19:53:43Z

src/util.cpp

@@ -58,7 +58,7 @@ Vector2 Util::get_min_max(const Ref<Image> p_image) {
 }

 /**
- * Returns a Image of a float heightmap normalized to RGB8 greyscale and scaled
+ * Returns a Image of a real_t heightmap normalized to RGB8 greyscale and scaled


Change to: Returns a scaled, RGB8 greyscale Image of a heightmap normalized to its heights

SlashScreen · 2023-10-21T20:06:40Z

oh dear. Will me removing godot-cpp from this PR cause issues in the main branch? The CI is freaking out right now.

TokisanGames · 2023-10-21T20:08:33Z

src/terrain_3d_editor.cpp

-					float alpha_clip = (brush_alpha < 0.1f) ? 0.0f : 1.0f;
-					int index_base = int(src.r * 255.0f);
-					int index_overlay = int(src.g * 255.0f);
+					real_t alpha_clip = (brush_alpha < real_t(0.1f)) ? 0.0f : 1.0f;


Was there any sort of warning from the compiler comparing real_t brush_alpha and 0.1f?

If not, let's remove all of the literal conversions. I now think we only need the real_t conversions for ints. For floats to doubles it is free and unnecessary, unless there are warnings. Let's remove them all from here down to the last lerp on line 233.

https://stackoverflow.com/questions/13045485/division-of-double-with-a-float

The only need I see is where it is casting dest_index

There were warnings when it came to function parameters; math.lerp didn't like an argument list of mixed floats and real_ts. I can remove the ones outside of those parameters, though.

Add any casts to remove warnings. Otherwise, we don't need it for float to double.

TokisanGames · 2023-10-21T20:11:29Z

I looked closer at the godot source. Excluding modules & thirdparty code, .h/.cpp lines using float: 10212, real_t: 6091.

There are only 2 usages of float in all of the physics servers, none in navigation.

Rendering is 99% float, needed only for locations or distance. The other uses are unnecessary.

The moviewriter is a new addition and has 3 floats, 0 real_t.

So my conclusions are:

it appears to be used where it's needed, so hopefully doubles actually does work.
Usage is all over the place and not very disciplined. When real_t is used in files, it is often used on inconsequential things and who knows if double support is completed and actually works.
Blanket search and replace will waste space for those using it, but unlikely to cause a problem.
Literals like 5.0 are doubles, and 5.0f are floats. Godot has real_t x = 1.0f; all over, so I don't think we need an explicit conversion in many cases unless the compiler produces a warning.

This link says float to double conversion on x87 fpu is free, and I've seen that other places as well.
https://stackoverflow.com/questions/13045485/division-of-double-with-a-float

So I think for consistency like the physics and navigation servers, we can use real_t everywhere. Let's wrap up this and merge it in. The shader portion can come separately.

Please make the following changes:

Address all the individual comments I made
Remove the changes to godot-cpp and project.godot. I will update the former soon along with a version upgrade. The godot-cpp tag is godot-4.1.1-stable
Squash your changes somewhat or completely
Rebase

SlashScreen · 2023-10-21T20:15:29Z

I've addressed the individual comments. I will soon remove explicit conversions, squash (hopefully I don't screw it up this time), and rebase.

TokisanGames · 2023-10-21T20:17:13Z

oh dear. Will me removing godot-cpp from this PR cause issues in the main branch? The CI is freaking out right now.

I don't mean removing godot-cpp from your repository, aka, deleting the folder and pushing that change. I mean reverting your godot-cpp to the previous version (git checkout godot-4.1.1-stable) and pushing that. That will revert the change you made and reset it back to the version in the main branch.

TokisanGames · 2023-10-22T09:17:41Z

Great job, thank you. In the future don't use git merge. You can do anything else like rebase, but let github do all merging.

SlashScreen · 2023-10-22T09:20:58Z

For the record, I never did git merge, I let GitHub do all of that. And, I never found any “merge” actions when I was rebasing.

…

On Sun, Oct 22, 2023 at 02:17, Cory Petkovsek ***@***.***(mailto:On Sun, Oct 22, 2023 at 02:17, Cory Petkovsek <<a href=)> wrote: Great job, thank you. In the future don't use git merge. You can do anything else like rebase, but let github do all merging. — Reply to this email directly, [view it on GitHub](#232 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AC6NJR5KLAXRSOONS3YBIHDYATQD7AVCNFSM6AAAAAA6GYOKROVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONZUGA2DCNZXGM). You are receiving this because you authored the thread.Message ID: ***@***.***>

TokisanGames marked this pull request as draft October 19, 2023 08:15

TokisanGames reviewed Oct 20, 2023

View reviewed changes

SlashScreen marked this pull request as ready for review October 21, 2023 07:08

TokisanGames reviewed Oct 21, 2023

View reviewed changes

godot-cpp Outdated

Copy link

Owner

TokisanGames Oct 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove from PR. I will update godot-cpp later. Current commit tag is godot-4.1.1-stable

TokisanGames reviewed Oct 21, 2023

View reviewed changes

SlashScreen requested a review from TokisanGames October 21, 2023 20:02

TokisanGames reviewed Oct 21, 2023

View reviewed changes

SlashScreen and others added 3 commits October 21, 2023 18:41

changed all float to real_t

40c43fb

Move Contributing from wiki to CONTRIBUTING.md

7331227

Update readme, Rename Contributors->Authors

4c23874

SlashScreen force-pushed the real_t branch from a090dc2 to 4c23874 Compare October 22, 2023 01:41

TokisanGames merged commit 5b69f3a into TokisanGames:main Oct 22, 2023

TokisanGames mentioned this pull request Oct 22, 2023

Support doubles #30

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change c++ code to work with doubles #232

Change c++ code to work with doubles #232

SlashScreen commented Oct 19, 2023

SlashScreen commented Oct 19, 2023

TokisanGames commented Oct 19, 2023 •

edited

Loading

SlashScreen commented Oct 19, 2023

SlashScreen commented Oct 19, 2023

TokisanGames Oct 20, 2023

SlashScreen Oct 20, 2023

SlashScreen commented Oct 21, 2023

TokisanGames Oct 21, 2023

TokisanGames Oct 21, 2023

TokisanGames Oct 21, 2023

TokisanGames Oct 21, 2023 •

edited

Loading

SlashScreen commented Oct 21, 2023

TokisanGames Oct 21, 2023

SlashScreen Oct 21, 2023

TokisanGames Oct 21, 2023

TokisanGames commented Oct 21, 2023

SlashScreen commented Oct 21, 2023

TokisanGames commented Oct 21, 2023

TokisanGames commented Oct 22, 2023

SlashScreen commented Oct 22, 2023 via email

Change c++ code to work with doubles #232

Change c++ code to work with doubles #232

Conversation

SlashScreen commented Oct 19, 2023

SlashScreen commented Oct 19, 2023

TokisanGames commented Oct 19, 2023 • edited Loading

SlashScreen commented Oct 19, 2023

SlashScreen commented Oct 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SlashScreen commented Oct 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TokisanGames Oct 21, 2023 • edited Loading

Choose a reason for hiding this comment

SlashScreen commented Oct 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TokisanGames commented Oct 21, 2023

SlashScreen commented Oct 21, 2023

TokisanGames commented Oct 21, 2023

TokisanGames commented Oct 22, 2023

SlashScreen commented Oct 22, 2023 via email

TokisanGames commented Oct 19, 2023 •

edited

Loading

TokisanGames Oct 21, 2023 •

edited

Loading