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

Support loading more DDS formats #81220

Merged
merged 1 commit into from
Dec 14, 2023

Conversation

BlueCube3310
Copy link
Contributor

@BlueCube3310 BlueCube3310 commented Sep 1, 2023

Depends on #80900

Adds support for loading the following DDS formats:

  • RGB10A2,
  • BGRA4,
  • BC6U / BPTC_UF,
  • BC6S / BPTC_SF,
  • BC7 / BPTC_RGBA,
  • R16F,
  • RG16F,
  • RGBA16F,
  • R32F,
  • RG32F,
  • RGBA32F
  • RGB9E5

An MRP containing DDS files encoded in these formats: DDSFormatsExtended.zip
Godot 4.2-dev3 is unable to load any of them correctly.

@fire
Copy link
Member

fire commented Nov 30, 2023

I support this, but I haven't had the chance to review and test what happens when I give it dds that are currently unsupported but will be supported.

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Code looks good. Tested locally with the images provided, and those from #80900 and can confirm that it works well.

Since this includes all the changes in #80900, should we just go ahead and merge this and close #80900?

@clayjohn clayjohn modified the milestones: 4.x, 4.3 Dec 7, 2023
@akien-mga
Copy link
Member

Since this includes all the changes in #80900, should we just go ahead and merge this and close #80900?

Either that, or if it makes sense to keep the commits separate (which it could, first refactor, then add features), then this PR should be rebases on top of 80900 and kept as two commits, instead of squashed.

@Zireael07
Copy link
Contributor

Looks like something went wrong when rebasing

@AThousandShips AThousandShips removed the request for review from a team December 7, 2023 16:08
@BlueCube3310 BlueCube3310 force-pushed the dds-formats-extended branch 5 times, most recently from ee06c18 to d9f9da7 Compare December 8, 2023 15:20
@clayjohn
Copy link
Member

Now that #80900 has been merged, this just needs a rebase and we can merge it :)

@clayjohn
Copy link
Member

Also, in case you are interested, I ran into another unsupported format in the wild
NormalsFittingTexture.zip

Renderdoc opens it as an A8_UNORM.

To be clear, I am not requesting you add support for another format to this PR.

@Calinou
Copy link
Member

Calinou commented Dec 14, 2023

Renderdoc opens it as an A8_UNORM.

For context:

$ file NormalsFittingTexture.dds
NormalsFittingTexture.dds: Microsoft DirectDraw Surface (DDS): 1024 x 1024, 8-bit color, alpha only

@BlueCube3310
Copy link
Contributor Author

Also, in case you are interested, I ran into another unsupported format in the wild NormalsFittingTexture.zip

Renderdoc opens it as an A8_UNORM.

I'll open another PR in the future with support for slightly less common formats such as this one. But on the topic of this format, what should Godot import it as? The engine doesn't support images with only the alpha channel, so should it be treated as l8 or l8a8 with fully black/white luminance channel?

@clayjohn
Copy link
Member

Also, in case you are interested, I ran into another unsupported format in the wild NormalsFittingTexture.zip
Renderdoc opens it as an A8_UNORM.

I'll open another PR in the future with support for slightly less common formats such as this one. But on the topic of this format, what should Godot import it as? The engine doesn't support images with only the alpha channel, so should it be treated as l8 or l8a8 with fully black/white luminance channel?

I would think l8. As its half the size of l8a8

@YuriSizov YuriSizov merged commit 58e6859 into godotengine:master Dec 14, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

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.

7 participants