Pillow icon indicating copy to clipboard operation
Pillow copied to clipboard
verified publisher icon python-pillow

Refactor DDS plugin

Open REDxEYE opened this issue 3 years ago • 2 comments

Changes proposed in this pull request:

  • Refactored DDS reader/writter
  • Add Tile namedtuple for code readability
  • Add support for reading/writing single channel DDS textures

REDxEYE avatar Aug 06 '22 21:08 REDxEYE

Is it possible to make pre-commit.ci not to reformat some parts of the code?

REDxEYE avatar Aug 06 '22 21:08 REDxEYE

Yes. There is # fmt: off and # fmt: on. See https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#code-style for documentation.

radarhere avatar Aug 07 '22 03:08 radarhere

I extracted the support for single channel images, and it has been merged as PR #6820.

radarhere avatar Dec 26 '22 10:12 radarhere

Awesome!

REDxEYE avatar Dec 26 '22 10:12 REDxEYE

I've updated branch and added BC5U support

REDxEYE avatar Jun 15 '23 00:06 REDxEYE

I created #7358 to separately add BC5U reading, and that has been merged.

radarhere avatar Sep 05 '23 02:09 radarhere

Anything I need to improve (besides rebase) to get this PR merged?

REDxEYE avatar Sep 05 '23 11:09 REDxEYE

Your intention here is, correct me if I'm wrong, not primarily to add new functionality, but to reorganise the plugin's API into a form that you feel is more logical. From my perspective, Pillow values backwards compatibility, and that is at odds with this attempt.

You're welcome to see if other members of the Pillow team feel differently. I'm not against enums, I'm just reluctant to go against one of our guiding principles.

radarhere avatar Sep 05 '23 11:09 radarhere

Ah, I see. I can refactor it to use variables instead of enums. What about the other parts of the refactor?

REDxEYE avatar Sep 05 '23 11:09 REDxEYE

@hugovk did you have any thoughts on this?

radarhere avatar Sep 05 '23 12:09 radarhere

Understood. I agree with all suggestions, I will apply them as soon as I can.

REDxEYE avatar Sep 05 '23 17:09 REDxEYE

Just found weird DDS file with DDPF_RGB flag, but RGBBitsCount = 8. So need to handle them too

REDxEYE avatar Oct 21 '23 23:10 REDxEYE

The benefits of the refactoring in this PR from my current understanding

  • adds _Tile to help code clarity
  • adds enums to DdsImagePlugin (without removing existing constants)
  • previously, only flags were used to determine the type of uncompressed L data. Now the flags and the number of bits are used, to be more certain about the data type

Other benefits we are getting at the same time

  • ~~reading uncompressed "RGB" images with 8 bits~~ (edit: this will be added in #7589 instead)
  • reading DX10 BC1 UNORM and TYPELESS images
  • reading BC4U images

radarhere avatar Nov 05 '23 04:11 radarhere

Just found weird DDS file with DDPF_RGB flag, but RGBBitsCount = 8

I previously presumed that this would be greyscale, but now I see no reason why the masks shouldn't be applied. I've created https://github.com/REDxEYE/Pillow/pull/10 to remove the 8 bit RGB support here, instead leaving it to #7589 to add that functionality.

radarhere avatar Nov 21 '23 09:11 radarhere

A bit offtop. I may work on adding support for PVR textures in some time. I already implemented ETC decoder(one of they compression formats) https://github.com/REDxEYE/TextureDecoder/blob/master/library/src/decoders/etc/etc1.cpp

REDxEYE avatar Dec 03 '23 00:12 REDxEYE

I've retitled the PR, and added release notes. The release notes also cover #7603

radarhere avatar Dec 05 '23 08:12 radarhere

Thank you for your patience! This will be in the next release, set for 2nd January 2024.

hugovk avatar Dec 06 '23 20:12 hugovk