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 DDS image load and save functionality #101994

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

Conversation

fire
Copy link
Member

@fire fire commented Jan 24, 2025

See: godotengine/godot-proposals#5748

The proposal involves adding the ImageLoaderDDS class, moving the DDS handling code inside it, and reusing the image loader (or part of it) in the texture loader in the future.

We are not supporting saving non-2d textures at this moment. Feature enhancement wanted.

  • BetsyCompressor has a runtime execution error Rendering team is handling it.
  • Only handle saving 2d-textures (like not 3d or 2d arrays)
  • Pass GitHub actions tests
  • Expand the list of supported Image::Formats and DDSFormats
  • Extract common header from DDS reader and DDS writer
  • Debug why BPTC dds loading isn't working
    • Fix BPTC unit test
  • Stress test the system with an example project https://v-sekai.github.io/manuals/decisions/20250122-VRChat-Loader-with-Godot-Engine.html
  • Add Image.load_dds_from_buffer()
  • Add Image.save_dds_from_buffer()
  • Error save_dds(const String &p_path) const
  • Vector<uint8_t> save_dds_to_buffer() const
  • Refactor DDS image preparation, update function signatures, and improve mipmap handling
  • Enhance DDS functionality by adding image saving, buffer handling, and loading capabilities
  • Expand DDS tests with image format conversions, metric checks, and compression methods
  • Update the DDS loader to use buffers in tests and handle unsupported formats
  • Improve DDS image saver to handle all mipmap levels and validate data sizes
  • Fix include guards and formatting in DDS image saver header files

@fire fire force-pushed the vsk-save-dds-4.4 branch from 02e11e1 to f335f2d Compare January 24, 2025 19:06
@bruvzg
Copy link
Member

bruvzg commented Jan 24, 2025

If info from the related MoltenVK issue I have mentioned here is correct, changing CI from runs-on: macos-latest to runs-on: macos-15 might fix the macOS issue (even if we won't use it since it's a preview image, it's probably worth checking just to confirm).

@fire
Copy link
Member Author

fire commented Jan 24, 2025

@bruvzg Is it possible/advisable to check for macOS-15 or newer at runtime?

@bruvzg
Copy link
Member

bruvzg commented Jan 24, 2025

Is it possible/advisable to check for macOS-15 or newer at runtime?

Yes:

if (@available(macOS 15.0, *)) {
// If it's Objective-C code.
}

or

if (__builtin_available(macOS 15.0, *)) {
// If it's C++ code.
}

@fire
Copy link
Member Author

fire commented Jan 24, 2025

How do I invert it? I want an if not macOS 15.0 or greater, then return.

if (__builtin_available(macOS 15.0, *)) {
// If it's C++ code.
}

@fire fire force-pushed the vsk-save-dds-4.4 branch 2 times, most recently from 32e1352 to 792acf7 Compare January 28, 2025 16:50
@fire

This comment was marked as outdated.

@fire fire force-pushed the vsk-save-dds-4.4 branch from 792acf7 to 6743043 Compare January 28, 2025 18:04
@BlueCube3310

This comment was marked as resolved.

@fire fire force-pushed the vsk-save-dds-4.4 branch 2 times, most recently from 7fd93df to 290131f Compare January 29, 2025 19:24
@fire

This comment was marked as resolved.

@fire

This comment was marked as resolved.

@fire fire force-pushed the vsk-save-dds-4.4 branch from e6aff30 to 2c4dd12 Compare February 5, 2025 08:07
@fire
Copy link
Member Author

fire commented Feb 5, 2025

Commentary in rocketchat mentioned:

  1. betsy isn't falling back properly in a headless environment
  2. Suggest that we write dds directly with uncompressed images

@fire fire force-pushed the vsk-save-dds-4.4 branch from 2c4dd12 to 4e1450d Compare February 5, 2025 18:55
@fire fire marked this pull request as ready for review February 5, 2025 19:32
@fire fire requested review from a team as code owners February 5, 2025 19:32
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.

Tested this with all supported Image formats (uncompressed + s3tc + rgtc + bptc), some images fail to save. This diff should fix that and BGRA4 channel swapping: dds_uncomp.patch

Regarding RGB565: It saves correctly, but it looks like the renderers (both RD and Compatibility) expect the colors to be stored as BGR565, while the Image class operates on RGB565.

Edit: Here's a testing project:
dds-saveload-test.zip

@fire fire force-pushed the vsk-save-dds-4.4 branch from 4e1450d to ae6394f Compare March 5, 2025 14:50
@BlueCube3310
Copy link
Contributor

#103635 should fix the issues with RGB565, otherwise this PR looks good in terms of functionality

Save and load DDS from Image class.

Co-authored-by: BlueCube3310 <[email protected]>
@fire fire force-pushed the vsk-save-dds-4.4 branch from 7b6f95d to 082511e Compare March 5, 2025 17:40
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.

Tested this with several different DDS files, loading still works correctly, saving also works. This will make it easier to debug compressed/VRAM-specific images

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Approved, Waiting for Production
Development

Successfully merging this pull request may close these issues.

4 participants