Remove map header from map data

OutpostUniverse / OP2Utility

C++ library for working with Outpost 2 related files and tasks.

MIT License

4 stars 0 forks source link

Remove map header from map data #228

Closed Brett208 closed 5 years ago

Brett208 commented 5 years ago

Also make MapReader and MapWriter static functions on MapData.

This one will probably take a while to fully read through. Data within the class MapData is still not fully encapsulated. I hope this is a good push in the right direction. It seemed to make MapData a little more complicated than I wanted it to.

If acceptable, this should close #223 and close #224.

-Brett

Brett208 commented 5 years ago

Ran out of time to review the bit twiddling this session. Will hopefully get to it soon.

I know this only partially encapsulates the class but would like to leave MapData in a partial status for now. Otherwise this branch will balloon to a big scope.

-Brett

DanRStevens commented 5 years ago

Ok, I've updated the Log2 code to use the bit twiddling algorithms, rather than floating point.

You may want to go over that code in MapWriter.cpp and update as the old code accepted a width of 0, while the bit twiddling code actively discounts 0 as a power of 2. This affects a couple of tests, which assume a default constructed MapData object has a valid width. I added in a small hack so the tests would pass with a width of 0, and commented out the simpler line that throws for widths of 0.

Edit: Oh, AppVeyor builds just failed because I didn't add the new source files to the project. Can you fix that?

Brett208 commented 5 years ago

What do you think of the name BitTwiddle.h/.cpp? It does convey that we are doing lower level bit work in the code but not really what that is for. I thought about MathBitTwiddle, but most all BitTwiddling is math related? Maybe this is an ok way to group the code, dunno.

Brett208 commented 5 years ago

Just tested by compiling in x64 release mode, loading eden01.map, then saving eden01.map as a new file. The new version loaded fine in Outpost 2 and appeared to be the same size on disk.

2 small comments above. Whether they are addressed or not, I'm okay with merging.

-Brett

DanRStevens commented 5 years ago

I think we're basically in a good state to merge now.

There are three minor things on my mind that we may or may not change:

Remove the hardcoded 0x1010 constant within an error message in MapReader.cpp.
Use CurrentMapVersion when creating new maps or writing maps to disk (or maybe it should convert after loading in case it's an older map).
The name BitTwiddle.h/.cpp does kind of suck. I don't think adding "Math" really helps, for the reasons you've stated. I couldn't think of anything better.

Again, all optional updates at this point.

Brett208 commented 5 years ago

Okay resolved the first two mentioned issues. Will punt on BitTwiddle name, possibly forever. Thanks for the quality reviews. Merging shortly.