Skip to content

Add a double-precision editor build to CI#55572

Merged
akien-mga merged 1 commit into
godotengine:masterfrom
aaronfranke:ci-double
Dec 10, 2021
Merged

Add a double-precision editor build to CI#55572
akien-mga merged 1 commit into
godotengine:masterfrom
aaronfranke:ci-double

Conversation

@aaronfranke

Copy link
Copy Markdown
Member

This PR fixes enough of Godot's code so that it compiles with float=64 again and adds a double-precision editor build to the CI. Some of the code changes are for the OpenGL support, some are in the text server, and some are in GLTF. Once this is in CI, this will check float=64 for every commit so that it will keep compiling.

@fire fire left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is required to keep float=64 from breaking.

@fire

fire commented Dec 3, 2021

Copy link
Copy Markdown
Member

A common request is to combine it with one of the existing cicd builds, but will wait for that request.

@fire

fire commented Dec 7, 2021

Copy link
Copy Markdown
Member

<Akien> Adds another build to CI, which will make CI slower. I haven't assessed in detail yet.

I think we need to keep the number of builds the same.

@Calinou

Calinou commented Dec 7, 2021

Copy link
Copy Markdown
Member

For nonstandard builds, I would probably look at moving them to a separate CI workflow that run according to a weekly schedule. This way, we don't have to worry about stealing CI time from other commits and pull requests. We can also do this to provide fully optimized builds for various platforms 🙂

@fire

fire commented Dec 8, 2021

Copy link
Copy Markdown
Member

The point is to prevent merges that fail this test and have the contributors attend to the breakage. Having them be non fatal would defeat this goal.

@bruvzg bruvzg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

TextServer still have a few more real_t variables used, it won't break a build, but should be eventually changed to avoid unnecessary conversions.

@akien-mga

Copy link
Copy Markdown
Member

I would prefer enabling double support in the existing linux + sanitizers build instead of duplicating it.

@aaronfranke

aaronfranke commented Dec 9, 2021

Copy link
Copy Markdown
Member Author

@akien-mga Done, it's now modifying that build instrad of adding a new one.

Comment thread scene/resources/font.cpp Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see here you're changing float to real_t for the width parameter, but in TextServer it's changed the other way around. How do you decide? (In both cases it's small numbers a very high precision pixel values don't really make sense so float would be good enough precision wise.)

@aaronfranke aaronfranke Dec 9, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I was going for the minimum changes to make it compile, but it's better to just make this float.

Comment thread scene/resources/importer_mesh.cpp Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should the 1.0f be changed?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It doesn't need to, it has the same behavior as (real_t)1.0 or similar. float - double will implicitly make the float become double, and 1.0f is represented exactly.

Comment thread scene/3d/cpu_particles_3d.cpp Outdated
@akien-mga akien-mga merged commit f455660 into godotengine:master Dec 10, 2021
@akien-mga

Copy link
Copy Markdown
Member

Thanks!

@HeadClot

Copy link
Copy Markdown

Bit of a question about this PR. Can we still use float with GDscript or do we have to use double? Either way very happy to see this merged :)

@Calinou

Calinou commented Dec 10, 2021

Copy link
Copy Markdown
Member

Scalar floats in GDScript are always double-precision, but packed arrays can store either 32-bit or 64-bit floats in Godot 4.0 (PackedFloat32Array vs PackedFloat64Array). In Godot 3.x, PoolRealArray always stores 32-bit floats. The same thing applies to packed/pooled integer arrays.

@HeadClot

Copy link
Copy Markdown

Scalar floats in GDScript are always double-precision, but packed arrays can store either 32-bit or 64-bit floats in Godot 4.0 (PackedFloat32Array vs PackedFloat64Array). In Godot 3.x, PoolRealArray always stores 32-bit floats. The same thing applies to packed/pooled integer arrays.

Ok good to know. Thank you for the very insightful reply :)

@aaronfranke aaronfranke deleted the ci-double branch December 10, 2021 21:04
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.

6 participants