Allow custom pitch when writing bitmaps #256
Conversation
This allows instantiating OP2BmpLoader without extracting op2_art.prt from the art.vol (volume)
Still need to look closely at 1 bit images to ensure they are forming fully correctly. Earlier it would just throw an error.
Better represents the fact that pitch does not have to be 4 byte intervals. Still provides a standard default value so user doesn't have to worry about calculating their own if they do not wish to.
|
Forgot to commit unit tests and out of time for now. Will try and fix tomorrow. -Brett |
…ault pitch interval
Interval implies a start value and end value. Granularity implies a minimum spacing for values.
…hub.com/OutpostUniverse/OP2Utility into Allow-Custom-Pitch-When-Writing-Bitmaps
| throw std::runtime_error("Unable to calculate a valid pitch based on height and pixel count"); | ||
| } | ||
|
|
||
| const std::size_t pitch = pixelCount / height; |
There was a problem hiding this comment.
This hack of mine is causing the unit tests to fail. They happen to be fairly robust for this small section of the code compared to others.
The next step is probably to figure out a proper bit twiddle or calculation that determines pitch without assuming it is a 4 byte interval. @DanRStevens, feel free to chime in if you have something up your sleeve.
-Brett
There was a problem hiding this comment.
Pitch is either assumed, or given. It's not generally calculated (other than in an assumption, such as rounding the scanline byte width up to a multiple of 4 bytes). The problem with trying to calculate it, is the data source could always have chosen a larger pitch than is necessary.
I find it a little non-intuitive to use pixelCount here. When I think pixel could, I generally assume it's simply width * height, which is not particularly useful for calculating pitch if you already have the width. Plus, pitch is a byte measurement, not a pixel measurement. The bitDepth needs to be accounted for.
There was a problem hiding this comment.
If you are wanting to work on this branch feel free. I'm not going to pick it up again for a long time.
-Brett
There was a problem hiding this comment.
I'm not sure I quite have the context to pick this one up right now.
| @@ -42,8 +42,9 @@ struct ImageHeader | |||
| void VerifyValidBitCount() const; | |||
| static void VerifyValidBitCount(uint16_t bitCount); | |||
|
|
|||
| std::size_t CalculatePitch() const; | |||
| static std::size_t CalculatePitch(uint16_t bitCount, int32_t width); | |||
| // Default pitch granularity is 4 bytes | |||
There was a problem hiding this comment.
I had intentionally left this as interval. Perhaps it is a regional language different but interval seems a better and easier to understand choice than granularity to me. Being a comment, it probably isn't worth arguing over now that it has been changed.
There was a problem hiding this comment.
A disagreement in terminology might signal a disagreement about what is intended here.
I would define an interval as having a start and end point. Granularity is about the minimum spacing of values.
Here the pitch is rounded to some multiple of a certain value. That is defining the granularity of the value. As in, values are a multiple of 4 bytes. Is does not define the length of an interval as 4 bytes. For example, the spacing between scanlines could easily be 12 bytes, even though the granularity of that value is 4 bytes.
More deeply, I'm not sure I would really consider pitch to be an interval. It's more of a spacing between scanlines. If pitch were an interval, I'd be uncertain how to define the start and end points.
Looking back at std::size_t CalculateDefaultPitch() const;, I'm actually a little uncertain what the comment is trying to tell me, or why it is important. Is it trying to tell me the value is a multiple of 4 bytes? From the example test code, that would seem to be the case.
I vaguely recall some requirement that pitch is a minimum of 4 bytes, though can be a larger multiple of it.
I might also consider the phrasing:
Default pitch is the scanline byte width rounded up to a multiple of 4 bytes
| @@ -62,8 +62,9 @@ void BitmapFile::ReadPalette(Stream::BidirectionalSeekableReader& seekableReader | |||
|
|
|||
| void BitmapFile::ReadPixels(Stream::BidirectionalSeekableReader& seekableReader, BitmapFile& bitmapFile) | |||
| { | |||
| std::size_t pixelContainerSize = bitmapFile.bmpHeader.size - bitmapFile.bmpHeader.pixelOffset; | |||
| BitmapFile::VerifyPixelSizeMatchesImageDimensionsWithPitch(bitmapFile.imageHeader.bitCount, bitmapFile.imageHeader.width, bitmapFile.imageHeader.height, pixelContainerSize); | |||
| const std::size_t pixelContainerSize = bitmapFile.bmpHeader.size - bitmapFile.bmpHeader.pixelOffset; | |||
There was a problem hiding this comment.
I think this might be more of a maximum upper bound, rather than an exact size. There may be other sections stored in the bitmap file, after the pixel data, before the end of the file.
|
I am attempting to pull some of the better ideas out of this branch and commit them as smaller, separate branches and then close this pull request. I think the custom pitch thrust itself went poorly, but there was some supporting code that I didn't want to toss when terminating the branch. |
This is a pretty extensive rollup of changes. I've been playing with extracting images from op2_art. These changes were a result of that work.
Doesn't need to be reviewed quickly.
-Brett