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

Godot attempting to load BPTC textures on Android #74451

Closed
Malcolmnixon opened this issue Mar 5, 2023 · 5 comments · Fixed by #74684
Closed

Godot attempting to load BPTC textures on Android #74451

Malcolmnixon opened this issue Mar 5, 2023 · 5 comments · Fixed by #74684

Comments

@Malcolmnixon
Copy link
Contributor

Godot version

Godot 4.0 Stable

System information

Windows 11, Vulkan Mobile, NVidia RTX 3070 TI - targeting Quest 2

Issue description

When running the Godot-XR-Tools demo project natively on the Quest, the main scene fails to load because the resource loader tries to load the BPTC version of the texture, even though the ASTC version was bundled into the APK.

Unable to open file: res://.godot/imported/SkyOnlyHDRI023_2K-TONEMAPPED.jpg-bcb5464e5ed42ee2463b71652f907f38.bptc.ctex.

Steps to reproduce

Build the latest godot-xr-tools demo project (https://github.com/GodotVR/godot-xr-tools) targeting the Quest.

The project has both S3TC/BPTC and ETC2/ASTC texture compression formats enabled.

The .import files mention both the S3TC/BPTC and ETC2/ASTC formats; however when exporting for Quest 2 (an Android target) Godot automatically enables the astc and etc2 features and bundles only the ASTC and ETC2 textures into the APK.

At startup the resource importer reads the .import files, sees S3TC/BPTC as the first enabled texture, sees that S3TC/BPTC is supported on the platform, and picks that format - totally ignoring what was actually bundled into the installer.

Minimal reproduction project

Build the latest godot-xr-tools demo project (https://github.com/GodotVR/godot-xr-tools) targeting the Quest.

@Malcolmnixon
Copy link
Contributor Author

This seems related to a similar issue in July 2022 (#62909), and in that case a "lazy" fix was applied which just lied about the supported capabilities of the operating system to trick the resource loader into only loading the desired formats.

@clayjohn
Copy link
Member

clayjohn commented Mar 5, 2023

A few of us discussed this on Roacketchat and we think we know what the issue is and how to resolve it.

The Issue

At export time the exporter is removing the BPTC and S3TC files as it should. But it is not removing the reference to the S3TC and BPTC files from the import metadata. At load time, the engine checks the graphics driver to see if the format is supported, if it is, it will try to load it.

Since there is a mismatch between what we export and what the GPU supports it ends up trying to load a non-existent resource.

Accordingly, there are two parts to the problem:

  1. Export formats are too rigid for mobile (i.e. we force ETC2/ASTC at export time even though some Android devices support S3TC/BPTC)
  2. The import metadata isn't correctly being amended at export time (i.e. the references to BPTC/S3TC files are left behind)

In 3.x we solved this problem by forcibly disabling reporting S3TC from the OpenGL driver side (I don't know why we didn't run into issues with BPTC).

The solution

First we need to change the exporter settings. On PC we have checkboxes for BPTC, S3TC, ETC, and ASTC. They need to be changed to only have 2 checkboxes S3TC/BPTC and ETC2/ASTC. We need to add these checkboxes for Android and remove the hardcoded options for ETC2 and ASTC.

Second, we need to figure out why the import metadata maintains reference to BPTC at export time and change it to respect the export checkboxes added above.

@akien-mga
Copy link
Member

First we need to change the exporter settings. On PC we have checkboxes for BPTC, S3TC, ETC, and ASTC. They need to be changed to only have 2 checkboxes S3TC/BPTC and ETC2/ASTC. We need to add these checkboxes for Android and remove the hardcoded options for ETC2 and ASTC.

For the record, according to @reduz it sounds like we should instead remove all configuration options for PC, and just stick to:

  • S3TC/BPTC for PC (ETC2/ASTC would be a superior option, but is not supported by most PC GPUs so it's not usable in practice)
  • ETC2/ASTC for mobile (apparently superior in quality to S3TC/BPTC, so there's no reason to use the latter)

@Malcolmnixon
Copy link
Contributor Author

We still have the "lazy" fix code present for S3TC in the gles3 driver, and this could be expanded to cover BPTC:

#if defined(ANDROID_ENABLED) || defined(IOS_ENABLED)
// Some Android devices report support for S3TC but we don't expect that and don't export the textures.
// This could be fixed but so few devices support it that it doesn't seem useful (and makes bigger APKs).
// For good measure we do the same hack for iOS, just in case.
s3tc_supported = false;
#else
s3tc_supported = extensions.has("GL_EXT_texture_compression_dxt1") || extensions.has("GL_EXT_texture_compression_s3tc") || extensions.has("WEBGL_compressed_texture_s3tc");
#endif

It looks like we have the equivalent code on the Vulkan side for S3TC and could be expanded to cover BPTC:

#if !defined(ANDROID_ENABLED) && !defined(IOS_ENABLED)
// Some Android devices report support for S3TC but we don't expect that and don't export the textures.
// This could be fixed but so few devices support it that it doesn't seem useful (and makes bigger APKs).
// For good measure we do the same hack for iOS, just in case.
if (p_feature == "s3tc" && RD::get_singleton()->texture_is_format_supported_for_usage(RD::DATA_FORMAT_BC1_RGB_UNORM_BLOCK, RD::TEXTURE_USAGE_SAMPLING_BIT)) {
return true;
}
#endif
if (p_feature == "bptc" && RD::get_singleton()->texture_is_format_supported_for_usage(RD::DATA_FORMAT_BC7_UNORM_BLOCK, RD::TEXTURE_USAGE_SAMPLING_BIT)) {
return true;
}

@clayjohn
Copy link
Member

clayjohn commented Mar 9, 2023

I think I have a solution for this. Will post a PR soon. Reduz was right. The code that exports the .import files was just copying them over as-is thought regard to the file types that were being trimmed at export time. The solution is to edit the exported copy of the .import file to only include the texture types that are being exported.

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

Successfully merging a pull request may close this issue.

3 participants