Better design for representing OP2 Maps in OP2Utility

[Vagabond]

Hey everyone,

Currently OP2Utility only has the ability to read a map. I'm looking at expanding this to add a save function so it can be used with the mapper. Currently the MapData constructor is responsible for loading a map via filename or stream.

I'm thinking for the big picture strategy:

• struct MapData - A data transfer object (DTO) that contains the data for a map and simple get/set functions if appropriate.
• class MapReader - Responsbile for reading a saved game or map into the struct MapData. This code is already written and just needs to be moved out of the MapData struct into a separate class.
• class MapWriter - Responsbile for writing a map to disk. (I'm not planning to support writing a saved game)

In the long term we will probably want a couple of functions that manipulate the MapData after it is loaded in more complex ways. For example, since tilesets are stored in MapData, you might want a function that adds a new tileset with default settings to the map. This could be a function attached to the MapData struct or it could be standalone function that works on the MapData, keeping MapData more of a DTO.

-----
EDIT

I also noted in op2-landlord, the enum CellType is used. This enum is also defined in Outpost2DLL. Are we wanting to recreate the enum outside of Outpost2DLL as it doesn't make sense to reference the entire project just for one enum? If so, I'll update OP2Utility to include a copy of the enum.

Right now OP2Utility's MapData just returns an int for cellType without telling the user what kind of cell it is via the enum.

Thoughts?

Thanks,
Brett

[leeor_net]

If we have an enumerated type to describe a cell type, any function that gets a cell type should return an enumerated type. This is a case of making an interface hard to use incorrectly.

I don't remember where I got the CellType enumeration from -- probably from the VisualBasic code that I stole adapted from the Visual Basic map editor sources. If this type can be used outside of the Outpost2DLL then we should make it so... maybe call it OP2Core or OP2Common or something equally inane.


EDIT: Heh, forgot to add 'incorrectly' in my first line. 

[Vagabond]

Good news! I managed to read an Outpost 2 map to memory and then save it as a new file on hard drive. It is the exact same size as the original and loads properly into Outpost 2! You can see the pull request here: https://github.com/OutpostUniverse/OP2Utility/pull/19

Of course this only happened because Hooman has documented the exact contents of a vol file through his own research. And he provided an example in the old mapper to follow. I just wrote it in C++11/14 style without the COM added in and probably adding my own set of blunders to the process.

I created a merge conflict as well, so this branch will need to be manually merged. I saw the conflict coming, but let it develop so I could try merging them manually using TortoiseGit. Hopefully I don't regret that decision...

The next project will be introducing the enum cellType.

MapWriter currently uses fstream to save the file. Are we interested in this being a more generic StreamWriter? If so, this would be queued behind adding the enum cellType. I'm not sure this is worth our time or not though?

Currently, the MapData class in OP2Utility fairly closely mirrors the actual content of an Outpost 2 map file. We need to decide if we want to break the file into some sort of TilesetManager as the originally mapper did. Since Outpost 2 saves all the tileset and tileGroup information straight in each map file, it is all contained in MapData. We could separate this data off into a TilesetManager instead though. I think there are pros and cons to both methods.

It is nice that MapData currently mirrors the saved map file structure and that it behaves like a map does in Outpost 2 (that the tileset and tileGroup information is embedded and could change for each loaded map). However, a mapper would probably prefer the data to be separate concerns. If they are separate concerns, the mapper will still need to understand that they are really not actually separate so when loading a new map it needs to force changing out the tilesets and tileGroups.

Thanks,
Brett

[Hooman]

Aha, this is the source of the MapReader / MapWriter thing I've been making comments on elsewhere.

Some interesting things to think above regarding the tileset info. There probably should be an easy way to set default tileset info that matches existing maps. Maybe a default const TileSetManager object that could be assigned?

As for data organization, the OP2Editor backend stored a pointer to the TileSetManager object within the MapFile object, though simple containment might have made more sense here. I suspect the choice to use a pointer was more to do with fighting C++ or COM syntax back when I was learning some of that stuff. Part of the difficulty may have been constructing the TileSetManager object part way through construction of the parent MapFile object. The map header needed to be processed before the tile set data could be processed. These days, you could use emplace to help with that, or construct a dummy object which loads data later outside of the constructor.


I approve of the cellType changes. Those are some good reasons to make that change.


I would also like to see more generic stream reading and writing classes. We should also move away from the memory mapped I/O, which is a little less portable. I've had some thoughts on the stream design, though it seems I keep running into trade-offs, some of which can be rather subtle, though still rather ugly.