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

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

Open
allenwp opened this issue Jul 7, 2024 · 2 comments

Comments

@allenwp
Copy link
Contributor

allenwp commented Jul 7, 2024

Tested versions

Commit 932c191 onwards

System information

Godot v4.3.beta (b97110c) - 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 godotengine/godot-proposals#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

@akien-mga akien-mga added this to the 4.3 milestone Jul 7, 2024
@allenwp
Copy link
Contributor Author

allenwp commented Jul 8, 2024

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.

@akien-mga akien-mga modified the milestones: 4.3, 4.4 Aug 8, 2024
allenwp added a commit to allenwp/godot that referenced this issue Sep 30, 2024
…behaviour of customizing texture resources.

The list of types that should not be both customized and skipped was created through trial-and-error with the following import types:

- AnimationLibrary
- BitMap
- Translation
- CompressedCubemap
- CompressedCubemapArray
- FontFile
- FontFile
- FontFile
- RDShaderFile
- Image
- AudioStreamMP3
- AudioStreamWAV
- ArrayMesh
- PackedScene
- CompressedTexture2D
- CompressedTexture2DArray
- CompressedTexture3D
- AtlasTexture
- AudioStreamOggVorbis

The reason `skip()` should not be called is because the original resource will now point to the `.ctex` (or equivalent) file of the resource that replaced it. In this scenario, the `.ctex` file that was initially referenced by the original resource will no longer be included in the project.

Co-authored-by: Tomek <kobewi4e@gmail.com>
sondredl pushed a commit to sondredl/godot that referenced this issue Nov 16, 2024
…behaviour of customizing texture resources.

The list of types that should not be both customized and skipped was created through trial-and-error with the following import types:

- AnimationLibrary
- BitMap
- Translation
- CompressedCubemap
- CompressedCubemapArray
- FontFile
- FontFile
- FontFile
- RDShaderFile
- Image
- AudioStreamMP3
- AudioStreamWAV
- ArrayMesh
- PackedScene
- CompressedTexture2D
- CompressedTexture2DArray
- CompressedTexture3D
- AtlasTexture
- AudioStreamOggVorbis

The reason `skip()` should not be called is because the original resource will now point to the `.ctex` (or equivalent) file of the resource that replaced it. In this scenario, the `.ctex` file that was initially referenced by the original resource will no longer be included in the project.

Co-authored-by: Tomek <kobewi4e@gmail.com>
@allenwp
Copy link
Contributor Author

allenwp commented Nov 19, 2024

Now that the behaviour is documented, this issue could maybe be closed? I don’t see it being fixed any time soon, at the very least…

tGautot pushed a commit to tGautot/godot that referenced this issue Feb 5, 2025
…behaviour of customizing texture resources.

The list of types that should not be both customized and skipped was created through trial-and-error with the following import types:

- AnimationLibrary
- BitMap
- Translation
- CompressedCubemap
- CompressedCubemapArray
- FontFile
- FontFile
- FontFile
- RDShaderFile
- Image
- AudioStreamMP3
- AudioStreamWAV
- ArrayMesh
- PackedScene
- CompressedTexture2D
- CompressedTexture2DArray
- CompressedTexture3D
- AtlasTexture
- AudioStreamOggVorbis

The reason `skip()` should not be called is because the original resource will now point to the `.ctex` (or equivalent) file of the resource that replaced it. In this scenario, the `.ctex` file that was initially referenced by the original resource will no longer be included in the project.

Co-authored-by: Tomek <kobewi4e@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants