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

Option for Texture Importer to ignore image alpha #81803

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Nomad1
Copy link
Contributor

@Nomad1 Nomad1 commented Sep 17, 2023

In some workflows, opaque images are stored in with alpha channel that results in DXT5 and other compression formats with abundant data. This patch adds an option to the Channel Pack dropdown to ignore the alpha channel. It works only for non-HDR RGBAF, RGBAH, and RGBA8 images.

Correctly cherry-picked and fixed.

@Nomad1 Nomad1 requested a review from a team as a code owner September 17, 2023 13:54
@AThousandShips AThousandShips changed the title Option for Texture Importer to ignore image alpha (corrected) Option for Texture Importer to ignore image alpha Sep 17, 2023
@AThousandShips AThousandShips added this to the 4.x milestone Sep 17, 2023
@Nomad1
Copy link
Contributor Author

Nomad1 commented Sep 17, 2023

One of the checks failed but it doesn't look even closely related to the code submitted. Is everything OK with the checker?

@Calinou
Copy link
Member

Calinou commented Sep 17, 2023

I believe this should be discarded automatically based on the presence of a transparent pixel (alpha < 1.0) in the source image, instead of asking the user to tick a checkbox. We have a method in the Image class for this 🙂

@Nomad1
Copy link
Contributor Author

Nomad1 commented Sep 17, 2023

I believe this should be discarded automatically based on the presence of a transparent pixel (alpha < 1.0) in the source image, instead of asking the user to check a checkbox automatically. We have a method in the Image class for this 🙂

I believe it should be both. In the best case, there should be an "Override format" drop-down that allows the user to select something alike RGBA4444 when needed without guessing the desired format. In my case, non-transparent texture atlases could have transparent pixels due to atlasing software adding borders and aligning elements to a grid. However, I still treat those textures as fully opaque and don't need an Alpha channel there.

@Calinou
Copy link
Member

Calinou commented Sep 17, 2023

There was a proposal requesting the ability to force an arbitrary pixel format, but there's not much demand for it: godotengine/godot-proposals#2107

@Nomad1
Copy link
Contributor Author

Nomad1 commented Sep 17, 2023

Don’t see why it wasn’t implemented. It’s a useful feature for many cases. At the moment I lack the knowledge of internal options mechanisms to implement it, but I’ll look if it can be done in a few hours.

@Nomad1
Copy link
Contributor Author

Nomad1 commented Sep 17, 2023

So the format change feature is pretty straightforward and ready for a pull request, however, I don't see a way to hide the dropdown if the texture has an incompatible format (already compressed or some special case where image::convert would fail). So it looks like this:

Screenshot 2023-09-17 at 19 50 30

However, some formats are not supported by samplers (RGBA4444 for instance) and some take way too long to compress, so trying each format in the list could result in console errors, long conversion times, or even a broken game if the format is unsupported. I can hide it under a separate category and a checkbox "Show Advanced Options" + add a tooltip stating that it is needed only for some special cases.

On the other hand, we already have a dropdown called "Channel Pack" and my initial patch added an option there. We could also add some more options there like channel swap for BGRA shaders:

Screenshot 2023-09-17 at 20 18 54

It makes perfect sense for me: from this drop-down we can drop the alpha channel, use sRGB channel layout, use an optimized layout or swap R and B channels if needed.

It's for engine maintainers to decide which way should we choose, the code is ready for both of them and I'll upload it when needed.

@fire
Copy link
Member

fire commented Sep 18, 2023

Does force format have a default option? I am placing this workflow as if there are a 1000 assets in a Godot Engine repository. Making manual changes would take a lot of time.

@Nomad1
Copy link
Contributor Author

Nomad1 commented Sep 18, 2023

Does force format have a default option? I am placing this workflow as if there are a 1000 assets in a Godot Engine repository. Making manual changes would take a lot of time.

Sure, the default option is to use the original texture format without the override. It would work for 95% of cases. You need to change the format only when the source format has more features than needed for the engine, i.e. you need only the Luminance channel or RGB and not the whole RGBA.
Screenshot 2023-09-17 at 20 19 20

@Nomad1
Copy link
Contributor Author

Nomad1 commented Sep 21, 2023

While the core team is still deciding if we need an explicit format selection, I've added more modes to the Channel Pack drop-down. You need to choose if we are taking this route (extending the Channel Pack) or this one.

Screen.Recording.2023-09-21.at.10.33.21.mov

editor/import/resource_importer_texture.cpp Outdated Show resolved Hide resolved
editor/import/resource_importer_texture.cpp Outdated Show resolved Hide resolved
editor/import/resource_importer_texture.cpp Outdated Show resolved Hide resolved
editor/import/resource_importer_texture.cpp Outdated Show resolved Hide resolved
editor/import/resource_importer_texture.cpp Outdated Show resolved Hide resolved
editor/import/resource_importer_texture.cpp Outdated Show resolved Hide resolved
Copy link
Contributor Author

@Nomad1 Nomad1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment style changes

@clayjohn
Copy link
Member

I'm a little unsure about this. AFAIK our current importer already checks whether the alpha channel is used, if it isn't, then it automatically drops the alpha channel.

Are you saying that you have textures where the alpha value is less than 255, but nevertheless alpha should be ignored?

For context, our importer looks at the texture, if all alpha bits are 255, it can ignore alpha and use another, more compressed format, if all alpha bits are 0 or 1, it assumes a binary alpha value and uses a compatible format, otherwise it keeps the four channel format.

@Nomad1
Copy link
Contributor Author

Nomad1 commented Sep 21, 2023

I'm a little unsure about this. AFAIK our current importer already checks whether the alpha channel is used, if it isn't, then it automatically drops the alpha channel.

Are you saying that you have textures where the alpha value is less than 255, but nevertheless alpha should be ignored?

For context, our importer looks at the texture, if all alpha bits are 255, it can ignore alpha and use another, more compressed format, if all alpha bits are 0 or 1, it assumes a binary alpha value and uses a compatible format, otherwise it keeps the four channel format.

There are many cases when the graphics department gives you a background image that should be opaque but it is in PNG RGBA format and the importer thinks it has Alpha channel and never uses DXT1 or ETC1 for it, doubling the file size and memory usage:
main_menu
Or the texture atlas packer gives you something like this:
scene

Or even you need only a single channel from SDF texture:
font4

Surely, we can tweak the textures by hand, discard the alpha channel, pre-convert to single channel TIFF, etc. And then do it again and again every time when the content changes. Or we can set a single option in the importer and never think of it again.

@Nomad1
Copy link
Contributor Author

Nomad1 commented Sep 21, 2023

Once again checks fail in some unrelated parts.

@AThousandShips
Copy link
Member

Unrelated to your PR, this is being investigated:

@Calinou
Copy link
Member

Calinou commented Sep 21, 2023

There are many cases when the graphics department gives you a background image that should be opaque but it is in PNG RGBA format and the importer thinks it has Alpha channel

There is an Image method that checks for actual non-opaque pixels, and could be used to detect whether DXT5 is worth using over DXT1.

I think the approach in your PR has merits (especially if support for high bit depth PNG is implemented in the future, as it would remain RGB(A)8 by default for performance reasons). That said, we need to make sure the "default" path provides a good level of optimization, as most users will not tweak it manually.

@Nomad1
Copy link
Contributor Author

Nomad1 commented Sep 21, 2023

There are many cases when the graphics department gives you a background image that should be opaque but it is in PNG RGBA format and the importer thinks it has Alpha channel

There is an Image method that checks for actual non-opaque pixels, and could be used to detect whether DXT5 is worth using over DXT1.

It's already used in godot/drivers/png/png_driver_common.cpp. I confirm that it doesn't work with the first of the images posted above and will not work for the other cases I mentioned:

    if (source_image->detect_alpha()) {
        source_image->convert(Image::FORMAT_RGBA8);
        png_img.format = PNG_FORMAT_RGBA;
    } else {
        source_image->convert(Image::FORMAT_RGB8);
        png_img.format = PNG_FORMAT_RGB;
    }

In some workflows opaque images are stored in with alpha channel that results in DXT5 and other compression formats with abundant data. This patch adds an option to Channel Pack dropdown to ignore alpha channel. It works only for non-HDR RGBAF, RGBAH and RGBA8 images.
Comment on lines 655 to +658
use_uncompressed = true;
}
}
} else if (pack_channels == 2) { // Discard alpha.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous if statement here (is_hdr) is checking has_alpha, but we should be setting has_alpha to false if pack_channels == 2 or 4 or some of the other numbers.

@@ -586,6 +586,18 @@ Error ResourceImporterTexture::import(const String &p_source_file, const String
}
}
}

if (pack_channels == 3) { // BGRA mode, swapping B and R channels.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is way too specific. A user is going to read BGRA and assume it means RGBA since there is no RGBA option currently.

@lyuma
Copy link
Contributor

lyuma commented Feb 19, 2024

I think it might be interesting to support remapping color channels (and specifying invert / 1-x for each) but we should make this its own proposal. Honestly that would also give an ideal place to drop unwanted / unused color channels (set them to 0 or 1 for alpha). Then, the Optimized case can handle discarding those all-0 and all-1 channels the same way it does now.

Here's my proposal. Ideally it can be collapsed, but maybe it's ok if it's at the bottom of the Texture import dock

Per-channel settings ▸

  • Red: dropdown [R, G, B, A, Invert R, Invert G, Invert B, Invert A, Unused (All 0), All 1]
  • Green: dropdown [R, G, B, A, Invert R, Invert G, Invert B, Invert A, Unused (All 0), All 1]
  • Blue: dropdown [R, G, B, A, Invert R, Invert G, Invert B, Invert A, Unused (All 0), All 1]
  • Alpha: dropdown [R, G, B, A, Invert R, Invert G, Invert B, Invert A, Unused (All 1), All 0]

And then if you want R-only you set green, blue and alpha to Unused.
Or if you want something complex like BGRA, you can set that.

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.

6 participants