Add support for custom bitmap format

[DanRStevens]

For some reason I thought this project already had support for the custom bitmap format used for tilesets, particularly since the OP2MapImager project can image map files. Though after checking both projects, it seems we don't have such code.

I suppose we can go back to the OP2Editor project and base something off of the code there. The relevant method is CTileSet::LoadTileSet: https://github.com/OutpostUniverse/OP2Editor/blob/e2d7b1a4cc75c88e7ec8b155784af2f517c6e4e4/CTileSet.cpp#L349

I figure the basic loading code should go into OP2Utility. The same code could be re-used by both the OP2MapImager and OP2BitmapConverter projects.

[Brett208]

In particular from CTileSet::LoadTileSet

// Adjust palette color component positions (RGB <-> BGR)
for (i = 0; i < bmInfo->bmiHeader.biClrUsed; i++)
{
    // Swap the Red and Blue components
    temp = bmInfo->bmiColors[i].rgbRed;
    bmInfo->bmiColors[i].rgbRed = bmInfo->bmiColors[i].rgbBlue;
    bmInfo->bmiColors[i].rgbBlue = temp;
}

[DanRStevens]

Ahh yes, that's an important point.

Could probably update that code to use std::swap.

Unless perhaps we have two different structs for RGB and BGR style color structs, and have a conversion method that copies like components in a way that naturally reverses the order. Though I may be overthinking things.

[Brett208]

Good idea to use std::swap. I was thinking about adding a function to the Color struct

// Invert color values. RGB <-> BGR
void Color::Invert()
{
    // use std::swap
}

---

So a standard Bitmap uses a file signature of BM. The Outpost 2 file format uses a file signature of PB.

@DanRStevens, do you know if the PB file format is a fully custom format written to support Outpost 2? A very brief google search seemed to indicate it wasn't recognized by others. I was reviewing the notes we have on the PB structure and it appears it contains a lot of the building blocks of a bitmap file format but leaves out assumed keys and values.

I am thinking we should do the following:

1. Check if the file signature is BM or PB
2. If BM, use standard BM logic already imbeded in OP2Utility
3. If PB, populate a BM standard file in memory using a custom parser

With this approach we can treat all bitmaps after parsing as standard BM files. It will keep the rest of the code more portable and easier to read by someone familiar with bitmaps. The notes look good and coupled with the example code mentioned earlier in this thread, it should not be difficult to write the parser.

[DanRStevens]

I agree with this approach. If there's a way to do a peak read, where 2 bytes are read, without advancing the stream, you can use that to determine which parsing method to try. You can then hand off the stream one of the two parsers and it can parse the whole thing, including the "BM" tag, or the longer "PBMP" tag. The more specific parse code can throw an exception if the tag doesn't match expectation.

I'm not sure the stream classes support peak reads. Though if you can copy a stream, which include the current stream offset, it may be possible to do the peak by reading from the copy, and then effectively reset the stream by passing the original stream into the particular parse method.

[Brett208]

I have OP2Utility loading well0001 into a standard bitmap file. I used the existing Bitmap::WriteIndexed to save it to disk as a standard bitmap after loading and everything looked well formed to my eye.

I'm trying to polish the code some more and then see what portions can be unit tested. Would like to push a PR by the end of the day but may slip into the weekend. I haven't been committing as I go, so would like to will spend some time trying to commit it in useful chunks instead of one monster commit to aide in review.

It looks like supporting writing back into the custom format may be fairly trivial to add based on the work completed so far I've locally on my machine. This would allow for nice unit testing since you can 'round trip' write and then read the format and if the memory compare is the same, it is a good indicator everything is working correctly.

The documentation we have on the custom tileset format is excellent. I wrote all the code to read the file into memory before testing, and everything parsed correctly on first try. Very pleasant.

[DanRStevens]

That's very good news. Glad to hear things have been going smoothly.

I like the idea of using round trip testing for the unit test code. I've often found that to be pretty helpful for testing. Would also be kind of neat to support converting to the OP2 specific bitmap format, even if such a feature didn't get much use.

[Brett208]

Importing the custom tilesets is now possible but a few loose ends remain:

• [X] Review error messaging formatting
  • Possible solutions in NAS2D library, under StringUtils.h
• [X] Add ability to allow importing a tileset agnostic from source (bitmap or custom tileset format)

Possible in a future issue or in this issue

• [X] Allow writing a bitmap to custom tileset format
• [X] Add 'round trip' write then read unit tests

[Brett208]

Finished all of the tasks except reviewing the error messaging format.

After more thorough testing, there is a disparity where the scan lines are being reversed. It can be fixed by multiplying the height by -1 after loading the custom tileset. Need to look a little closer to determine the best way to fix this.

[DanRStevens]

A number of graphics utilities always write bitmaps as top-down images. Likely they convert a bottom-up bitmap during loading, and always use the top-down format internally. If a bottom-up image is detected, the scanlines in the pixel data can be reversed, and then a corresponding top-down height value can be stored internally.

I would be tempted to assume an internal format with a positive height. There would be no need to sprinkle the code with std::abs, and the math would all work out as expected. However, the height would need to be converted to a negative value during save, since top-down Windows bitmap data is written with a negative height value. An alternative might be to assume the height value is always negative internally, so it can be saved easily when writing, though that would require some adjustment when calculating pixel data size.

Considering Windows bitmap format is the only format I know of that defaults to bottom up bitmaps, or where a negative height value is needed for a top-down bitmap, I'd be tempted to use positive heights for the more common top-down format, and consider the negative height adjustments, or possible bottom-up pixel data to be a serialization format specific concern, which would be adjusted during saving and loading.

[Brett208]

I sorted through the different issues trying to use a template function stack for generic read error messaging. It was a combination of the template not understanding how to process our Tag class in addition to a couple of mixed types being passed into the functions. The C++ error messages were not particularly helpful in debugging (partially because I am not well familiar with template programming).

The function stringFrom below is basically an exact copy from StringUtils.h in NAS2D. I just added a block to deal with the Tag class.

@ldicker83 or @DanRStevens what are your thoughts if I paste this function and unit tests into OP2Utility? Do you want some sort of attribution? I could probably solve a little bit less robustly and not wholesale copy if it is a concern.

template<typename T>
std::string stringFrom(T value)
{
    if constexpr (std::is_same_v<T, std::string> || std::is_same_v<T, const char*>) {
        return value;
    }
    else if constexpr (std::is_same_v<T, bool>) {
        return value ? "true" : "false";
    }
    else if constexpr (std::is_same_v<T, Tag>) {
        return value;
    }
    else {
        return std::to_string(value);
    }
}

template<typename T>
std::string formatReadErrorMessage(std::string propertyName, T value, T expectedValue)
{
    return "Tileset property " + propertyName + " reads " + stringFrom(value) + ". Expected a value of " + stringFrom(expectedValue) + ".";
}

template<typename T>
void throwReadError(std::string propertyName, T value, T expectedValue)
{
    throw std::runtime_error(formatReadErrorMessage(propertyName, value, expectedValue));
}

[DanRStevens]

I'm fine with copy/paste from another OPU project.

I'm not entirely clear on why there is a special case for Tag. I would be hesitant to put support for custom types into a rather generic method like that, which deals largely with primitive and really common standard library types. It might make more sense to convert a Tag to a std::string before passing it to the function. Or perhaps the type could support auto conversion to std::string by using a type cast overload:

operator std::string() const { ... }

https://en.cppreference.com/w/cpp/language/cast_operator

[Brett208]

The cast overload already exists

// Helper method to allow easily converting to std::string
operator std::string() const {
    return std::string(text.data(), text.size());
}

However, the template function will not recognize it as a string when called, so it will attempt to apply std::to_string to the Tag class which causes the error.

I think we could write the following, but I haven't tested yet:

throwReadError("FileSignature", std::string(tag), std::string(fileSignatureTag);

The conversion could be handled in formatReadErrorMessage instead. Or it could be handled each time throwReadError is called.

I was trying to not write the conversion each time the function is called, as I thought it would add a bit of clarity to hide the 'cast', but it only affects a couple of locations, so probably isn't a big deal to write out the conversion either at the point of calls for throwReadError.

[DanRStevens]

You probably want to avoid putting std::string(tag) inside of any template methods, but at the call point of a template method should be fine.

---

It would be reasonable to upgrade the stringFrom method to handle objects with an operator std::string() const member, which would convert to string by calling that method. I think that would nice solve the problem you're trying to handle.

[Brett208]

@DanRStevens I really like the point you bring up about checking if implicit conversion is allowed in StringFrom. I hadn't thought about this even being possible. After doing some research I came up with the solution below which compiles without complaining about ingesting the Tag class, although I haven't directly tested the results.

template<typename T>
std::string stringFrom(T value)
{
    if constexpr (std::is_same_v<T, std::string> || std::is_same_v<T, const char*>) {
        return value;
    }
    else if constexpr (std::is_convertible<T, std::string>::value) {
        return value;
    }
    else if constexpr (std::is_same_v<T, bool>) {
        return value ? "true" : "false";
    }
    else {
        return std::to_string(value);
    }
}

If this looks on the right track, I'll push this code and try to update the unit tests to include this use case.

I'm glad we worked through the possibilities or I would have never gotten here on my own.

[DanRStevens]

That looks like it should work. Nicely done.

[Brett208]

Addressing comment earlier in thread about handling negative height. There appears to be some inconsistencies internal to OP2Utility's BitmapFile::CreateIndexed where it assumes a positive height but the underlying code assumes a negative or positive height is allowed. This probably needs addressed before this issue can be closed out.

The BitmapFile code we have home-rolled does not yet well address how to switch between a negative and positive height. It just assumes you enter the data correctly and never want to switch the sign of the height value (or that you would manually switch the scan lines as needed). This sort of complication was something I was hoping to avoid and is pointing to the advantage of bringing in an external bitmap library instead of home rolling one. I guess we are this far in and need to just figure a solution out. Will look at addressing in a separate issue how to handle inverting the scan lines. If we are going to support saving back into the custom format, we have to address the fact that the user could load a bitmap with a negative or positive height and Outpost 2 would need that translated. But this would be needed if the Sprite code ever finished maturing anyways to allow saving sprites I think.

[Brett208]

With the merging of #380 I think this issue is ready for closure. I'd like to do a bit more testing converting the original wells and loading files into Outpost 2 post OP2Utility conversion before closing. May be able to complete later today.