Basis plugins: support UASTC HDR #145

pezcode · 2024-09-11T19:05:35Z

Adds support for HDR format encoding and transcoding to BasisImageConverter and BasisImporter. UASTC HDR support got added to Basis in version 1.50.

The limitations for HDR currently are:

can't transcode LDR -> HDR or HDR -> LDR
HDR alpha is always 1

Depends on #119.

Todos:

tests
tests
did I mention tests
~~update example code for supported format detection code~~ requires _ldr and _hdr pseudo extensions for profiles of WEBGL_compressed_texture_astc

mosra

First of all, thanks a lot!

Doing a really tired review, so apologies if something doesn't make sense or the ramblings are too long. ... or if I'm pointing out something you mentioned in the description, such as tests. Or tests.

src/MagnumPlugins/BasisImageConverter/BasisImageConverter.cpp

mosra · 2024-09-11T21:36:06Z

src/MagnumPlugins/BasisImageConverter/BasisImageConverter.cpp

+                for(std::size_t x = 0; x != src.size()[1]; ++x)
+                    dst[y][x] = Math::Vector3<T>::pad(src[y][x]); /* Alpha implicitly opaque */
+
+    } else CORRADE_INTERNAL_ASSERT_UNREACHABLE(); /* LCOV_EXCL_LINE */


Ah, this reminds me you requested the batch swizzle. On my list :)

src/MagnumPlugins/BasisImageConverter/BasisImageConverter.cpp

src/MagnumPlugins/BasisImporter/BasisImporter.h

pezcode · 2024-09-27T20:24:46Z

Oops, I committed the basisu binary 🙈 Have to undo that

src/MagnumPlugins/BasisImporter/Test/BasisImporterTest.cpp

R32F is now a valid format and leads to an unexpected successful image conversion

src/MagnumPlugins/BasisImporter/BasisImporter.cpp

codecov · 2024-09-28T21:00:57Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.00%. Comparing base (a18f9d8) to head (801e501).
Report is 9 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #145   +/-   ##
=======================================
  Coverage   96.99%   97.00%           
=======================================
  Files         147      147           
  Lines       16552    16600   +48     
=======================================
+ Hits        16054    16102   +48     
  Misses        498      498

Flag	Coverage Δ
	`97.00% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mosra

I'll try to do a more thorough review later today, but so far seems like I won't have much to complain about. 👌

src/MagnumPlugins/BasisImporter/BasisImporter.cpp

Instead of an unavoidable copy. If users want 4-byte alignment, they can choose to perform the copy themselves. Most other importers do the same thing.

mosra

One last bit, other than that looks good to me (plus the 1-byte alignment from above, and arrayCast() instead of mutablePixels()).

I'm still rushing deadlines, so I won't be able to add that fake ldr/hdr extension for WebGL until later next week. So either the PR will wait, or I'll merge and the loading code will get (partially) updated later.

src/MagnumPlugins/BasisImageConverter/Test/BasisImageConverterTest.cpp

…d mutablePixels()

pezcode · 2024-09-29T19:02:42Z

So either the PR will wait, or I'll merge and the loading code will get (partially) updated later.

I think it's okay to do that later, otherwise this PR might get stalled out needlessly. Sometimes things come up etc. and this is just a convenient doc detail.

I reverted back to arrayCast() for the slicing, but my code for getting a view on the alpha channel looks terrible. Feel free to codegolf that a bit (or revert the commit) if it's worse than before 😸

mosra · 2024-09-29T20:24:46Z

Damn, I thought it is possible to slice a const view to single channels at least, sorry.

I'll fix that, those getters should return a const&, like Pair etc does. Various documentation mentions slice(&Color4::a) etc as one of the main slicing use cases, it'd be silly if that only worked on mutable views.

mosra · 2024-10-01T18:52:50Z

Squashed a bit and merged as a18f9d8...e327796. I reverted back to .slice(&Color4h::a), as of mosra/magnum@d5b5158 that works properly on const views as well. (Oh and it seems some trailing whitespace sneaked into your commits, I cleaned those up in the process but you might want to check your editor settings, or whether it could use the .editorconfig that's in the repository.)

In any case, thanks a lot for this!

mosra added this to the 2023.0a milestone Sep 11, 2024

mosra reviewed Sep 11, 2024

View reviewed changes

pezcode force-pushed the basis-hdr branch from c5ff083 to c130c36 Compare September 27, 2024 20:14

pezcode commented Sep 27, 2024

View reviewed changes

src/MagnumPlugins/BasisImporter/Test/BasisImporterTest.cpp Outdated Show resolved Hide resolved

pezcode force-pushed the basis-hdr branch from c130c36 to 6f25040 Compare September 28, 2024 16:21

pezcode added 5 commits September 28, 2024 18:33

BasisImporter: add HDR support

b32b699

BasisImporter: handle output formats that are not a multiple of 4 bytes

f7ca0cc

BasisImageConverter: add HDR support

77179b2

BasisImageConverter: list all files used in the test

4ab691a

GltfSceneConverter: adapt to BasisImageConverter adding HDR support

c051508

R32F is now a valid format and leads to an unexpected successful image conversion

pezcode force-pushed the basis-hdr branch from 6f25040 to c051508 Compare September 28, 2024 16:33

BasisImporter: list all files used in the test

683146e

pezcode commented Sep 28, 2024

View reviewed changes

src/MagnumPlugins/BasisImporter/BasisImporter.cpp Outdated Show resolved Hide resolved

mosra reviewed Sep 29, 2024

View reviewed changes

src/MagnumPlugins/BasisImporter/BasisImporter.cpp Outdated Show resolved Hide resolved

pezcode added 2 commits September 29, 2024 14:55

BasisImporter: set alignment to 1 for unaligned formats

3eb2b84

Instead of an unavoidable copy. If users want 4-byte alignment, they can choose to perform the copy themselves. Most other importers do the same thing.

BasisImporter: make codecov happy

0690611

mosra reviewed Sep 29, 2024

View reviewed changes

src/MagnumPlugins/BasisImageConverter/Test/BasisImageConverterTest.cpp Outdated Show resolved Hide resolved

pezcode added 2 commits September 29, 2024 20:45

Basis{ImageConverter,Importer}: use conventional view slicing to avoi…

46f9115

…d mutablePixels()

BasisImageConverter: use two importer instances for better legibility

801e501

mosra added the changelog mention added label Oct 1, 2024

mosra closed this Oct 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basis plugins: support UASTC HDR #145

Basis plugins: support UASTC HDR #145

pezcode commented Sep 11, 2024 •

edited

Loading

mosra left a comment

mosra Sep 11, 2024

pezcode commented Sep 27, 2024

codecov bot commented Sep 28, 2024 •

edited

Loading

mosra left a comment

mosra left a comment

pezcode commented Sep 29, 2024

mosra commented Sep 29, 2024 •

edited

Loading

mosra commented Oct 1, 2024

Basis plugins: support UASTC HDR #145

Basis plugins: support UASTC HDR #145

Conversation

pezcode commented Sep 11, 2024 • edited Loading

Todos:

mosra left a comment

Choose a reason for hiding this comment

mosra Sep 11, 2024

Choose a reason for hiding this comment

pezcode commented Sep 27, 2024

codecov bot commented Sep 28, 2024 • edited Loading

Codecov Report

mosra left a comment

Choose a reason for hiding this comment

mosra left a comment

Choose a reason for hiding this comment

pezcode commented Sep 29, 2024

mosra commented Sep 29, 2024 • edited Loading

mosra commented Oct 1, 2024

pezcode commented Sep 11, 2024 •

edited

Loading

codecov bot commented Sep 28, 2024 •

edited

Loading

mosra commented Sep 29, 2024 •

edited

Loading