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

Revert "Implement loading DDS textures at run-time" #81126

Merged
merged 1 commit into from
Aug 29, 2023

Conversation

clayjohn
Copy link
Member

This reverts commit 34ab1c8.

Reverts: #69085

It turns out that by registering the load function with ImageLoader the import system would automatically detect .dds files as images that need to be imported. This has 2 negative side-effects:

  1. a .import file is created for the .dds file with import settings
  2. The image is imported as if it isn't a compressed texture, so Godot fails at multiple points during the import process and produces a broken texture as a result.

DDS files should not be imported like other textures.

The root of the issue is how we detect resources at import time. When loading an image at run time, we run through all the loaders registered with ImageLoader. If the file extension matches one of the loaders, then we call that loader to load the file.

At import time, we loop through all files in the project, and check them against the support extensions of all loader types (ImageLoader, ResourceLoader, etc.), which each loop through all of their registered loaders. This is used to a create a map of what resources should be imported and by which loader etc. in current master .dds is registered with ResourceLoader and ImageLoader. But for some reason ImageLoader seems to take precedence and the file is treated like an image.

If we want to support loading dds files at runtime, then we need to figure out a way to exclude them during import, but keep them registered with ImageLoader.

If users ran projects with .dds files after #69085 was merged. Even after this PR, the .import files will remain in the project, but they will be ignored and the only import option will be "keep file (no import)"

CC @TokisanGames @marcinn1

@clayjohn clayjohn added this to the 4.2 milestone Aug 29, 2023
@clayjohn clayjohn requested review from a team as code owners August 29, 2023 12:45
@TokisanGames
Copy link
Contributor

Source issue TokisanGames/Terrain3D#190
#69085 (comment)

@marcinn1
Copy link

I did quick test and it looks that disabling registration of "dds" extension in ImageLoaderDDS solves the issue, and still gives a possibility of loading DDS at runtime.

It would be nice if this little patch can be applied instead of reverting whole PR. Maybe this could be checked by @TokisanGames?

diff --git a/modules/dds/image_loader_dds.cpp b/modules/dds/image_loader_dds.cpp
index d661f6664a..6e1a379c58 100644
--- a/modules/dds/image_loader_dds.cpp
+++ b/modules/dds/image_loader_dds.cpp
@@ -417,5 +417,5 @@ ImageLoaderDDS::ImageLoaderDDS() {
 }
 
 void ImageLoaderDDS::get_recognized_extensions(List<String> *p_extensions) const {
-       p_extensions->push_back("dds");
- 
+       //p_extensions->push_back("dds");
 }

image

The problem with import system is more complex and related to the core of Godot Editor. I tried to change implementation of ResourceImageDDS by disabling extensions and types, but only removing extension in ImageLoaderDDS forces editor to keep dds files untouched.

@TokisanGames
Copy link
Contributor

I commented out the one line and Terrain3D loads up as it did before in 4.1.1 with textures untouched, no import files, the Import tab blank and disabled, and no errors. Didn't/Can't try export (w/o the export files).

I then ran the demo scene with this in ready. The file is 1024x1024, 10 mipmaps

func _ready():
	var img: Image = Image.new()
	img.load("res://demo/textures/ground037_alb_ht.dds")
	print(img.get_size())

	img = Image.load_from_file("res://demo/textures/ground037_alb_ht.dds")
	print(img.get_size())


Output:
WARNING: Loaded resource as image file, this will not work on export: 'res://demo/textures/ground037_alb_ht.dds'. Instead, import the image file as an Image resource and load it normally as a resource.
     at: load (core/io/image.cpp:2495)
(0, 0)
WARNING: Loaded resource as image file, this will not work on export: 'res://demo/textures/ground037_alb_ht.dds'. Instead, import the image file as an Image resource and load it normally as a resource.
     at: load_from_file (core/io/image.cpp:2504)
ERROR: Failed to load image. Error 15
   at: (core/io/image.cpp:2511)

<<<editor crashed, game ran>>>

There are no code errors in the editor. But upon running the terrain3d demo, the editor stops on an error, img.get_size(), then crashes. The editor window goes away. The console remains open, and my demo scene runs fine.

image

@marcinn1
Copy link

@TokisanGames Thanks.

@akien-mga akien-mga merged commit cfe9cd5 into godotengine:master Aug 29, 2023
@akien-mga
Copy link
Member

Thanks!

@andreymal
Copy link

Should #69101 also be reverted?

@clayjohn clayjohn deleted the revert-dds-runtime branch August 29, 2023 20:43
akien-mga added a commit that referenced this pull request Sep 1, 2023
@Spartan322
Copy link
Contributor

Spartan322 commented Nov 23, 2024

Would not a viable solution to godotengine/godot-proposals#5748 trying to use the ImageLoader first have been ResourceLoader::add_resource_format_loader(resource_loader_dds, true) since p_at_front which would put it ahead of the ResourceLoaderImage among the ResourceLoader::loader array? Or is there a reason for this not to be done?

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

Successfully merging this pull request may close these issues.

7 participants