OutpostUniverse / OP2Utility

C++ library for working with Outpost 2 related files and tasks.
MIT License
4 stars 0 forks source link

Eliminate need for IndexedBmpWriter::VerifyPixelsContainedInPalette #201

Closed Brett208 closed 5 years ago

Brett208 commented 5 years ago

Currently we have a function called VerifyPixelsContainedInPalette. Basically, it determines if any pixel is outside the range of palette entries. I was thinking that if we force the palette to always be full based on the bit count, then it is impossible for the indexed pixel to be outside of its range.

We could:

I've read in a couple of places that forcing a palette to be 'at capacity' based on bit count is a common practice when writing BMPs.

The other option would be to leave the function in and then add support for properly setting header values if the palette is not full, which we are currently not doing. So I guess there is a bug in the code right now.

Thanks, -Brett

DanRStevens commented 5 years ago

This might depend on what the user of the library wants. Even if you were to force a full sized palette during write, you might still want to validate data you read. I have nothing against making that method publicly available. I think placement of the method is the bigger concern. It's not really related to writing of bitmap data, but rather to validating input data.

The curious question, is what to do if an input source has out of range pixels. Should the pixels be cleared? Should they be mapped to 0 by extending the palette? Should it be left to the library user to decide?

More curiously, do such pixels contain hidden information such as in Steganography? I suppose something similar can be done in the palette data itself, within unused entries, or duplicate entries resulting in different index values for the same color, or even in the padding bytes of the pixel data. Sadly though, we're not concerned with steganography here :cry:


I think it's fine if we remove the method, though it might be nice to keep it available. Generally most validation is done on input data, such as loading a file, or setting properties on an object. If we have the method available on a generic bitmap class, that could be a good place for it.

Brett208 commented 5 years ago

Good point that while expanding the palette will ensure the file is well formed, it still doesn't make sense to allow pixels outside the pre-expanded palettes value range. Because the function in its current form is poorly written, I'm going to close this issue when the PR is merged. A new issue was opened to re-introduce this function when it is ready.

-Brett