godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.16k stars 97 forks source link

Implement loading DDS textures at run-time #5748

Open marcinn opened 1 year ago

marcinn commented 1 year ago

Describe the project you are working on

An alternate editor for existing train simulator (not written in Godot).

Describe the problem or limitation you are having in your project

The original project is using DDS format for textures. DDS format in Godot is handled as a texture loader only (during importing assets within the editor), but my tool must work with original data files. These files are external for Godot.

Godot can't handle DDS files via Image.load(). The method is returing ERR_FILE_UNRECOGNIZED at runtime. DDS is handled only at import stage.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

The proposal is about adding ImageLoaderDDS class, moving DDS handling code inside it, and reusing image loader (or part of it) in texture loader.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

Here is a prototype version of a patch (currently only for 3.x branch): https://github.com/marcinn/godot/tree/image-loader-dds

It allows to use DDS files directly:

var im = Image.new()
im.load("/path/to/external/texture.dds")

If this enhancement will not be used often, can it be worked around with a few lines of script?

It cannot be worked around by few lines of script. Image loading is a core of the engine.

Is there a reason why this should be core and not an add-on in the asset library?

  1. The patch seems to be fully compatible
  2. It makes Godot more compliant with the table of supported formats: https://docs.godotengine.org/en/stable/tutorials/assets_pipeline/importing_images.html
Calinou commented 1 year ago

This should be added in a similar manner to the load_*_from_buffer() class, so that you can also load DDS textures from memory.

andreymal commented 1 year ago

I guess this should be reopened now

YuriSizov commented 1 year ago

Indeed, the PR was reverted due to issues with DDS files in the project folder being imported as image resources which prevented them from being used in their raw form. The feature in this proposal is still approved though.

marcinn1 commented 1 year ago

Hi.

I checked carefully images loading, resources importing and resources loading in Godot.

The ImageLoader is used in many resource importers (for textures, texture atlas, bitmask, and so on). File formats, which will be selected to import, are based on extensions (ImageLoader::get_recognized_extensions() called from ResourceImporter::get_recognized_extensions(). Also images handled by ImageLoader::load_image(), called mostly from Image::load_from_file(), are based on ImageLoader::get_recognized_extensions(). That's why adding dds as recognized extension by ImageLoader caused importing of DDS files (ResourceImporterTexture was used).

All image formats supported by ImageLoader can be also imported, if there is at least one resource importer based on ImageLoader. That's how Godot works.


To support loading of DDS files just by ImageLoader without importing them, we have to distinguish between all supported extensions and import-only extensions, i.e. by adding ImageLoad::get_recognized_extensions_to_import(). I've created a patch to illustrate this concept, but I don't like it: https://github.com/godotengine/godot/commit/26dffb868ece3d6d790eaf89362e9b3570cd8b92

I don't like to have a second method just for import stage, and I don't like erasing "dds" extension from p_extensions list. Well... Godot's methods like get_supported_extensions are named wrongly because they aren't getters, but this does not matter at all. I also thought about blacklisting extensions, but I don't like the concept, too.

I think the clean way to distinguish extensions between import stage and other cases is to change the interface of get_recognized_extensions to (List<String>p_extensions, bool is_importing). But if we make that change, we should consider adding this flag to ImageFormatLoaderExtension::get_recognized_extensions(), which will introduce a breaking change for all custom image loaders. I'm not sure we want to do that.


I've also found that using ResourceLoader instead of ImageLoader is a way to load DDS textures at runtime. So instead of doing this:

        var im = Image.load_from_file(dds_absolute_path)
        var t = ImageTexture.create_from_image(im)
        m.albedo_texture = t
        $CSGBox3D.material = m

I can do this:

        var t = ResourceLoader.load(dds_absolute_path)
        m.albedo_texture = t
        $CSGBox3D.material = m

That would skip dds parsing / converting, it should be faster, so it should fit my use case better.


But I still don't understand why does Documentation says that Godot can import DDS files. It can't. Godot can use DDS files. There are no import options for DDS. The files will be visible in the editor, but they aren't imported (they have no .import files).

image

Also the documentation of Image::load is pointing to the page, where DDS is listed as supported format. In fact, the DDS format is NOT supported by Image::load, what my initial patch was trying to solve:

image


So I see two solutions for now:

  1. change the docs to describe the actual state of image handling in Godot,
  2. implement ImageLoader, ImageFormatLoader and ImageFormatLoaderExtension API changes to distinguish between image loading and importing.

What do you think?

marcinn1 commented 1 year ago
 var t = ResourceLoader.load(dds_absolute_path)
        m.albedo_texture = t
        $CSGBox3D.material = m

I was made aware that ResourceLoader is designed to work with Godot's internal format. I have no idea why it works with external (not imported) DDS files. I'm confused by Godot's inconsistency.

Could someone tell how Godot SHOULD load external media files? There are load_from_buffer methods here and there. But how it should load DDS textures (it can skip Image class, I think)? How it should load OGG/WAV/MP3 files? I'm asking about audio files because of a related issue #8055

Calinou commented 1 year ago

Could someone tell how Godot SHOULD load external media files? There are load_from_buffer methods here and there. But how it should load DDS textures (it can skip Image class, I think)?

Adding Image.load_dds_from_buffer() and making Image.load_from_file() work with DDS seems like the most logical option to me.

PS: It seems like Image.load_from_file() doesn't work with .ktx files even though we have Image.load_ktx_from_buffer() already.