fix submap saving, erase memory leaks, ignore incomplete submaps

[BevapDin]

fixes #5980, at least for that specific save.

The savefile from this report http://smf.cataclysmdda.com/index.php?topic=5251.msg85763#msg85763 is technically invalid, as it contains 59930 submaps. This is against the assumption that mapgen always creates 4 submaps at once (59930==14982*4+2).
Specifically there is a submap (-1,68,0) and (-1,69,0), but no submap (-2,68,0) nor (-2,69,0)

mabuffer.cpp line 226: the position of submap is written, even if the submap does not exists (lookup_submap on the next line returns NULL).

mapbuffer::save_quad will write the submaps (-2,68,0), (-2,69,0), (-1,68,0), (-1,69,0) in that order into the file. Calling lookup_submap for (-2,68,0), the file only contains 3 values, that have just been written, so lookup_submap fails.

Than it looks for (-2,69,0) and now the file contains 6 values (coords of (-2,68,0) and (-2,69,0): 2 lines with 3 numbers each) and the unserialize_submaps thinks that it contains a whole submap so it reads it. But that file starts with the coordinates of (-2,68,0), so the loaded submap is stored as that one and lookup_submap returns NULL ((-2,69,0) still does not exist).

Now the submap (-2,68,0) suddenly exists. The other two submaps are written normally.

Back into mapbuffer::save, the loop head (very clever actually) assumes that mapbuffer::save_quad deletes all 4 submaps mentioned above, as they have just been saved. The submap (-2,68,0) has not been deleted, it exists now. So it starts the loop again with submap (-2,68,0), which is now the first in the map.

But saved_submaps already contains the position of that submap, so it's ignored and the loop start again. This goes on as the lines 176-189 do not change the state of the loop condition and therefor lead to an infinite loop.

Similar happens again at (94,-430,0), similar reason (not investigated that one).

Solution: don't call lookup_submap when saving submaps, as lookup_submap will load the submap if it's missing (or trying to do so, because all it needs is 5 values, that are loaded into locx, locy, locz, turn, temperature. Then eof is checked, than the rest of the submap is loaded without checking if there is any rest at all - there should be SEEX*SEEY terrain entries, but this is not checked).

Btw. if that matters: tested under linux, the save seem to come from windows.

[KA101]

OK. So far as I can tell, this conversion process (post-BevapDin) puts Cata into (Not Responding) mode in Windows, if you switch away. (I don't blame you. Watching those numbers scroll is kinda boring.)

It doesn't actually mean the program hung. You let it run for as long as it'd take to sit there and watch it scroll the numbers, and it finishes. Just that Cata doesn't kick back the "I'm still here" code.

[BevapDin]

If it does not update the "Please wait as the map saves" prompt it is stuck in an infinite loop, I checked it with a debugger. There might be some differences on how the OS handles simultaneous read and write access.

I added another commit that removes the possibility of an infinite loop.

[kevingranade]

This will be somewhat faster in addition to avoiding the issue, good stuff.

[kevingranade]

I was a bit worried about that 4-submap assumption, no idea how the game got into that state in the first place o_0. Thanks a ton BevapDin

[KA101]

Next time get onto the IRC and tell me you're planning on adding to your PR! ;-P

[BevapDin]

no idea how the game got into that state in the first place

It should be impossible, maybe a crash while saving the maps had erased some submaps.

@KA101: I'm sorry, for that, it was not planed. It seemed that the first commit did not work for you so I added the second to be sure.

It might have been an OS issue, as windows might prevent reading from the already opened file. In that case the bug would not be triggered.

[KA101]

Deleting it, yeah. Reading, I'm not so sure.

I was mostly joking there (hence the tongue emoticon). 's all good.