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

Validate texture format selection when exporting to Windows or Linux #84913

Closed
wants to merge 1 commit into from

Conversation

clayjohn
Copy link
Member

@clayjohn clayjohn commented Nov 14, 2023

Fixes: #81234
Closes: godotengine/godot-proposals#7230

In #73829 we made s3tc + bptc the default export options. But we exposed users to the risk that, if they disable one or the other, their games would not load.

In Godot 4.x, it is not possible to use S3TC without BPTC (or vice versa). Nor is it possible to use ETC2 without ASTC (or vice versa).

This change validates that S3TC and BPTC have the same value (either both on or both off) and either S3TC/BPTC or ETC2/ASTC are used at export time.

Finally it updates the docs for the settings and removes "etc" from the feature list (as ETC textures are no longer used by the engine).

@clayjohn clayjohn added bug topic:export cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Nov 14, 2023
@clayjohn clayjohn added this to the 4.3 milestone Nov 14, 2023
@clayjohn clayjohn requested review from a team as code owners November 14, 2023 21:28

if (uses_s3tc != uses_bptc) {
valid = false;
err += TTR("If using either s3tc or bptc texture format, you must use both s3tc and bptc.");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
err += TTR("If using either s3tc or bptc texture format, you must use both s3tc and bptc.");
err += TTR("If using either S3TC or BPTC texture format, you must enable both.");

@akien-mga
Copy link
Member

Seems a bit pointless to me to have texture_format/bptc and texture_format/s3tc as separate options if they can't be used separately.

Maybe we should unify them as texture_format/bptc_s3tc, and likewise rename the etc2 one to texture_format/etc2_astc, matching what we have for the project settings.

The compatibility with the old preset properties could likely be handled with _set/_get methods.

@clayjohn
Copy link
Member Author

ould likely be handled with _set/_get methods.

That sounds fine to me. I didn't realize we had the ability to use _set/_get methods inside the editor_export_* classes. I only left it like this to preserve compatibility

@akien-mga
Copy link
Member

I didn't realize we had the ability to use _set/_get methods inside the editor_export_* classes.

I haven't checked if that would actually work, to be clear. But it's worth testing.

@clayjohn
Copy link
Member Author

I didn't realize we had the ability to use _set/_get methods inside the editor_export_* classes.

I haven't checked if that would actually work, to be clear. But it's worth testing.

Absolutely. I'll take a look and see what I can do

@@ -58,16 +58,16 @@
- [code]{cmd_args}[/code] - Array of the command line argument for the application.
</member>
<member name="texture_format/bptc" type="bool" setter="" getter="">
If [code]true[/code], project textures are exported in the BPTC format.
If [code]true[/code], project textures are exported in the BPTC format. If used, [member texture_format/s3tc] must be used as well.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If [code]true[/code], project textures are exported in the BPTC format. If used, [member texture_format/s3tc] must be used as well.
If [code]true[/code], project textures are exported in the BPTC format. If enabled, [member texture_format/s3tc] must be enabled as well.

Same for all these

@akien-mga
Copy link
Member

Seems a bit pointless to me to have texture_format/bptc and texture_format/s3tc as separate options if they can't be used separately.

Maybe we should unify them as texture_format/bptc_s3tc, and likewise rename the etc2 one to texture_format/etc2_astc, matching what we have for the project settings.

I've done that in #88325, please review :)

@clayjohn
Copy link
Member Author

Superseded by #88325

@clayjohn clayjohn closed this Feb 16, 2024
@clayjohn clayjohn removed this from the 4.3 milestone Feb 16, 2024
@AThousandShips AThousandShips removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Mar 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants