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

Preserve existing alpha channel when using Normal Map Invert Y import option #93714

Merged

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Jun 28, 2024

While normal maps with RGTC compression cannot contain an alpha channel, it is possible for Godot to read non-RGTC normal maps that can contain an alpha channel that custom shaders can read. This is the case both in 2D (where VRAM compression and therefore RGTC is generally not used in the first place) and 3D.

The green channel inversion is premultiplied by alpha to avoid green outlines in the transition between opaque and transparent pixels, particularly in generated mipmaps.
Edit: Premultiplication is no longer done (see below), so this PR just preserves the alpha channel and makes no other changes.

The visual output for opaque images (including RGTC normal maps) is unaffected by this change.

I noticed this while testing #91284 (which doesn't have that issue with the alpha channel).

Testing project: test_normal_map_invert_y_alpha.zip

Preview

The normal map is used as an albedo texture for easier visibility.

Before After
Screenshot_20240628_204018 png webp Screenshot_20240628_204156 png webp!

Note that the "After" screenshot has green outlines visible on the mipmaps, but I believe this is more an issue of how mipmap generation is done. The outline goes away if we premultiply the green channel inversion by the alpha channel, but this breaks use cases other than usage as albedo (which I don't think will be frequent):

Screenshot_20240628_204423 png webp

Non-inverted normal map for reference:

Screenshot_20240628_204010 png webp

@Calinou Calinou added this to the 4.4 milestone Jun 28, 2024
@Calinou Calinou requested a review from a team as a code owner June 28, 2024 18:51
… option

While normal maps with RGTC compression cannot contain an alpha channel,
it is possible for Godot to read non-RGTC normal maps that can contain
an alpha channel that custom shaders can read.

The visual output for opaque images (including RGTC normal maps)
is unaffected by this change.
@Calinou Calinou force-pushed the normal-map-invert-y-preserve-alpha-channel branch from e6694ed to 49b4c20 Compare June 28, 2024 19:48
Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

I am ok with the feature proposal.

Did not test on my use cases.

@lyuma
Copy link
Contributor

lyuma commented Jun 28, 2024

I just want to be clear about something lest this PR set a precedent: importing normal maps with alpha channel is unsupported in Godot. The RGTC is one example of why it is unsupported but more generally speaking, it should be assumed that using the alpha channel of a texture imported as normal map is undefined behavior. Disregarding the alpha channel or even the blue channel in a normal texture is a standard practice an assumption made in many places.

I understand the point here is to pack non-normal data into the alpha (or maybe also blue) channel of an otherwise uncompressed normalmap texture. This is unconventional but I could see the appeal to supporting more kinda of channel pack.

Therefore, this PR is phrased in a misleading way: Normal maps don't have alpha, but this is importing non-normal data into the alpha channel. The only supported way to do this in Godot is to tell the engine that you are not importing a normalmap, which I assume is what you are doing.

The thing I do agree with is this importer option really has nothing to do with normal maps, other than that was the justification likely used to add the feature (which is why we don't currently have options to invert red or blue channels). It could just as well be called simply "invert green channel". I would not be surprised if users use this oddly named import option for this purpose, and for that reason alone I agree that this PR is okay. 👍

But I do hope there is some consideration in the future to acknowledge the underlying issues here: users want more flexibility perhaps in regards to how channels are packed and what import options are available to adapt assets made with less common channel packs.

I am also mildly curious what process / pipeline was used that produced normal textures with packed alpha but an inverted y normalmap. Generally speaking, unless we make an effort to make the texture importer more flexible for channel packing, it is simply not possible to adapt all textures to Godot using import settings alone, and I don't think we should bend over backwards to make our existing settings work for every usecase. The code to adapt a texture to match godot conventions is not difficult to write, and this type of operation can also be performed in programs such as imagemagick.

I also want to note that as your description notes, this is for an imported texture "that custom shaders can read". If you are already using a custom shader, would it not be simple to perform the 1-normal.y operation also in the shader code?

In summary, TLDR: I disagree with this premise for this change, and I do not want this to set a precedent, but I think the option in question is misnamed and the change is harmless so I'm ok with it.

@Calinou
Copy link
Member Author

Calinou commented Jun 28, 2024

but I think the option in question is misnamed and the change is harmless so I'm ok with it.

See #39202 for rationale on why the import option is called "Normal Map Invert Y" as opposed to "Invert Green Channel".

Copy link
Contributor

@BlueCube3310 BlueCube3310 left a comment

Choose a reason for hiding this comment

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

I agree that this import option shouldn't affect the alpha channel

@Repiteo Repiteo merged commit 3d6e712 into godotengine:master Nov 18, 2024
18 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 18, 2024

Thanks!

@Calinou Calinou deleted the normal-map-invert-y-preserve-alpha-channel branch January 21, 2025 16:20
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.

5 participants