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

Rename float=64 build option to precision=double #988

Merged
merged 1 commit into from
Jan 10, 2023

Conversation

rburing
Copy link
Member

@rburing rburing commented Jan 9, 2023

This makes the build system consistent with Godot again; see godotengine/godot#67399.

TODO:

  • Update CMake build file.

Also fixes the CMake build to define REAL_T_IS_DOUBLE when needed.

@rburing rburing requested review from a team as code owners January 9, 2023 10:08
@@ -151,7 +151,7 @@ def scons_generate_bindings(target, source, env):
str(source[0]),
env["generate_template_get_node"],
"32" if "32" in env["arch"] else "64",
"double" if (env["float"] == "64") else "float",
"double" if env["precision"] == "double" else "float",
Copy link
Member

Choose a reason for hiding this comment

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

This begs for maybe refactoring further and changing the values from single|double to float|double to match what is used in the GDExtension config:

gdextension/extension_api.json
12:                     "build_configuration": "float_32",
173:                    "build_configuration": "float_64",
334:                    "build_configuration": "double_32",
495:                    "build_configuration": "double_64",
658:                    "build_configuration": "float_32",
973:                    "build_configuration": "float_64",
1288:                   "build_configuration": "double_32",
1603:                   "build_configuration": "double_64",

Would need to be done upstream too and break compat slightly again, but might be worth it?

@akien-mga
Copy link
Member

CMake changes (untested), provided we'd do what I suggested above to change the default from single to float to match what should be passed to the bindings generator:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index f3cf4fb..128d4a3 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -4,7 +4,7 @@
 # godot-cpp cmake arguments
 # GODOT_GDEXTENSION_DIR:	Path to the directory containing GDExtension interface header and API JSON file
 # GODOT_CUSTOM_API_FILE:	Path to a custom GDExtension API JSON file (takes precedence over `gdextension_dir`)
-# FLOAT_TYPE				Floating-point precision (32, 64)
+# FLOAT_PRECISION			Floating-point precision (float, double)
 #
 # Android cmake arguments
 # CMAKE_TOOLCHAIN_FILE:		The path to the android cmake toolchain ($ANDROID_NDK/build/cmake/android.toolchain.cmake)
@@ -45,11 +45,6 @@ if("${CMAKE_BUILD_TYPE}" STREQUAL "")
 	set(CMAKE_BUILD_TYPE Debug)
 endif()
 
-set(FLOAT_TYPE_FLAG "float" CACHE STRING "")
-if(FLOAT_TYPE EQUAL 64)
-	set(FLOAT_TYPE_FLAG "double" CACHE STRING "")
-endif(FLOAT_TYPE EQUAL 64)
-
 if(NOT DEFINED BITS)
 	set(BITS 32)
 	if(CMAKE_SIZEOF_VOID_P EQUAL 8)
@@ -60,6 +55,7 @@ endif()
 # Input from user for GDExtension interface header and the API JSON file
 set(GODOT_GDEXTENSION_DIR "gdextension" CACHE STRING "")
 set(GODOT_CUSTOM_API_FILE "" CACHE STRING "")
+set(FLOAT_PRECISION "float" CACHE STRING "")
 
 set(GODOT_GDEXTENSION_API_FILE "${GODOT_GDEXTENSION_DIR}/extension_api.json")
 if (NOT "${GODOT_CUSTOM_API_FILE}" STREQUAL "")  # User-defined override.
@@ -136,7 +132,7 @@ execute_process(COMMAND "${Python3_EXECUTABLE}" "-c" "import binding_generator;
 )
 
 add_custom_command(OUTPUT ${GENERATED_FILES_LIST}
-		COMMAND "${Python3_EXECUTABLE}" "-c" "import binding_generator; binding_generator.generate_bindings(\"${GODOT_GDEXTENSION_API_FILE}\", \"${GENERATE_BINDING_PARAMETERS}\", \"${BITS}\", \"${FLOAT_TYPE_FLAG}\", \"${CMAKE_CURRENT_BINARY_DIR}\")"
+		COMMAND "${Python3_EXECUTABLE}" "-c" "import binding_generator; binding_generator.generate_bindings(\"${GODOT_GDEXTENSION_API_FILE}\", \"${GENERATE_BINDING_PARAMETERS}\", \"${BITS}\", \"${FLOAT_PRECISION}\", \"${CMAKE_CURRENT_BINARY_DIR}\")"
 		VERBATIM
 		WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
 		MAIN_DEPENDENCY ${GODOT_GDEXTENSION_API_FILE}

Otherwise if we stick to single, it needs something like FLOAT_TYPE_FLAG re-added to do the extra conversion of single to float before passing to binding_generator.

@akien-mga akien-mga added enhancement This is an enhancement on the current functionality breaks compat topic:buildsystem Related to the buildsystem or CI setup labels Jan 9, 2023
@rburing rburing force-pushed the precision=double branch 2 times, most recently from fc8a780 to 0bab6f9 Compare January 9, 2023 11:07
@rburing
Copy link
Member Author

rburing commented Jan 9, 2023

I updated all the user-facing scripts to match the (current) Godot way of specifying precision=double. Internally we indeed still have build_configuration formatted differently. I would prefer to leave any further changes to a different PR. (Personally I prefer single|double to float|double.)

By the way, the CMake file never defines REAL_T_IS_DOUBLE (and I don't know how to do it), so I doubt it ever worked.

@akien-mga
Copy link
Member

akien-mga commented Jan 9, 2023

Fair enough, then further diff for CMake which should add REAL_T_IS_DOUBLE and handle the single->float conversion:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index dcf7475..2511a7e 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -52,10 +52,17 @@ if(NOT DEFINED BITS)
 	endif(CMAKE_SIZEOF_VOID_P EQUAL 8)
 endif()
 
+set(FLOAT_PRECISION "single" CACHE STRING "")
+# "single" needs to be "float" for the binding generator.
+set(FLOAT_PRECISION_FLAG "float")
+if ("${FLOAT_PRECISION}" STREQUAL "double")
+	set(FLOAT_PRECISION_FLAG "double")
+	add_definitions(-DREAL_T_IS_DOUBLE)
+endif()
+
 # Input from user for GDExtension interface header and the API JSON file
 set(GODOT_GDEXTENSION_DIR "gdextension" CACHE STRING "")
 set(GODOT_CUSTOM_API_FILE "" CACHE STRING "")
-set(FLOAT_PRECISION "single" CACHE STRING "")
 
 set(GODOT_GDEXTENSION_API_FILE "${GODOT_GDEXTENSION_DIR}/extension_api.json")
 if (NOT "${GODOT_CUSTOM_API_FILE}" STREQUAL "")  # User-defined override.
@@ -132,7 +139,7 @@ execute_process(COMMAND "${Python3_EXECUTABLE}" "-c" "import binding_generator;
 )
 
 add_custom_command(OUTPUT ${GENERATED_FILES_LIST}
-		COMMAND "${Python3_EXECUTABLE}" "-c" "import binding_generator; binding_generator.generate_bindings(\"${GODOT_GDEXTENSION_API_FILE}\", \"${GENERATE_BINDING_PARAMETERS}\", \"${BITS}\", \"${FLOAT_PRECISION}\", \"${CMAKE_CURRENT_BINARY_DIR}\")"
+		COMMAND "${Python3_EXECUTABLE}" "-c" "import binding_generator; binding_generator.generate_bindings(\"${GODOT_GDEXTENSION_API_FILE}\", \"${GENERATE_BINDING_PARAMETERS}\", \"${BITS}\", \"${FLOAT_PRECISION_FLAG}\", \"${CMAKE_CURRENT_BINARY_DIR}\")"
 		VERBATIM
 		WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
 		MAIN_DEPENDENCY ${GODOT_GDEXTENSION_API_FILE}

@akien-mga
Copy link
Member

Seems like your last push doesn't include all changes from my second diff. The FLOAT_PRECISION_FLAG intermediate variable is needed to pass float instead of single to the python script.

@rburing
Copy link
Member Author

rburing commented Jan 9, 2023

I changed generate_bindings to take precision instead (earlier). We can do it the other way if it is preferable.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Ah that's great then 👍

This makes the build system consistent with Godot again.
Also fix CMake build to define REAL_T_IS_DOUBLE when precision=double.
@akien-mga akien-mga merged commit 2b7094f into godotengine:master Jan 10, 2023
@akien-mga
Copy link
Member

Thanks!

@rburing rburing deleted the precision=double branch January 10, 2023 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaks compat enhancement This is an enhancement on the current functionality topic:buildsystem Related to the buildsystem or CI setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants