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

DdsImporter: (most) uncompressed Dxt10 formats support #34

Merged
merged 2 commits into from Jun 25, 2017

Conversation

Projects
None yet
3 participants
@Squareys
Contributor

Squareys commented Jun 23, 2017

Hi @mosra,

here is the work in progress pullrequest for the DXT10 uncompressed formats support in the DdsImporter, which I require for an upcoming magnum example.

Cheers, Jonathan.

TODO:

  • (Instanced) Tests for the new supported file formats
  • Test for too short files (which cannot contain DXT10 extended header) and fixes in Importer
  • Test for unsupported dds compression type (DXT4)
  • Test for unsupported DXT10 format
  • Update documentation to advertise new formats
case DxgiFormat::R8G8B8A8UNorm:
return {PixelFormat::RGBA, PixelType::UnsignedByte};
//case DxgiFormat::R8G8B8A8UNormSRGB:
//return {PixelFormat::SRGBA, PixelType::Byte}; // TODO: No such thing as PixelFormat::SRGBA

This comment has been minimized.

@mosra

mosra Jun 23, 2017

Owner

There is sRGB + Alpha texture format, but somehow there's no corresponding pixel format for this. Weird, I'll investigate.

This comment has been minimized.

@mosra

mosra Jun 24, 2017

Owner

Hmm. Apparently there's no way to explicitly mark the pixel data as sRGB and in this case it would be just RGBA. Numerous sources (e.g. Apple or e.g. the OpenGL ES 3.2 spec, table 8.2 on page 147) say that the matching pixel format for sRGB(Alpha) is just RGB(A). So I guess that settles it.

This is a bit bad, because only input textures that actually are in sRGB should be uploaded to sRGB textures. OTOH, almost all 8-bit color data already are in sRGB, unless someone went an extra step and kept them linear.

More info (huh, I apparently went through all of this already in the past): mosra/magnum#125

This comment has been minimized.

@Squareys

Squareys Jun 25, 2017

Contributor

Would adding a simple flag to PixelStorage make sense maybe?

This comment has been minimized.

@mosra

mosra Jun 25, 2017

Owner

PixelStorage is meant to just wrap the data layout parameters, in the extent that OpenGL supports. sRGB doesn't really fit there. I will think about this.

case DxgiFormat::R8UInt:
return {PixelFormat::RedInteger, PixelType::UnsignedByte};
case DxgiFormat::A8UNorm:
//return {PixelFormat::Alpha, PixelType::UnsignedByte}; //TODO:Alpha? Using same as R8Unorm

This comment has been minimized.

@mosra

mosra Jun 23, 2017

Owner

GL treats both as single-channel (PixelFormat::Red) without any semantic distinction. There was Luminance, Alpha and LuminanceAlpha once, but were deprecated in favor of those that don't have any implicitly associated meaning. The app should know what given texture contains.

I would make it the same as R8Unorm but put a detailed chapter into the docs about such mapping "exceptions".

DdsImporter importer;
CORRADE_VERIFY(!importer.openFile(Utility::Directory::join(DDSIMPORTER_TEST_DIR, "unknown_compression.dds")));
CORRADE_COMPARE(out.str(), "Trade::DdsImporter::openData(): unknown compression DX10\n");
}

This comment has been minimized.

@mosra

mosra Jun 23, 2017

Owner

This would make the "unknown compression" error branch uncovered. I think we should still check this to make sure we handle corrupted files well. Is it possible to craft such a file that fits the criteria?

This comment has been minimized.

@Squareys

Squareys Jun 23, 2017

Contributor

Yes, of course :)

This comment has been minimized.

@Squareys

Squareys Jun 25, 2017

Contributor

I will use DXT4 fourcc instead

DdsImporter importer;
CORRADE_VERIFY(importer.openFile(Utility::Directory::join(DDSIMPORTER_TEST_DIR "/Dxt10TestFiles", "2D_R32G32_FLOAT.DDS")));
CORRADE_VERIFY(importer.openFile(Utility::Directory::join(DDSIMPORTER_TEST_DIR "/Dxt10TestFiles", "2D_R8G8B8A8_UINT.DDS")));

This comment has been minimized.

@mosra

mosra Jun 23, 2017

Owner

btw., the uppercase extension would make this broken on Linux -- the files have lowercase extension as far as I see ;)

This comment has been minimized.

@Squareys

Squareys Jun 23, 2017

Contributor

Yes, this is fixed locally. The windows tools output upper case, but I didn't like that ;)

@Squareys Squareys changed the title from DdsImporter: (most) uncompressed Dxt10 formats support to [WIP] DdsImporter: (most) uncompressed Dxt10 formats support Jun 25, 2017

@Squareys

This comment has been minimized.

Contributor

Squareys commented Jun 25, 2017

@mosra I started adding an instanced test. I am unsure how much sense it would make to compare data for every test file (after all, it is loaded with the same code for every of the formats, so I think checking the Pixel Type and Format should be sufficient?)
I would still do the data comparison for at least one of the file formats, of course.

The dxt10unsupportedFormat is still missing a file (coming soon).

@mosra

This comment has been minimized.

Owner

mosra commented Jun 25, 2017

Right, if it's loaded the same every time, then I would verify the loaded binary data just once, along with all the other properties like width, height, pixel storage, preferably with some more complex case (non-square dimensions, some alignment padding, if that's supported etc.).

@coveralls

This comment has been minimized.

coveralls commented Jun 25, 2017

Coverage Status

Coverage decreased (-0.8%) to 86.44% when pulling 7571ca7 on Squareys:dds-dxt10 into 36913e9 on mosra:master.

@coveralls

This comment has been minimized.

coveralls commented Jun 25, 2017

Coverage Status

Coverage decreased (-0.8%) to 86.44% when pulling ece7a0e on Squareys:dds-dxt10 into 36913e9 on mosra:master.

@Squareys Squareys changed the title from [WIP] DdsImporter: (most) uncompressed Dxt10 formats support to DdsImporter: (most) uncompressed Dxt10 formats support Jun 25, 2017

@mosra

Almost there :) Great work!

&DdsImporterTest::useTwice});
addInstancedTests({&DdsImporterTest::dxt10formats2d}, 46);
addInstancedTests({&DdsImporterTest::dxt10formats3d}, 12);

This comment has been minimized.

@mosra

mosra Jun 25, 2017

Owner

Could you use an enum constant for the size and then also use the same constant below there in the files struct? Talking from experience, it will prevent annoying problems when extending/shortening the list later. Details in the docs.

This comment has been minimized.

@Squareys

Squareys Jun 25, 2017

Contributor

Even better... I am using sizeof(that constexpr array in the anonymous namespace) now ;)

This comment has been minimized.

@mosra

mosra Jun 25, 2017

Owner

that doesn't work like that :P

the only way is std::size() and that is since C++17

This comment has been minimized.

@Squareys

Squareys Jun 25, 2017

Contributor

Which uncovered that I still had the 3D file names in the 2d files array 😅

This comment has been minimized.

@Squareys

Squareys Jun 25, 2017

Contributor

(Okay, I understand how that was a bad idea, works with the constant now)

This comment has been minimized.

@Squareys

Squareys Jun 25, 2017

Contributor

Done ✔️

@@ -241,6 +254,153 @@ void DdsImporterTest::dxt5() {
TestSuite::Compare::Container);
}
void DdsImporterTest::dxt10formats2d() {
static struct TestFile {

This comment has been minimized.

@mosra

mosra Jun 25, 2017

Owner

I fear that the static keyword implies some unnecessary code generation (mutexes for thread safety). I'm completely okay with this being a constexpr anonymous struct in an anonymous namespace outside of a function. Details how that might look like again in the docs (sorry) ;)

This comment has been minimized.

@Squareys

Squareys Jun 25, 2017

Contributor

Done ✔️

{"3D_R32G32B32_UINT.dds", PixelFormat::RGBInteger, PixelType::UnsignedInt},
{"3D_R32G32_FLOAT.dds", PixelFormat::RG, PixelType::Float},
};
const TestFile& file = files[testCaseInstanceId()];

This comment has been minimized.

@mosra

mosra Jun 25, 2017

Owner

could be just const auto& and then you can drop the struct name

This comment has been minimized.

@Squareys

Squareys Jun 25, 2017

Contributor

Oh, magic! learned something new :D

- `R8G8B8_(TYPELESS|UINT|SINT|UNORM|SNORM)`
- `R8G8_(TYPELESS|UINT|SINT|UNORM|SNORM)`
- `R8_(TYPELESS|UINT|SINT|UNORM|SNORM)`
- `A8_UNORM` (Loaded as @ref PixelFormat::Red)

This comment has been minimized.

@mosra

mosra Jun 25, 2017

Owner

👍

Can you mention the sRGB thing here as well?

This comment has been minimized.

@Squareys

Squareys Jun 25, 2017

Contributor

Ah, I did indeed forget to list that :D Good catch 👍
Is listing enough or should I mention it is loaded without a hint at the sRGB-ness?

This comment has been minimized.

@mosra

mosra Jun 25, 2017

Owner

should I mention it is loaded without a hint at the sRGB-ness?

yes please

This comment has been minimized.

@Squareys

Squareys Jun 25, 2017

Contributor

Done ✔️

return {PixelFormat::RGBAInteger, PixelType::UnsignedByte};
case DxgiFormat::R8G8B8A8UNorm:
case DxgiFormat::R8G8B8A8UNormSRGB:
return {PixelFormat::RGBA, PixelType::UnsignedByte}; // TODO: Propagate sRGB property

This comment has been minimized.

@mosra

mosra Jun 25, 2017

Owner

Can this be a /** @todo */ to have Doxygen pick it up?

This comment has been minimized.

@Squareys

Squareys Jun 25, 2017

Contributor

Damn, vim doesn't highlight it anymore now 😛

This comment has been minimized.

@mosra

mosra Jun 25, 2017

Owner

you are holding it wrong ;)

I have a list of keywords to highlight in my editor, I bet vim has that too.

This comment has been minimized.

@Squareys

Squareys Jun 25, 2017

Contributor

Done ✔️

{"3D_R32G32B32_UINT.dds", PixelFormat::RGBInteger, PixelType::UnsignedInt},
{"3D_R32G32_FLOAT.dds", PixelFormat::RG, PixelType::Float},
};
const TestFile& file = files[testCaseInstanceId()];

This comment has been minimized.

@mosra

mosra Jun 25, 2017

Owner

Same here, please :)

This comment has been minimized.

@Squareys

Squareys Jun 25, 2017

Contributor

Done ✔️

{"3D_R32G32B32_FLOAT.dds", PixelFormat::RGB, PixelType::Float},
{"3D_R32G32B32_SINT.dds", PixelFormat::RGBInteger, PixelType::Int},
{"3D_R32G32B32_UINT.dds", PixelFormat::RGBInteger, PixelType::UnsignedInt},
{"3D_R32G32_FLOAT.dds", PixelFormat::RG, PixelType::Float},

This comment has been minimized.

@mosra

mosra Jun 25, 2017

Owner

Just a heads-up:

  • you either need to list all those files explicitly in CMakeLists.txt so they are properly picked up by Android and Emscripten (there is already a list, so just extend it), but I fear this might be too long to fit on the compiler command-line for Emscripten
  • or compile them in as a resource

This comment has been minimized.

@Squareys

Squareys Jun 25, 2017

Contributor

Done ✔️

@coveralls

This comment has been minimized.

coveralls commented Jun 25, 2017

Coverage Status

Coverage decreased (-0.2%) to 87.002% when pulling ee404f4 on Squareys:dds-dxt10 into 36913e9 on mosra:master.

@Squareys Squareys force-pushed the Squareys:dds-dxt10 branch from c368a8f to b2bb778 Jun 25, 2017

@coveralls

This comment has been minimized.

coveralls commented Jun 25, 2017

Coverage Status

Coverage decreased (-0.2%) to 87.067% when pulling c368a8f on Squareys:dds-dxt10 into 36913e9 on mosra:master.

@coveralls

This comment has been minimized.

coveralls commented Jun 25, 2017

Coverage Status

Coverage decreased (-0.2%) to 87.067% when pulling b2bb778 on Squareys:dds-dxt10 into 36913e9 on mosra:master.

@Squareys Squareys force-pushed the Squareys:dds-dxt10 branch from b2bb778 to 3bc02e2 Jun 25, 2017

@coveralls

This comment has been minimized.

coveralls commented Jun 25, 2017

Coverage Status

Coverage decreased (-0.2%) to 87.067% when pulling 3bc02e2 on Squareys:dds-dxt10 into 36913e9 on mosra:master.

Squareys added some commits Jun 25, 2017

DdsImporter: Add support for most uncompressed DXT10 formats
Signed-off-by: Squareys <squareys@googlemail.com>
DdsImporter: Test support for uncompressed DXT10 formats
Signed-off-by: Squareys <squareys@googlemail.com>

@Squareys Squareys force-pushed the Squareys:dds-dxt10 branch from 3bc02e2 to c6a1a62 Jun 25, 2017

@coveralls

This comment has been minimized.

coveralls commented Jun 25, 2017

Coverage Status

Coverage decreased (-0.2%) to 87.067% when pulling c6a1a62 on Squareys:dds-dxt10 into 36913e9 on mosra:master.

@mosra mosra merged commit c6a1a62 into mosra:master Jun 25, 2017

2 of 3 checks passed

coverage/coveralls Coverage decreased (-0.2%) to 87.067%
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mosra

This comment has been minimized.

Owner

mosra commented Jun 25, 2017

Merged, thank you!

@mosra mosra added this to the 2018.02 milestone Feb 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment