Fix ATM terrain to furniture migration

[HadeanLake]

Summary

Bugfixes "Fix ATM terrain to furniture migration"

Purpose of change

Fix #76200 The var iid_furn has only one use inside the for loop that loads terrains from terrain_rle - store furniture id in case of terrain-to-furniture migration. Var is not reset after sequence of repeated terrains requiring it is placed. As a result, furniture will be placed on all remaining tiles until the end of the submap. to_ter JSON field is not specified for ATM migration, which does not cause an error when loading the game and results in terrain under new furniture-ATM being set to nothing.

Describe the solution

Add to_ter to the JSON data. ATM terrain will be migrated to concrete floor. Set iid_furn to null_id if there is not migration for this terrain. Set was_loaded parameter to false so that missing/failed to load mandatory member error can be thrown for contributor to see if they omitted mandatory json member.

Describe alternatives you've considered

Check for !remaining again where iid_furn has value and clear it there

Testing

Loaded old save and visited previously explored places with atms. Loaded the test save from linked issue. +Checked that there are no errors when loading the save, all ATM terrain tiles were replaced with the proper number of concrete floor + ATM furniture tiles.

Removed mandatory field from json +Checked that missing mandatory member error has occured

Additional context

[akrieger]

Why not at line 4920 where we test for !remaining already?

[mqrause]

Simply removing the check in line 4924 should be enough, I think.

[HadeanLake]

Why not at line 4920 where we test for !remaining already?

I thought since iid_furn almost never has value, it makes sense to check if it should still have a value for the next iteration only when it does. But yes it does not hurt to reset it somewhere in if(!remaining) anyway for every processed sequence instead.

Simply removing the check in line 4924 should be enough, I think.

That is inside if( auto it = ter_migrations.find( terstr ) and does not run unless ter_furn_migration for this terrain exists. This will not fix the issue, but the check can be removed anyway. And it actually must be removed if iid_furn = furn_str_id::NULL_ID().id(); is moved into the else block so that it can reset if we have terrain to terrain migration immediately after terrain to furniture migration.

(And maybe also add a check if the furniture id is actually valid.

validity of furniture and terrain in ter_furn_migration JSON is checked when save is loading: https://github.com/CleverRaven/Cataclysm-DDA/blob/1a13be36d01648cbbaa206f79de5b58ed6ae62fc/src/init.cpp#L825

[HadeanLake]

There may be a problem here: https://github.com/CleverRaven/Cataclysm-DDA/blob/1a13be36d01648cbbaa206f79de5b58ed6ae62fc/src/savegame_json.cpp#L4582-L4587 Omitted mandatory json member to_ter does not causes an error May be out of scope of this PR #76246

And here: https://github.com/CleverRaven/Cataclysm-DDA/blob/1a13be36d01648cbbaa206f79de5b58ed6ae62fc/src/savegame_json.cpp#L4928-L4933 if to_ter is omitted from JSON terstr will be t_null terstr.is_valid() in this case returns true

Apparently nothing (t_null, NULL_ID terrain) is valid because its id exists. Sholud I check terstr for nothing there?

--- a/src/savegame_json.cpp
+++ b/src/savegame_json.cpp
@@ -4921,6 +4921,10 @@ void submap::load( const JsonValue &jv, const std::string &member_name, int vers
                         auto migrate_terstr = [&]( ter_str_id terstr ) {
                             if( auto it = ter_migrations.find( terstr ); it != ter_migrations.end() ) {
                                 terstr = it->second.first;
+                                if( terstr == ter_str_id::NULL_ID() ) {
+                                    debugmsg( "tried to migrate terrain to nothing" );
+                                    terstr = ter_t_dirt;
+                                }
                                 iid_furn = it->second.second.id();
                             } else {
                                 iid_furn = furn_str_id::NULL_ID().id();

[mqrause]

Omitted mandatory json member to_ter does not causes an error May be out of scope of this PR #76246

If it's really just flipping true to false, I'd say it's fine in this PR. But it could be a separate PR as well.

Null ids being valid is by design. Though I would assume migration to t_null is not something that should happen.

[HadeanLake]

Terrain can be nothing only if to_ter is missing or invalid, but to_ter is a mandatory field. Missing mandatory field is a game load breaking error. So checking for null id there is not necessary because game will throw an error while checking for validity or fail to load if missing anyway(when 76246 is resolved). As for fixing #76246 I think I need to add some fields to ter_furn_migrations class

[HadeanLake]

I noticed was_loaded bool member is often used in mandatory and optional calls as second parameter. I thought maybe migrations can become ter_furn_migrations class instances and also have it. That would have also allowed lines like terstr = it->second.first; to be replaced with something more readable terstr = migration->to_ter; etc. But after some reading and debugging it with breakpoints to see how it loading works I abandoned that idea. It does not need was_loaded member, readability change is minimal and such small object probably does not need to be that complex. Until more comlex terrain-furniture migrations with more complex associated jsons need to be introduced at least.

So I will just pass false as second parameter in mandatory and optional calls. This object is loaded exactly once, I see no reason to assume the object has already been loaded and suppress errors

[HadeanLake]

so, this is why it was true - optional assigns default value if was_loaded is false and that value which is "" for empty_string_id overrides previously assigned null id https://github.com/CleverRaven/Cataclysm-DDA/blob/c06e515d3416d1f97de91cff677a7a87f3ef89db/src/generic_factory.h#L834-L836

[mqrause]

You can add a default value as another parameter to the optional calls.