Fix mission reference to nonexistent om_terrain

[sparr]

Summary

Bugfixes "Scrap trader mission no longer generates duplicate refugee center"

Purpose of change

The refugee center should be globally unique.

Describe the solution

The mission referenced an old om_terrain "refctr_S3e" which doesn't exist any more. I replaced this with the same "evac_center_18" that is used in the equivalent chemist and lumbermill missions.

Describe alternatives you've considered

"refctr_S3e_north" would refer to the same map in the refugee center as before, but would still be inconsistent with the other missions.

Testing

I was not able to reproduce the duplication of the refugee center.

Additional context

Originally reported at https://www.reddit.com/r/cataclysmdda/comments/1fq6w4z/only_164_tiles_to_the_refugee_center_awesome_wait/lp33kmd/ by https://www.reddit.com/user/TheShoopdahoop

[RenechCDDA]

Worth backporting if this bug existed on 0.H

[sparr]

I haven't checked yet but I suspect this bug existed back to at least 0.F

[sparr]

@RenechCDDA what is the process for a backport?

[RenechCDDA]

Just open a new PR targeting the 0.H branch with your changes. You should be able to cherry pick the commit right onto your new branch, there shouldn't be any merge conflicts.

[sparr]

Apparently this has flipped back and forth recently and wasn't buggy for some previous versions. I got a dozen commits deep in tracking down which versions of the terrain names matched vs didn't and gave up. I think it's ok in 0.H-branch

see #70847

[Procyonae]

For posterity this PR didn't do anything, refctr_S3e is still a valid om_terrain, the assign_mission_target field doesn't expect a rotation and even if it did the one you've changed it to doesn't have a rotation so I'm slightly confused what you thought this did '^^
The real fix is making a way to have json defined fallbacks for if the target can't be found in the search_range which defaults to 900 past just forcing placement or replacing a singular OMT. Weirdly there is a create_if_necessary bool in mission_target_params so you can disable forced placement but it isn't read from anywhere and defaults to true.

[sparr]

@Procyonae What I thought was happening here is that the mission code was looking for an OMT of type refctr_S3e, not finding it, and then creating the target om_special from scratch. Changing the om_terrain to one that does exist within the om_special resolved that misbehavior.

Also, just in general, it was weird for this single mission to reference a different OMT than all the other "find or create the refugee center" mission definitions.

I can look into this again if you're saying that's not what was going on?

[Procyonae]

refctr_S3e does exist within the om_special and exists anywhere that evac_center_18 exists

Cataclysm-DDA/data/json/overmap/overmap_special/specials.json

Line 2131 in df56dc5

{ "point": [ 7, 8, 0 ], "overmap": "evac_center_18_north" },

Cataclysm-DDA/data/json/overmap/overmap_special/specials.json

Line 2211 in df56dc5

{ "point": [ 7, 14, 0 ], "overmap": "refctr_S3e_north" },

so unless it's right on the edge of the search range this yields the exact same results (I left that id alone at the time bc the computers search for that OMT and I planned on making the unique behaviour of that function exposed to the json methods but never figured it out over multiple attempts)

You're correct in that it's failing to find it and then force placing the special though ye, again though this wants generic C++ solutions to allow you to define in json what to do if one can't be found, like minor radiant NPC quests should probably just say they "can't remember where it was actually" or whatever while faction "main questlines" should probably force place.
Another way to solve this without any extra infra would be preconditioning minor missions with something like

  {
    "condition": {
      "and": [
        { "not": { "expects_vars": [ "npc_failed_search_minor_mission_target" ] } },
        {
          "or": [
            { "expects_vars": [ "npc_minor_mission_target" ] },
            {
              "npc_location_variable": {
                "npc_val": "minor_mission_target",
                "target_params": {
                  "om_terrain": "minor_mission_omt",
                  "search_range": 300,
                  "false_eocs": {
                    "id": "set_npc_failed_search_minor_mission_target",
                    "effect": { "math": [ "npc_failed_search_minor_mission_target", "=", "1" ] }
                  }
                }
              }
            }
          ]
        }
      ]
    }
  }

so you'd only see the start mission dialog if a target is found and it would only do the search once for that npc. I think that would work fine but it's very cumbersome to tack onto every minor mission and if an NPC had lots of minor missions it'd do all the searches as soon as the dialog is available which given the lag 1 large search can cause probably isn't ideal
For globally unique specials it should also have special handling to check if said special has been placed and if so search all the explored OMs for it (or we just start tracking where they got placed) bc if we know it's placed there's no point generating new OMs looking for it.
I am thinking about other solutions for the ideal long term solution for assign_mission_target and am happy to take it on I just haven't thought it out fully yet. With the long term goal of making the world more realistically spread out this is only going to get worse so I'd rather not just bandaid this.

[sparr]

I wasn't able to find the code path that would add the direction suffix and allow refctr_S3e_north to be a match for refctr_S3e. I did note that the literal refctr_S3e had been removed in the past.

[Procyonae]

If you thought lacking the rotation was the issue (which it isn't, why would you want to make it _north and only match 1/4 of the time?), why did you think changing it to "evac_center_18" without specifying a rotation would help?

[sparr]

Because evac_center_18 exists in the om_special definition.

And also, again, because the other two similar missions use evac_center_18.

[Procyonae]

Because evac_center_18 exists in the om_special definition.

I literally linked the line where refctr_S3e is also there, I don't understand why you're so adamant it not being despite that and turning it into an argument lol, like I said to start I'm merely pointing this out for posterity for anyone confused by this change in the future