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

Add feature tags to signify engine float precision #69538

Merged
merged 1 commit into from
Dec 11, 2022

Conversation

mihe
Copy link
Contributor

@mihe mihe commented Dec 3, 2022

This adds double (edit: and single) as a "feature tag" to OS::has_feature to signify whether an engine was built with double-precision, also known as float=64 or REAL_T_IS_DOUBLE.

This allows, for example, the engine to load different extension libraries (see here) depending on if the engine was built with float=32 or float=64, allowing extension authors to bundle libraries for both precision types.

Without this ability to differentiate between float=32 and float=64 the engine will crash when loading an extension with a mismatching precision type.

@mihe mihe requested a review from a team as a code owner December 3, 2022 17:30
@mihe mihe force-pushed the feature-tag-double branch 2 times, most recently from b1624c5 to 778f78f Compare December 3, 2022 17:46
@mihe
Copy link
Contributor Author

mihe commented Dec 3, 2022

It's worth mentioning that this PR will be further complemented by #67906, which will make the engine prioritize the more specific entry in a .gdextension file, for cases where there are multiple matching entries, like so:

[libraries]
linux.x86_64.double = "res://my-extension-double-precision-x64.so"
linux.x86_64 = "res://my-extension-single-precision-x64.so"

It's also worth mentioning that this PR does not address the case where a float=64 engine tries to load an extension which does not explicitly specify a double library, like so:

[libraries]
linux.x86_64 = "res://my-extension-single-precision-x64.so"

... which would match both float=32 and float=64, causing the engine to load it either way and subsequently crash.

This could be partially addressed by also adding a single feature tag, or however you want to name it, to signify single-precision (float=32), allowing extension authors to be specific about that condition as well, like so:

[libraries]
linux.x86_64.single = "res://my-extension-single-precision-x64.so"

Let me know if you think something like single should be added to this PR as well.

@JoNax97
Copy link
Contributor

JoNax97 commented Dec 3, 2022

I think double_precision or float_64 would be clearer names

@mihe mihe changed the title Add double as a feature tag to signify float=64 Add feature tag to signify float=64 Dec 3, 2022
@mihe
Copy link
Contributor Author

mihe commented Dec 3, 2022

Fair point! Something like float_64 would make the naming of the single-precision equivalent a bit less weird as well, with float_32. I'll go ahead and change that right away.

@mihe mihe changed the title Add feature tag to signify float=64 Add feature tag to signify engine float precision Dec 3, 2022
@mihe
Copy link
Contributor Author

mihe commented Dec 3, 2022

There, I've renamed double to float_64 and went ahead and added float_32 as well.

@Mickeon
Copy link
Contributor

Mickeon commented Dec 3, 2022

Hold on. #67399 has been approved (not merged as of the time writing this). So "double" may make more sense.

@mihe
Copy link
Contributor Author

mihe commented Dec 3, 2022

Ah, yes, thank you for linking that PR.

In that case I agree that something like double/single would make more sense.

I guess another thing to consider with regards to the naming is that the editor already adds .double as a suffix to its executable when you build it with double-precision, so there is some precedence there.

Also, if I understand it correctly, when #67906 ends up being merged, extensions will be able to use their library filenames as feature tags, without specifying anything in their .gdextension file. So having my-extension.double.x86_64.dll (which lines up with the editor executable) would check for the features double and x86_64.

I'll leave it unchanged for now, for further discussion.

@mihe
Copy link
Contributor Author

mihe commented Dec 5, 2022

Considering the 4.x milestone this was just given I went ahead and changed the names back to the more likely double/single.

@mihe mihe changed the title Add feature tag to signify engine float precision Add feature tags to signify engine float precision Dec 5, 2022
Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

Having feature tags like this is a good idea, and the specific names proposed here are very good names.

@mihe
Copy link
Contributor Author

mihe commented Dec 10, 2022

Any chance of having this merged for 4.0?

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Looks good to me. After this is merged, remember to update the Feature tags documentation as well (you can open a PR as draft right away).

@mihe
Copy link
Contributor Author

mihe commented Dec 10, 2022

you can open a PR as draft right away

Done: godotengine/godot-docs#6476

@akien-mga akien-mga modified the milestones: 4.x, 4.0 Dec 11, 2022
@akien-mga akien-mga merged commit e7d8921 into godotengine:master Dec 11, 2022
@akien-mga
Copy link
Member

Thanks!

@mihe mihe deleted the feature-tag-double branch December 11, 2022 10:55
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.

7 participants