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

Turning on doubles via scons precision=double will change the docs #71416

Open
fire opened this issue Jan 14, 2023 · 7 comments
Open

Turning on doubles via scons precision=double will change the docs #71416

fire opened this issue Jan 14, 2023 · 7 comments

Comments

@fire
Copy link
Member

fire commented Jan 14, 2023

Godot version

60d0317

System information

Windows 11, Nvidia 3000

Issue description

Turning on doubles will change the doctool behaviour.

Problems:

  1. The API should not change. Means you can't use the same API dump for multiple targets
  2. Add a CI check for this, we already need to compile twice

Steps to reproduce

  1. build godot with scons precsion=double
  2. run godot with --doctool
  3. check git diff

Minimal reproduction project

N/A

@fire fire changed the title Turning on doubles via scons precision=double will change the doctool Turning on doubles via scons precision=double will change the docs Jan 14, 2023
@akien-mga
Copy link
Member

Can you attach the full diff?

@akien-mga akien-mga added this to the 4.0 milestone Jan 14, 2023
@fire
Copy link
Member Author

fire commented Jan 15, 2023

Sure. I'm not at a ready state so I'll get it in a few days.

@akien-mga
Copy link
Member

Here's the diff (from #71575):

diff --git a/doc/classes/Curve3D.xml b/doc/classes/Curve3D.xml
index 362d792..96311c7 100644
--- a/doc/classes/Curve3D.xml
+++ b/doc/classes/Curve3D.xml
@@ -40,7 +40,7 @@
 			</description>
 		</method>
 		<method name="get_baked_tilts" qualifiers="const">
-			<return type="PackedFloat32Array" />
+			<return type="PackedFloat64Array" />
 			<description>
 				Returns the cache of tilts as a [PackedFloat32Array].
 			</description>
diff --git a/doc/classes/HeightMapShape3D.xml b/doc/classes/HeightMapShape3D.xml
index 206981e..0243bb0 100644
--- a/doc/classes/HeightMapShape3D.xml
+++ b/doc/classes/HeightMapShape3D.xml
@@ -10,7 +10,7 @@
 	<tutorials>
 	</tutorials>
 	<members>
-		<member name="map_data" type="PackedFloat32Array" setter="set_map_data" getter="get_map_data" default="PackedFloat32Array(0, 0, 0, 0)">
+		<member name="map_data" type="PackedFloat64Array" setter="set_map_data" getter="get_map_data" default="PackedFloat64Array(0, 0, 0, 0)">
 			Height map data, pool array must be of [member map_width] * [member map_depth] size.
 		</member>
 		<member name="map_depth" type="int" setter="set_map_depth" getter="get_map_depth" default="2">
diff --git a/doc/classes/PhysicsDirectSpaceState2D.xml b/doc/classes/PhysicsDirectSpaceState2D.xml
index d4cb073..10b43cc 100644
--- a/doc/classes/PhysicsDirectSpaceState2D.xml
+++ b/doc/classes/PhysicsDirectSpaceState2D.xml
@@ -12,7 +12,7 @@
 	</tutorials>
 	<methods>
 		<method name="cast_motion">
-			<return type="PackedFloat32Array" />
+			<return type="PackedFloat64Array" />
 			<param index="0" name="parameters" type="PhysicsShapeQueryParameters2D" />
 			<description>
 				Checks how far a [Shape2D] can move without colliding. All the parameters for the query, including the shape and the motion, are supplied through a [PhysicsShapeQueryParameters2D] object.
diff --git a/doc/classes/PhysicsDirectSpaceState2DExtension.xml b/doc/classes/PhysicsDirectSpaceState2DExtension.xml
index fbbb98a..8f19f4c 100644
--- a/doc/classes/PhysicsDirectSpaceState2DExtension.xml
+++ b/doc/classes/PhysicsDirectSpaceState2DExtension.xml
@@ -16,8 +16,8 @@
 			<param index="4" name="collision_mask" type="int" />
 			<param index="5" name="collide_with_bodies" type="bool" />
 			<param index="6" name="collide_with_areas" type="bool" />
-			<param index="7" name="closest_safe" type="float*" />
-			<param index="8" name="closest_unsafe" type="float*" />
+			<param index="7" name="closest_safe" type="double*" />
+			<param index="8" name="closest_unsafe" type="double*" />
 			<description>
 			</description>
 		</method>
diff --git a/doc/classes/PhysicsDirectSpaceState3D.xml b/doc/classes/PhysicsDirectSpaceState3D.xml
index cc1cf8a..78829e2 100644
--- a/doc/classes/PhysicsDirectSpaceState3D.xml
+++ b/doc/classes/PhysicsDirectSpaceState3D.xml
@@ -12,7 +12,7 @@
 	</tutorials>
 	<methods>
 		<method name="cast_motion">
-			<return type="PackedFloat32Array" />
+			<return type="PackedFloat64Array" />
 			<param index="0" name="parameters" type="PhysicsShapeQueryParameters3D" />
 			<description>
 				Checks how far a [Shape3D] can move without colliding. All the parameters for the query, including the shape, are supplied through a [PhysicsShapeQueryParameters3D] object.
diff --git a/doc/classes/PhysicsDirectSpaceState3DExtension.xml b/doc/classes/PhysicsDirectSpaceState3DExtension.xml
index 4297846..036c5cd 100644
--- a/doc/classes/PhysicsDirectSpaceState3DExtension.xml
+++ b/doc/classes/PhysicsDirectSpaceState3DExtension.xml
@@ -16,8 +16,8 @@
 			<param index="4" name="collision_mask" type="int" />
 			<param index="5" name="collide_with_bodies" type="bool" />
 			<param index="6" name="collide_with_areas" type="bool" />
-			<param index="7" name="closest_safe" type="float*" />
-			<param index="8" name="closest_unsafe" type="float*" />
+			<param index="7" name="closest_safe" type="double*" />
+			<param index="8" name="closest_unsafe" type="double*" />
 			<param index="9" name="info" type="PhysicsServer3DExtensionShapeRestInfo*" />
 			<description>
 			</description>

@akien-mga akien-mga modified the milestones: 4.0, 4.1 Feb 17, 2023
@akien-mga
Copy link
Member

Fixing this will require breaking compatibility, but it's a bit late to do for 4.0 anyway, so lets work on that for 4.1.

@dsnopek
Copy link
Contributor

dsnopek commented Jun 19, 2023

I did a little bit of research and this one is trickier than I initially thought.

The switch to using real_t in both those files was on purpose, in these two PRs (both by @aaronfranke):

I didn't look as closely at the physics one, but looking at Curve3D, it does make sense for it to use double for its own math if Vector3 is using double (as it does with precision=double).

So, if we decide that exposed APIs changing between float and double is a bad thing we want to avoid... Does that mean these should always use double and PackedFloat64Array, wasting some space in precision=single? Or, that we should convert to float and PackedFloat32Array when exposed to the API, losing some precision when using precision=double?

Or, is the API changing when doing precision=double just part of the cost of having this as an option?

@aaronfranke
Copy link
Member

I don't see it as a big deal that the docs and APIs change between precision levels, I think it's fully expected that the different precision levels are not compatible.

However, if we did pick one single size for exposing something, it should be double, not single. We already have the precedent that GDScript Variant float and Array[float] are always double precision.

@aaronfranke aaronfranke removed this from the 4.1 milestone Jun 19, 2023
@fire
Copy link
Member Author

fire commented Jun 19, 2023

I also support picking the widened type.

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

No branches or pull requests

4 participants