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 16-bits TGA support #65717

Merged
merged 1 commit into from
Sep 26, 2022
Merged

Conversation

Ithamar
Copy link

@Ithamar Ithamar commented Sep 12, 2022

This adds support for TGA files with 16 bits per pixel (basically, RGBA5551, where the alpha bit is optional).
It should be possible to cherry-pick this into 3.x branch with no conflicts too.

Closes #65715

@Ithamar Ithamar requested a review from a team as a code owner September 12, 2022 19:14
@Calinou Calinou added this to the 4.0 milestone Sep 12, 2022
@fire
Copy link
Member

fire commented Sep 12, 2022

We made a decision not to support dithered artwork, but not sure the current state of this decision.

@Ithamar
Copy link
Author

Ithamar commented Sep 12, 2022

We made a decision not to support dithered artwork, but not sure the current state of this decision.

Does that also mean that the RGB565 / RGB5551 Image formats would be dropped in 4.x?

@Calinou
Copy link
Member

Calinou commented Sep 12, 2022

We made a decision not to support dithered artwork, but not sure the current state of this decision.

This is only for import – the resulting image format remains whatever Godot supports.

Does that also mean that the RGB565 / RGB5551 Image formats would be dropped in 4.x?

No, they won't be dropped. I think @fire is referring to paletted (256-color) textures, which Godot can import (e.g. from PNGs) but GPUs haven't supported for decades. These are converted to RGB8 or RGBA8 on import depending on whether the PNG contains an alpha channel.

@LauPhi
Copy link

LauPhi commented Sep 16, 2022

I see that the RGBA5551 TGA is still being saved as an Image::FORMAT_RGBA8 (32-bit).
Would it be possible to save it as a proper Image::FORMAT_RGBA5551 (16-bit) so that it would take two times less VRAM while loaded in the GPU?

@Ithamar
Copy link
Author

Ithamar commented Sep 16, 2022

I see that the RGBA5551 TGA is still being saved as an Image::FORMAT_RGBA8 (32-bit). Would it be possible to save it as a proper Image::FORMAT_RGBA5551 (16-bit) so that it would take two times less VRAM while loaded in the GPU?

It seems the TGA loader simply converts anything it reads to FORMAT_RGBA8, including monochrome images. This could indeed be improved, but I think it is something for another PR (I would be happy to look into that, but I prefer this PR getting resolved first).

@Ithamar
Copy link
Author

Ithamar commented Sep 16, 2022

On another note, while I recently used Image::load_bmp_from_buffer, I notice the docs actually state that is does not support 16-bit, though it does not give a reason. @Calinou are you aware of why that is?

@Calinou
Copy link
Member

Calinou commented Sep 17, 2022

On another note, while I recently used Image::load_bmp_from_buffer, I notice the docs actually state that is does not support 16-bit, though it does not give a reason. @Calinou are you aware of why that is?

The only mention of 16-bit BMP I can see is in #30634. I don't think there's a specific reason for 16-bit BMP not being supported, so feel free to add support for it as well 🙂

Please do so in a separate pull request, so they can be reviewed and merged independently.

Copy link
Member

@SaracenOne SaracenOne left a comment

Choose a reason for hiding this comment

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

Seems fine to me!

@akien-mga akien-mga merged commit 985fc6c into godotengine:master Sep 26, 2022
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@akien-mga akien-mga added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Sep 26, 2022
@timothyqiu
Copy link
Member

Cherry-picked for 3.6

@timothyqiu timothyqiu removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Dec 4, 2022
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.

TGA import/loading does not support 16-bits-per-pixel files
7 participants