[RDY] Json overmap saves

[kevingranade]

Turns out overmap save format was blocking my work on hordes 2.0, so I took care of this first so I can have more flexibility when saving/loading the new horde data structures without backwards compatibility being a problem.

[kevingranade]

Fixed the issue BevapDin pointed out.

[BevapDin]

This doesn't work correctly. save_info returns a JSON string, it is written here as a single string to JSON, but the deserialize code expects a normal JsonObject.

Example (that's what is written):

"npcs":["{\"str_cur\":1,\"str_max\":6,\"dex_cur\":5,\"dex_max\":8,\"int_cur\":3,\"int_max\":6,\"per_cur\":7,\"per_max\":

This is what is expected:

"npcs":[
    { "str_cur":1,"str_max":6,"dex_cur":5,"dex_max":8,"int_cur":3,"int_max":6,"per_cur":7,

Edit: changing it to write( *i ) should do the trick.

[kevingranade]

Ok, this is what I should have done in the first place, big fat integration test where I load an overmap save and then test the overmap for having the expected contents.
Not done, I still need to read in some NPCs and some vehicles, but I wanted to push it in the current state for feedback. Also I'm planning on having it write the new version of the file, then read it back in and check the same values again to make sure nothing changed.

[kevingranade]

Side note, init_global_state() in savegame_test.cpp is how to stand up an instance of the game in a unit test, that's what took me the longest time to sort out, but now you should be able to just do that and write whatever unit tests.

[kevingranade]

And there we go, spot-checks a whole bunch of overmap entities, and we can repeat the same process for future savegame migrations.

[BevapDin]

Why is the target read-only, but the position is writable? Shouldn't it be the other way, the position is also used as key in the map of groups, so it should always match the value in the object.

[kevingranade]

No good reason, I just restyled it while I was changing it from a 3-tuple to a tripoint and left how it works alone.

[BevapDin]

The savegame_test binary must be run from the main folder, but when started from the Makefile it is (like all test binaries) run from within the tests folder. Therefor it won't find the game data and consequently crashes.

It works fine from the main folder.

Old maps and converted maps load fine.

[kevingranade]

Addressed everything but the last point, path configuration quickly turned into a can of worms I don't want to deal with right now, unless there's a simple way to handle it I'm missing.

[BevapDin]

I needed to partially revert the last Lua changes, the build would fail otherwise.

The wrapper generates a setter function named mongroup_set_target (mongroup is the class type, set indicates a setter function, target is the member that is set).

An equally named function is generated for mongroup::set_target (in which set_target denotes the function name).

Those two function conflict as they have the very same signature. Ideally the Lua wrapper really unique names.

[narc0tiq]

I hesitate to suggest "more name mangling" as the solution, but if the first were to translate to mongroup_setter_for_member_target and the second to mongroup_method_set_target, that would be unlikely to be made to conflict in the same way in future.

I'm assuming length of the generated name doesn't matter, as they should never be explicitly used by a human except through the translation layer.

[macrosblackd]

This breaks Play Now! functionality

[BevapDin]

There were changes in newcharacter.cpp that leave the WINDOW pointer with a nullptr when started via "Play now". It worked fine in curses. I looked up the version of delwin in cursesport.cpp and it has a check for nullptr, so I thought it would be fine.

However, the versions of werase and wrefresh in cursesport.cpp (which I did hadn't checked), and many functions more, don't have that check.

Should I add the check (and thereby make the functions more compatible with curses) or should I revert the changes in newcharacter.cpp, which would avoid using the nullptr in the first place?

[macrosblackd]

I would think that the changes need to be reverted, or the logic changed. Right now it says only create a window when you're not using play now.

[kevingranade]

I'd prefer making cursesport compatible, not least because that is keeping the unit test from crashing. Alternately you (or I for that matter) could protect the calls to wrefresh etc from the nullptr, but I'm leaning toward the former.