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

Add Alpha from Grayscale import option to Texture2D #88669

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

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Feb 22, 2024

This can be used to import textures that are on a solid black background and use them for VFX.

This also adds and exposes Image::get_format_with_alpha() and Image::get_format_without_alpha() static methods for convenience when working with image formats.

Testing project: test_alpha_from_grayscale.zip
Sprite from Kenney Particle Pack, licensed under CC0 1.0 Universal.

Preview

alpha_from_grayscale

@Exyde @FyiurAmron Can you confirm this has the expected appearance?

@Calinou Calinou added this to the 4.x milestone Feb 22, 2024
@Calinou Calinou requested review from a team as code owners February 22, 2024 14:51
doc/classes/Image.xml Outdated Show resolved Hide resolved
doc/classes/Image.xml Outdated Show resolved Hide resolved
doc/classes/Image.xml Outdated Show resolved Hide resolved
@FyiurAmron
Copy link

FyiurAmron commented Feb 22, 2024

@Calinou GIMP's "Color to Alpha" does a different trick, it doesn't apply alpha from grayscale properly. To have it match the reference, you'd have to import the image as its own transparency/alpha mask. Then, the result matches the behaviour of your code perfectly:

image

tl;dr LGTM, I confirm your code does exactly what it should, +1.

BTW, what's with the brightness on your reference image? It's toned down vs what the actual images from the ZIP have, e.g.
image
?

@Mickeon
Copy link
Contributor

Mickeon commented Feb 22, 2024

BTW, what's with the brightness on your reference image? It's toned down vs what the actual images from the ZIP have, e.g.

Linear to standard RGB, judging by the gamma change.

@Calinou
Copy link
Member Author

Calinou commented Feb 22, 2024

BTW, what's with the brightness on your reference image? It's toned down vs what the actual images from the ZIP have, e.g.

Filmic tonemapping with whitepoint 6 is used in the MRP.

@Calinou Calinou force-pushed the import-add-alpha-from-grayscale branch from 249288e to 37572ac Compare February 22, 2024 18:33
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

That extra detail in is_compressed is very, very necessary

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Feb 22, 2024
@akien-mga akien-mga requested a review from a team February 22, 2024 22:33
@akien-mga akien-mga modified the milestones: 4.3, 4.x Feb 22, 2024
@clayjohn
Copy link
Member

I'm not sure that this is something we need to have in our importer. It seems like a workaround for a very specific workflow. It seems the problem that this is solving is that users have assets that are in grayscale with no alpha since they are designed to be used as additive textures, but they want to use them as an alpha mask without making any changes to the texture in an external program.

I'm not sure this belongs in Godot asset authoring is something that is better handled by programs dedicated to asset authoring. I think it makes sense to include common operations in Godot or things that are required for places where Godot may differ from the asset authoring tool (like flipping the green channel in normal maps), but I don't think we should add every image operation just because we can.

The underlying proposal doesn't seem to have gained much interest either in the 2 years its been open. I have a feeling it is something that is very niche.

@Zireael07
Copy link
Contributor

better handled by programs dedicated to asset authoring

The problem with that line of thinking is the same as with thinking "just make assets that line up with Godot's default forward axis". Namely, a lot of devs use freely available assets (e.g. from asset stores, Open Game Art, or other free asset sites) and do NOT have ANY influence on how the asset is made

@FyiurAmron
Copy link

FyiurAmron commented Feb 24, 2024

@clayjohn

It seems like a workaround for a very specific workflow.

That has been addressed directly in godotengine/godot-proposals#5247 (comment)

It seems the problem that this is solving is that users have assets that are in grayscale with no alpha since they are designed to be used as additive textures, but they want to use them as an alpha mask without making any changes to the texture in an external program.

Yes, that's exactly what it's solving. See above, ditto.

I'm not sure this belongs in Godot asset authoring is something that is better handled by programs dedicated to asset authoring.

This, by itself, belongs to neither, and in ideal situation can be handled by either.

I think it makes sense to include common operations in Godot or things that are required for places where Godot may differ from the asset authoring tool (like flipping the green channel in normal maps),

Yes.

but I don't think we should add every image operation just because we can.

Yes, but this is not "every image operation". Ditto. Many assets and workflows are made with this assumption in mind, and, arguably, that's why e.g. Unity provides this option. Code-wise, it's trivial to implement (as @Calinou clearly showed) - meanwhile, batch-processing the assets to include additional unneeded data (4th channel that is implied from avg. pixel brightness) is tedious and prone to errors.

The underlying proposal doesn't seem to have gained much interest either in the 2 years its been open. I have a feeling it is something that is very niche.

See above. If 10 people voted on having something that required literally one operation (equivalent of 1 LLOC more or less) in a loop and some glue to implement, IMVHO that's the correct ratio of need-vs-effort. Also, we already have the code, courtesy of @Calinou , so actually the cost is near-zero ATM; however, the question here would be: should we go and remove all features in Godot that didn't gather more than N votes in proposals during T months/years?

Also maybe let's keep the code/implementation-related discussion here and the feature/idea discussion in the proposal discussion (instead of introducing the latter here in MR)?

@Calinou Calinou force-pushed the import-add-alpha-from-grayscale branch 2 times, most recently from 2fde8ec to 0d173de Compare March 21, 2024 22:55
@FyiurAmron
Copy link

@Calinou are there any blockers/prerequisites for this (IIUC, #88763 was, but it's merged already?), or is this ready to be merged?

@FyiurAmron
Copy link

FyiurAmron commented Aug 19, 2024

@Calinou ^ bumping the question above for great justice :) Is there any help, be it testing or coding, needed here?

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.

Needs a rebase, but otherwise looks great

This can be used to import textures that are on a solid black
background and use them for VFX.

This also adds and exposes `Image::get_format_with_alpha()` and
`Image::get_format_without_alpha()` static methods for convenience
when working with image formats.
@Calinou Calinou force-pushed the import-add-alpha-from-grayscale branch from 0d173de to adf52dc Compare November 14, 2024 16:00
@Calinou
Copy link
Member Author

Calinou commented Nov 14, 2024

Rebased and tested again, it works as expected.

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.

Approving for import

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.

Add "Alpha from GrayScale" texture import setting
8 participants