ULTIMA8: Store minimap data for all maps and add to savegame

[OMGPizzaGuy]

ULTIMA8: Store minimap data for all maps and add to savegame

This fixes bug #11486
Since this is an update to the save version, I wanted some discussion around it.

First, this now creates new surfaces for every visited map that are added to the save. When each surface is actually initialized it becomes 512x512 2bpp. With 38 maps in Ultima8, that would mean roughly 19 mb of ram. For saving, the surfaces are cropped to the non-black areas, resulting in a noticeably smaller size. I tested this by generating full maps for around 30 of the maps in game and was pleased that the savegave only increased by less than 500kb, thanks to savegame compression.

Any thoughts on this are greatly appreciated. Thanks!

[mduggan]

Thanks for working on this! It's been deep down on my list of things to look at for a while. Here are some thoughts:

Size

The savegame size increase seems fine, especially since very few users will ever fill all the maps.

If we want we could consider some sort of saving for in-memory maps. Two ideas spring to mind:

• Write to a zlib memory stream and then decompress the current map only
• Maybe easier, size the map surface with an offset to the written area - since most maps don't cover the whole coordinate space. When initialising the surface it wouldn't be too hard to find the top-left and bottom-right locations for fixed items and save the offset (we already save size).

That does add a bit of complication though, and overall max of 19MB doesn't seem terrible except for memory-constrained platforms. Will leave it up to you whether it's worth the extra effort.

Location of code

Saving it in the MiniMapGump isn't so bad since it "owns" the data in some sense, but it's a bit leaky because of the max map number.

To clean that up, we could avoid limiting the max maps and just keep pointers in a HashMap against the map number. You can iterate the HashMap to save since order isn't important. That would also avoid saving empty surfaces for unvisited maps.

[OMGPizzaGuy]

Thanks for working on this! It's been deep down on my list of things to look at for a while. Here are some thoughts:

Size

The savegame size increase seems fine, especially since very few users will ever fill all the maps.

If we want we could consider some sort of saving for in-memory maps. Two ideas spring to mind:

• Write to a zlib memory stream and then decompress the current map only
• Maybe easier, size the map surface with an offset to the written area - since most maps don't cover the whole coordinate space. When initialising the surface it wouldn't be too hard to find the top-left and bottom-right locations for fixed items and save the offset (we already save size).

That does add a bit of complication though, and overall max of 19MB doesn't seem terrible except for memory-constrained platforms. Will leave it up to you whether it's worth the extra effort.

I'm inclined to worry less about ram use now that it's down below 20Mb for the textures, but I could be wrong.
Compression seems like a plausible answer, although depends on USE_ZLIB. I considered run-length encoding, but I'm not sure there would be a significate save with that.

Attempting to reduce surface size to only written area seems interesting. I might look into that further.

Location of code

Saving it in the MiniMapGump isn't so bad since it "owns" the data in some sense, but it's a bit leaky because of the max map number.

To clean that up, we could avoid limiting the max maps and just keep pointers in a HashMap against the map number. You can iterate the HashMap to save since order isn't important. That would also avoid saving empty surfaces for unvisited maps.

I think I agree an alternate collection would be a good move (perhaps in World as well) - I will look into this.

I'll be out of town for a few days, so I'll continue on this next week

[OMGPizzaGuy]

Bigger change in the last commit - Added Minimap class to hold the generation logic & save / load. Now the gump focuses primarily on display and we could consider moving ownership of the MiniMaps. Note that with the current ownership, MiniMap generation doesn't start until the first opening of the gump & the gump is never closed, just hidden after that.

I'm going to experiment with reducing size of saved data next.

[sev-]

Does it truly make sense to spend time on reducing save sizes since they are gzipped anyway?

[OMGPizzaGuy]

Does it truly make sense to spend time on reducing save sizes since they are gzipped anyway?

Cropping the saved surfaces to non-black areas seems to be a modest saving, but was easy enough implement. Currently testing it out and the mostly empty maps saved about 500 kb on the save.

There's also potential to not fully build out the full surface size on load, which would cut down on ram usage. I'll not worry about unless a need arises.

[OMGPizzaGuy]

This should be ready now. I've updated the title & description to reflect the current state of the pull request.

[mduggan]

Sorry, I forgot to put a stamp on this but it looks great to me. Needs a manual merge now though, will leave that to you.

Thanks again for implementing!