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 single/double as export features automatically #84711

Merged
merged 1 commit into from
Apr 26, 2024

Conversation

mihe
Copy link
Contributor

@mihe mihe commented Nov 10, 2023

Related to #69538.

This adds either single or double to the list of export feature tags (to signify floating-point precision) based on whether the editor from which you're exporting is built with single or double precision.

This was arguably something that should've been part of #69538 to begin with, as not having this can pose a problem for extensions that use these feature tags as part of their .gdextension file, and potentially other things that rely on feature tags.

For example, an extension might add the single feature tag to all its [libraries] to make sure it doesn't get used/loaded in a double-precision build of Godot, or vice versa. In that scenario you would have to manually add either the single or double feature tag to the export options in order for the extension to be bundled (and not throw an error) when exporting a project using said extension.

With that said, this change is a little bit crude/simplistic, as it always adds these feature tags regardless of platform with no configurability, so at the risk of further complicating this PR, here are some questions/concerns:

  1. Should this instead be done in each respective EditorExportPlatform::get_platform_features? Are there (or could there be) platforms that don't support double-precision?
  2. ... or should this instead be done in each respective EditorExportPlatform::get_preset_features, with "Double Precision" being a configurable preset option? Does it even make sense to export a single-precision build from a double-precision editor and vice versa?

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.

I don't know what's best for the the precise details (like how to organize this code), but this looks good, and I am in favor of the feature. It's good to have export feature flags for this for GDExtension to read.

  1. ... Are there (or could there be) platforms that don't support double-precision?

Technically in RISC-V, floats are an optional feature. However even in that case both float and double are available in the ABI implemented in software on top of integer operations, so it's always available (but slow).

But anyway, even if platforms existed that did not support double, we should not officially support those. Even in single-precision mode, Godot uses double-precision in a ton of places. Without double support, the engine will not compile in either single or double precision mode.

  1. ... Does it even make sense to export a single-precision build from a double-precision editor and vice versa?

No, that would not be useful.

@dsnopek
Copy link
Contributor

dsnopek commented Nov 10, 2023

Hm. Let me make sure I'm understanding the situation correctly.

GDExtension will already load the correct GDExtension because of PR #69538. However, this PR is about setting feature tags on export, which are then used to determine which GDExtension libraries should be included in the export. Is that correct?

Assuming I'm understanding this correctly, the one bit that worries me is that it's using whether or not the editor was compiled in single or double precision mode in order to set feature flags on the export preset. What we really care about here is whether or not the export template that's being used is single or double precision, not the editor that's doing the exporting.

I could imagine someone using a single precision editor, but pointing the export preset to double precision export templates. This isn't ideal, since running the game during development would be running with different precision than the exported game, but I could still imagine someone doing it.

@mihe
Copy link
Contributor Author

mihe commented Nov 10, 2023

However, this PR is about setting feature tags on export, which are then used to determine which GDExtension libraries should be included in the export. Is that correct?

Correct, but I assume they have some sort of effect outside of just extension loading as well, given that there's a bunch of texture format stuff in there and whatnot, so I don't think it's strictly an extension thing.

What we really care about here is whether or not the export template that's being used is single or double precision, not the editor that's doing the exporting.

Yes, I did look around for a bit to see if there was any precedent for deriving some feature flag from the template used, but I wasn't able to find anything. That would certainly be the ideal solution though, if such a thing is possible.

This isn't ideal, since running the game during development would be running with different precision than the exported game, but I could still imagine someone doing it.

That would be the second concern mentioned in the PR description I guess. The seemingly only reasonable use-case I can think of there would be if someone were to make both single- and double-precision builds of their project, and wanted to use the same editor build (but different template builds) to export them both. It strikes me as a very niche use-case though.

One relatively easy (albeit messy) solution to that would be (as mentioned above) to move this to EditorExportPlatform::get_preset_features instead and add an export option to every export platform (similar to the "Custom Template" options) that lets you toggle with a checkbox whether or not you're exporting for a double-precision build or not, with perhaps its default value being derived from the editor's REAL_T_IS_DOUBLE.

@akien-mga
Copy link
Member

akien-mga commented Nov 10, 2023

double should probably be treated like mono and require a whole specific set of export templates, and prevent mixing them.

I.e. it would append to the module_config from version.py via env.add_module_version_string("double"), and the resulting Godot version string (also used for the export templates location) would be e.g. 4.2.beta6.double (and when using .NET 4.2.beta6.double.mono - not sure which would come first, depends on the buildsystem order).

@aaronfranke
Copy link
Member

While this could potentially be improved further, the PR also looks good as-is, so it would make sense to me to accept it.

@akien-mga akien-mga merged commit 285c917 into godotengine:master Apr 26, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

4 participants