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

[WIP] DDS import #7

Merged
merged 9 commits into from Sep 3, 2015

Conversation

2 participants
@Squareys
Contributor

Squareys commented Aug 9, 2015

Hi @mosra !

As promissed, here is the WIP Pullrequest. The tests currently still fail and print two identical arrays (for compressed images). For uncompressed, the size of the imagedata seems larger than it should, did not figure out why that is yet (the size calculated correctly in the importer).

Current features/TODOs:

  • Uncompressed RGB
  • Uncompressed RGBA
  • DXT(1|3|5) Compressed images
  • Mipmaps (untested for compressed images)
  • Cubemaps (untested)
  • 3D Textures

Greetings,
Squareys :)

@mosra

This comment has been minimized.

Owner

mosra commented Aug 9, 2015

Okay, first things first: 👍

The tests are printing the same values but failing because Containers::Array does not have operator== implemented and so just pointer values are compared. You have to do something like this:

#include <Corrade/TestSuite/Compare/Container.h>

...

CORRADE_COMPARE_AS(image->data(), Containers::ArrayView<const char>(pixels),
    TestSuite::Compare::Container);

For uncompressed, the failing test is my bug mosra/magnum#104 (yes, you are computing the size correctly).

colorFormat.uncompressed = ColorFormat::BGR;
components = 3;
} else if (ddsh.ddspf.rgbBitCount == 8) {
// colorFormat = ColorFormat::Luminance; //TODO equivalence in Magnum?

This comment has been minimized.

@mosra

mosra Aug 9, 2015

Owner
#ifndef MAGNUM_TARGET_GLES2
ColorFormat::Red;
#else
ColorFormat::Luminance;
#endif
};
inline UnsignedInt clampSize(UnsignedInt size) {
return (size <= 0) ? 1 : size = 1;

This comment has been minimized.

@mosra

mosra Aug 9, 2015

Owner

Just Math::max(size, 1) might be enough ...

This comment has been minimized.

@Squareys

Squareys Aug 9, 2015

Contributor

Yeah, I tried that, but my compiler complained and I didn't have the patience to deal with it yet :) I will try again later.

This comment has been minimized.

@mosra

mosra Aug 9, 2015

Owner

Oh, right... Math::max(size, 1u) may solve the ambiguous type deduction.

This comment has been minimized.

@Squareys

Squareys Aug 10, 2015

Contributor

@mosra Ah! Perfect, thanks! Will try this later :)

@Squareys Squareys force-pushed the Squareys:dds-importer branch 2 times, most recently from 03f7a42 to d4e00f2 Aug 10, 2015

@Squareys

This comment has been minimized.

Contributor

Squareys commented Aug 10, 2015

For uncompressed, the failing test is my bug mosra/magnum#104 (yes, you are computing the size correctly).

Alright, cool :) Tests for compressed images run through now.

/* shrink to next power of 2 */
w = Math::max(w >> 1, 1u);
h = Math::max(h >> 1, 1u);
d = Math::max(d >> 1, 1u);

This comment has been minimized.

@mosra

mosra Aug 10, 2015

Owner

Just a suggestion, but I think that using Vector3i/Vector3ui instead of separate width/height/depth would reduce the verbosity and amount of function parameters quite a bit. Math functions will work without change (as expected), including Math::max() and operator >>.

This comment has been minimized.

@Squareys

Squareys Aug 10, 2015

Contributor

Sure, will do. That is a remnant of the code I started with.

UnsignedInt DdsImporter::doImage2DCount() const { return _imageData2D.size(); }
std::optional<ImageData2D> DdsImporter::doImage2D(UnsignedInt id) {
if (id < _imageData2D.size()) {

This comment has been minimized.

@mosra

mosra Aug 10, 2015

Owner

This check is not needed (the AbstractImporter does this already using information from doImage2DCount()), similarly for the 3D image below.

void DdsImporter::doOpenFile(const std::string& filename) {
/* let doOpenData handle it */
AbstractImporter::doOpenFile(filename);
}

This comment has been minimized.

@mosra

mosra Aug 10, 2015

Owner

Completely removing the doOpenFile() override will have the same effect. ;)

(Yes, I had this in the PngImporter too, it was ugly, it's removed now and I'm sorry about that)

This comment has been minimized.

@Squareys

Squareys Aug 10, 2015

Contributor

Alright, good to know :D

This comment has been minimized.

@Squareys

Squareys Aug 10, 2015

Contributor

Uhm, really? All tests fail now...

colorFormat.uncompressed = ColorFormat::BGR;
components = 3;
} else if (ddsh.ddspf.rgbBitCount == 8) {
#ifndef MAGNUM_TARGET_GLES2

This comment has been minimized.

@mosra

mosra Aug 10, 2015

Owner

You can indent the preprocessor stuff too, it's not a portability problem as far as I can tell.

@Squareys Squareys force-pushed the Squareys:dds-importer branch from df19c00 to 5946c48 Aug 10, 2015

Squareys added some commits Aug 2, 2015

DdsImporter: Add initial CMake projects and skeleton.
Signed-off-by: Squareys <Squareys@googlemail.com>
DdsImporter: Add tests and test data.
Signed-off-by: Squareys <Squareys@googlemail.com>
DdsImporter: Implement 2D (RBG/RGBA/DXT(1|3|5)) image loading.
Mipmaps are supported. Uncompressed RGBA and Cubemap loading are untested.
Missing 3D Image support.

Signed-off-by: Squareys <Squareys@googlemail.com>
DdsImporter: Add test and test data for volume textures.
Signed-off-by: Squareys <Squareys@googlemail.com>
DdsImporter: Implement support for volume textures.
Also: Fix data not being cleared when loading the next file.

Signed-off-by: Squareys <Squareys@googlemail.com>
DdsImporter: Use Vector3i for image dimensions.
Signed-off-by: Squareys <Squareys@googlemail.com>
DdsImporter: Remove unnecessary code.
Signed-off-by: Squareys <Squareys@googlemail.com>

@Squareys Squareys force-pushed the Squareys:dds-importer branch 2 times, most recently from f297226 to c78495d Aug 14, 2015

ImageDataOffset& dataOffset = _imageData[id];
// copy image data
Containers::Array<char>* data = new Containers::Array<char>(dataOffset._data.size());

This comment has been minimized.

@mosra

mosra Aug 19, 2015

Owner

Why the new here? (also you are leaking the data)

Similarly in doImage3D().

This comment has been minimized.

@Squareys

Squareys Aug 20, 2015

Contributor

ImageData2D doesn't data, right? Don't I need to make sure it stays around? For the uncompressed case, I'm moving data, which would, of course make the new superfluous. There does not seem to be a constructor to move data into ImageData2D when uncompressed.

This comment has been minimized.

@mosra

mosra Aug 20, 2015

Owner

Ahh, right, sorry, I missed the static_cast<void*>(data->data()) line. I'm now doing a rework of image classes (in order to fix mosra/magnum#104) and then there of course will be a constructor that moves the data into uncompressed ImageData2D.

Currently you can do just data->release().release() to "disown" the array from image and then disown the data pointer from array. Then the new is not needed.

This comment has been minimized.

@Squareys

Squareys Aug 20, 2015

Contributor

and then there of course will be a constructor that moves the data into uncompressed ImageData2D.

Awesome!

data->release().release()

Okay, thanks, will do so.

/**
* Flags to indicate which members of a DdsHeader contain valid data.
*/

This comment has been minimized.

@mosra

mosra Aug 19, 2015

Owner

These enums and the DdsHeader struct are not part of the public API, so no need for Doxygen comments (just /* ... */ is enough).

This comment has been minimized.

@Squareys

Squareys Aug 20, 2015

Contributor

Alright, will do.

/* load all surfaces for the image (6 surfaces for cubemaps) */
const UnsignedInt numImages = (isCubemap) ? 6 : 1;
size_t offset = 128;

This comment has been minimized.

@mosra

mosra Aug 19, 2015

Owner

Can the 128 be represented using some calculation? E.g. sizeof(DdsHeader) + something? Ideally in combination with static_assert() that it it really equals to 128.

This comment has been minimized.

@Squareys

Squareys Aug 20, 2015

Contributor

Yup, sure, sizeof(DdsHeader) + 4.

@mosra

This comment has been minimized.

Owner

mosra commented Aug 19, 2015

Sorry for the late reply -- looks great, really, I'm amazed :)

Commenting inline about some minor things, other than that ready to merge I think?

@Squareys

This comment has been minimized.

Contributor

Squareys commented Aug 20, 2015

I think there are some if (s around, I will generally check for code style and take care of the issues you pointed out. The data memleak will take some more discussion, though.

Sorry for the late reply

No problem! I figured you were on vacation ;)

Squareys added some commits Aug 14, 2015

DdsImporter: Refactor to accomodate for more usecases.
Primary usecase is load one image from all. I would be a shame to have to
wait for all images being possibly converted from BGRA to RGBA when loading
only one single image.

Also, calling `imageData2D/3D` twice was not possible up to now.

The strategy now is to do everything which has to be done at least once
anyway in `doImageData()` and create the images themselves from stored
offsets lazily in `doImageData*D()`.

Signed-off-by: Squareys <Squareys@googlemail.com>
DdsImporter: Code cleanup and doc
Use simple `/* */` istead of doxygen comments for private files, some
minor code formatting fixes and alot of documentation.

Signed-off-by: Squareys <Squareys@googlemail.com>

@Squareys Squareys force-pushed the Squareys:dds-importer branch from ebc2dbf to 131bf3e Aug 20, 2015

@Squareys

This comment has been minimized.

Contributor

Squareys commented Aug 20, 2015

@mosra Here you go, this is pretty much finished from my side. If there are any bigger issues, I'd be glad to take care of them, for minor issues I will look at your cleanup commit once you merged ;)

@mosra

This comment has been minimized.

Owner

mosra commented Aug 20, 2015

Great, thanks a lot!

I merged it locally and made some modifications so all the tests pass now, but it depends on not yet published Magnum changes, so I'll push everything at once... another crazily large change :)

69 files changed, 3692 insertions(+), 2576 deletions(-)

@Squareys

This comment has been minimized.

Contributor

Squareys commented Aug 20, 2015

Awesome, thank you! :D

@mosra mosra referenced this pull request Aug 28, 2015

Closed

Next release #108

34 of 34 tasks complete

@mosra mosra merged commit 131bf3e into mosra:master Sep 3, 2015

1 check failed

continuous-integration/appveyor AppVeyor build failed
Details
@mosra

This comment has been minimized.

Owner

mosra commented Sep 3, 2015

Merged (finally). Thank you.

However, my additions are not yet completely done, so the tests are still failing (in current master). Will be fixed as part of mosra/magnum#104.

@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