godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
88.23k stars 19.98k forks source link

`EditorExportPlugin`: `skip()` can cause issues when customizing a `CompressedTexture2D` #94045

Open allenwp opened 1 month ago

allenwp commented 1 month ago

Tested versions

Commit 932c191 onwards

System information

Godot v4.3.beta (b97110cd3) - Windows 10.0.19045 - Vulkan (Forward+) - dedicated NVIDIA GeForce GTX 980 Ti (NVIDIA; 31.0.15.3699) - Intel(R) Core(TM) i7-6700K CPU @ 4.00GHz (8 Threads)

Issue description

I made changes to EditorExportPlugin in PR #93878 that allowed all resources to be skipped through the _export_file function. I have found no issues with most imported files, such as .wav audio files, but have found an issue that I believe is specific to Texture2D's interaction between _customize_resource and skip().

Works: Skipping an AudioStreamWAV and customizing a different AudioStreamWAV to be replaced by the skipped AudioStreamWAV.

Does not work: Skipping a CompressedTexture2D and customizing a different CompressedTexture2D to be replaced by the skipped CompressedTexture2D.

This is important functionality for porting because it allows the developer to, for example, replace existing PC-specific textures (such as an Xbox controller diagram) with mobile-specific textures (such as a touch-screen controls diagram). This functionality is needed to fully implement https://github.com/godotengine/godot-proposals/issues/10051 as an add-on for all resource types, including textures. (Update: there is a workaround, see my next comment.)

Explanation

When a resource is customized in EditorExportPlugin , a new .res file is added to the exported folder. For something like AudioStreamWAV, this will contain all of the resource data, so it doesn't matter if the source resource data is skipped because a full copy is made into the newly customized resource.

...But for a CompressedTexture2D resource, the full resource data is not copied. Instead, the new .res file will simply point to a .ctex file inside of the imported folder. So if this source texture is skipped, it will no be copied to the imported folder and this customized resource will now be invalid.

Development Status

I'm looking into a fix for this, but I would appreciate any thoughts that others might have on how to correctly solve this.

I don't see this as a regression in any way. It's just a very specific interaction that was not fully addressed when texture skipping was introduced in PR #93878.

Steps to reproduce

Create an EditorExportPlugin with something like this:

func _begin_customize_resources(platform: EditorExportPlatform, features: PackedStringArray) -> bool:
    return true

func _customize_resource(resource: Resource, path: String) -> Resource:
    if path == "res://default.svg":
        return load("res://mobile.svg")
    return null

func _export_file(path: String, type: String, features: PackedStringArray) -> void:
    if path == "res://mobile.svg":
        skip()

...And then export a project that uses res://default.svg.

Additionally, order doesn't matter: Renaming res://mobile.svg to res://amobile.svg does not change the behaivour.

Minimal reproduction project (MRP)

Testxport-customize-skip.zip

allenwp commented 1 month ago

Simply not skipping the CompressedTexture2D files that are expected to be referenced by another is a reasonable workaround: In this case, the CompressedTexture2D that is customized will have its .ctex files excluded from the project because it is now pointing to another.

This means that, for the specific case of CompressedTexture2D files, there is no need to skip the customized CompressedTexture2D or the CompressedTexture2D that it points towards; only the required ctex file will be kept in the project in this case.

This bug is definitely still an issue because it requires the user to handle CompressedTexture2D resources differently than any other type of file, but so long as you know about this behaviour, it's actually easy to workaround.