Fix crash on Android devices supporting S3TC

[lawnjelly]

Fixes #28308

Android devices that support S3TC will currently crash if you export / run from the IDE a project that has S3TC textures imported. This is because these are not exported in the APK (android exporter, exporter.cpp, get_preset_features()), but the .import file still contains a reference to the S3TC files, and the app will attempt to load the resource and fail.

This PR fixes this by simply faking lack of hardware support for S3TC on android devices.

An alternative method maybe to remove the reference to the s3tc in the .import file.

[lawnjelly]

Note there may be a more elegant way of solving this, but I've been using this fix for the past few months. See the issue quoted above for details.

[akien-mga]

That's hacky, the proper fix would be to prevent trying to load S3TC files, or remove references to them when exporting for a platform which doesn't use them.

[lawnjelly]

That's hacky, the proper fix would be to prevent trying to load S3TC files, or remove references to them when exporting for a platform which doesn't use them.

Yep very true and I mentioned this in the original issue, I'm really just being cheeky and using the (hacky) PR to try and stimulate some direction on the best way to address this from some of the more core guys, maybe in one of the PR meetings (otherwise I'm afraid it will languish in issue purgatory, and it does and will cause crashes on affected hardware with this and similar bugs, because it might be a bit of a general oversight in the export / loading logic).

I think the removing references at export would be ideal but a little bug prone (being a text file), and finding the loading bit was a bit impenetrable when I looked at the time, but I'll have another look.

If this is bad form I can close this and try and get some advice on the irc channel. 👍

[akien-mga]

I'm really just being cheeky and using the (hacky) PR to try and stimulate some direction on the best way to address this from some of the more core guys, maybe in one of the PR meetings (otherwise I'm afraid it will languish in issue purgatory

Hehe that's a proven method indeed, a better PR is often better to build momentum around an issue than just commenting on one issue out of 5000. :)

[m4gr3d]

Agree it's not ideal, but should it be considered as a temp fix for 3.2?

I lack expertise in the export and loading logic so it's possible that fixing these instead may be simple and quick, but if not, this fix may do.

[m4gr3d]

The original issue referenced a quick fix for iOS and javascript as well. Are those no longer an issue?

[lawnjelly]

Sorry I've been away from internet (and will be until at least tomorrow afternoon I expect). Yup, I did notice the potential for the same issue to occur on iOS and javascript, although I can't confirm or not whether those do occur in practice (as iOS is quite fixed hardware, and I haven't been able to get an HTML5 export working for quite some time!).

The quick fix here does work for this specific case, and according to some hardware stats I looked at, it might affect up to 1/5 of android devices (those that support S3TC, and they would probably also crash with this). It is possible to workaround by unticking S3TC in the compressed texures bit of project settings, and reimporting textures before making the final export for android. However this is easy to forget, and not at all obvious. So it is potentially quite an easy way of expanding the compatibility with Android devices for 3.2.

However I am in agreement that it is a bit hacky and is potentially the symptom of a larger problem whereby the features in the export are not taken into account at runtime, it's something that we (or probably me lol) need to ask reduz (is it likely to be a one off bug, or a systemic problem) either in PR meeting or on IRC.

[akien-mga]

For Javascript it's more complex than that, as it does use S3TC on desktop browsers, but likely something else on mobile browsers (didn't check what exactly).

[lawnjelly]

Rather than modifying the import files, it might be easier to use the approach of saving which features have successfully been exported (maybe this is done already? A long time since I looked at this), loading this feature set in at runtime and perhaps converting to bitflags, then checking the bitflags before attempting to load a resource. This would solve the problem in a generic way as it could potentially occur in several areas.

Or we could simply log a warning message when failing to load a resource, and go for the next one in the list, rather than crashing... 😄

I'll try and have another look at this problem after the weekend.

[aaronfranke]

@lawnjelly Is this still desired? If so, it needs to be rebased on the latest master branch and made to work for Vulkan.

[lawnjelly]

@lawnjelly Is this still desired? If so, it needs to be rebased on the latest master branch and made to work for Vulkan.

Yes, we still need a fix for this, as it will be causing crashes in devices. We really need a discussion of this problem in PR meeting (as it may be a common issue in the export process).

[TIBI4]

A workaround as I'm working on 3.2.3 stable official:

In the editor, go to Project -> Export... -> Your Android Preset To Use -> Features -> Custom, and then add "s3tx" to the custom list (this worked for me).

The project should start normally. But I have a question, as I don't know what is happening there though. Is this a bad idea?

[lawnjelly]

That's an interesting approach. The downside is that if it is including the s3tc files, the apk is bigger than it could be (as presumably you are also including ETC for the majority of devices that don't support s3tc).

I haven't tested this, but there may be another possible workaround:

• If you go to the project_settings->rendering->vram_compression page, and click to select import_s3tc, in the top right of the dialog you can add an override for android, and turn off import_s3tc for android only.

I've no idea whether this would work, not having time to test at the moment.

[akien-mga]

After thinking some more about it, I think the approach taken in this PR might actually be good.

In theory those Android devices could use and benefit from having S3TC, which would imply register s3tc as a supported feature for Android as discussed above. But the main issue is that many devices do not support S3TC, and it becomes tricky to define what APK to provide to users of each type of device.

So as long as all Android devices support ETC, it's likely best to intentionally ignore S3TC support on Android and mandate the use of ETC.

Edit: ETC, not ETC2 as we're talking about GLES2. Though I guess the same issue might happen with GLES3 too?

[akien-mga]

• If you go to the project_settings->rendering->vram_compression page, and click to select import_s3tc, in the top right of the dialog you can add an override for android, and turn off import_s3tc for android only.

I don't think that would work, it would disable importing S3TC when running the editor on an Android device, but the import would still happen when you develop on PC.

[DmitriySalnikov]

Unfortunately, this problem applies to the new Windows Subsystem for Android. Therefore, to test apps with VRAM textures, it is necessary to add the s3tc feature.
I tested it in tps-demo and in platformer-3d in GLES3 mode (although here the fix is only for GLES2).
The tps-demo takes a very long time to load, shows 3-30 FPS, and the size of the game has increased almost 2 times, but it works.

What about giving developers the option to add etc2, s3tc, pvrtc to the archive at the same time, but in AAB separate these textures for target devices?

Screenshots

[Calinou]

What about giving developers the option to add etc2, s3tc, pvrtc to the archive at the same time, but in AAB separate these textures for target devices?

As I understand it, PVRTC compression isn't really used much anymore due to its low quality. It also only works on PowerVR GPUs (which in practice restricts it to iOS devices).

[DmitriySalnikov]

@Calinou but there are still many devices with Adreno graphics and ATC compression. There is also an ASTC.
But at the moment there is only s3tc, etc, etc2 in Godot.
To be honest, I don't mind if etc2 is forced.

[Gromph]

We had a user running the Android version of our game on an intel chromebook report this crash. This Pull Request fixed it. Thank you.

[akien-mga]

Thanks! Took a while to assess and come to the conclusion that we don't have the time/will to do this better for now :D

[akien-mga]

Cherry-picked for 3.4.3.