Cataclysm-DDA icon indicating copy to clipboard operation
Cataclysm-DDA copied to clipboard

Terminating support for obsolete primitive field camp

Open cake-pie opened this issue 2 years ago • 12 comments

Summary

Content "Terminating support for obsolete primitive field camp"

Purpose of change

Since build 9665 (Sep 24, 2019) it was no longer possible to start a faction camp using the legacy "primitive field camp" (aka "Old Camp") design. For people using stable builds, this took effect as of 0.E.

The source code contains hardcoded special casing required in order to support save games that still have faction camps of this variety, carried over from before its removal, such as:

https://github.com/CleverRaven/Cataclysm-DDA/blob/27d6b46dcda2ce6e0e02f7fdb8927093c23650b8/src/basecamp.cpp#L294-L299

Now that 0.G has dropped, we are at two full releases since 0.E; time to consider ending support for these legacy camps.

This will additionally allow us to eliminate other legacy code that is lurking around from various changes in the implementation of faction camps during the early stages of development.

Describe the solution

  • Remove hardcoded special casing from basecamp::has_water()
    • primitive field camp will no longer support NPCs automatically using its well(s) to quench their thirst.
    • fbmh_well_north special case is retained to continue support for any that were constructed between b9162 (Jun 16, 2019) and b9644 (Sep 20, 2019), since the "Modular Field Camp version 1" is still available and supported. This special case could be removed if that type of camp was obsoleted, or when has_water() is modified to actually check for presence of terrain/furniture that acts as a water source rather than relying on JSON blueprint_provides.
  • Document the purpose of blueprint_provides water_well in BASECAMP.md
  • Remove hardcoded special case handling for primitive field camp's "Gather Materials" mission, which required extra machinery to allow it to return gathering yield that varies according to the camp level 1~20.
  • Remove hardcoded special case handling for faction_base_camp_1 mission (command tent) not requiring food
    • the recipe is being deleted so no other special cases remain, but the machinery is already there, so JSONize the food requirement waiver with a NO_FOOD_REQ flag anyway, in case it comes in useful in future.
    • an alternative approach here would have been to strip out all code related to waiving the food requirement entirely so that it is no longer possible
  • Remove machinery related to "recover camp" which assigns a new NPC as overseer after the previous one died.
    • obsoleted after overseer replaced by bulletin board
      in #28742
    • associated talk option was removed later on
      in #29807
  • Remove talk option to "remove overseer" and related machinery
    • was implemented to allow players to free up NPC from obsoleted overseer role
      in #28742
    • players have had since b8878 (experimental) / 0.E (stable) to switch from overseer to bulletin board
    • also remove stray "become overseer" left over
      after #28826
  • Delete obsolete TALK_CAMP_OVERSEER dialogues, no longer needed without overseers
  • Confirmed that it was already no longer possible to start the primitive field camp nor add its expansions:
    • faction_base_camp_0 was removed from all_faction_base_types
      in #34184
    • faction_base_blacksmith_0 was removed from all_faction_base_expansions
      in #34411
    • faction_base_kitchen_0 was removed from all_faction_base_expansions
      in #34496
  • Deletion of obsolete json entries / files:
    • base building recipes for faction_base_camp_0 and its upgrades, and associated update/nested mapgens
    • base building recipes for faction_base_blacksmith_0 and its upgrades, and associated update/nested mapgens
    • base building recipes for faction_base_kitchen_0 and its upgrades, and associated update/nested mapgens
  • Added npc dialog to inform players about the changes / upcoming removal of support

Future work

Continue checking for legacy code, dead code and obsolete JSON for removal.

Final step of end-of-life for the primitive field camp would be to have a savegame migration process that deletes any remaining camp/expansion of these types from the camp/expansion list and reverts the overmap ids that they occupy to field. This would enable culling of all associated "type": "overmap_terrain" json entries. Prefer to defer this for one more version so people have a chance to salvage resources and abandon camp "in-game" rather than be forcibly ejected.

Describe alternatives you've considered

Leave it as it is.

Testing / checking

It is highly recommended to review this PR commit-by-commit rather than try to digest all the changes at once.

  • has_water(): code inspection.
  • gather mission: check that gather mission not broken for other camp types. For primitive field camp, change is semi-visible to player; people who are very observant may notice that the yields have changed, but otherwise it still functions
  • NO_FOOD_REQ: add the flag to a long mission, e.g. one of the week-long ones that set up an expansion. Should be able to dispatch companion even with very low food stocks in the camp.
  • recover camp, remove overseer, overseer talk topic: code inspection; these are basically dead code
  • json deletions: follow the links I provided to previous PRs to confirm that the base/expansions can no longer be started

Additional context

I originally misdiagnosed the hardcoded blueprint_provides in basecamp::has_water() as a lack of consistent jsonization. Thanks to @PatrikLundell for pointing out the error.
PR was repurposed; original OP preserved below.


Click for original OP

Summary

Infrastructure "Expand and standardize jsonization of basecamp::has_water()"

Purpose of change

Spotted this: https://github.com/CleverRaven/Cataclysm-DDA/blob/27d6b46dcda2ce6e0e02f7fdb8927093c23650b8/src/basecamp.cpp#L294-L299

It does not make sense if every time someone added a new basecamp expansion -- a primarily JSON task -- that provides a water supply directly to the camp that they would have to come and add their corresponding blueprint_provides to this growing list -- in C++ code.

Describe the solution

water_well is already used for this purpose in some of the basecamp expansion defs. Make that a standard thing, document it, and add it to those that don't already have it. Remove the redundant entries from the C++.

Describe alternatives you've considered

Have some other id for this, like water_source, but it seems that all the existing ones are indeed wells and I don't think it's worth splitting hairs over what the id should be even if some hypothetical expansion supplies water to the camp by some means other than a well.

Testing

Hang around in a camp with a water well expansion and ensure that NPCs there continue to correctly get a drink automatically when thirsty enough (thirst > 40); they will say camp_water_thanks line when they do this. Can also check in periodically and check that no NPCs are building up high thirst levels.

cake-pie avatar Jan 22 '23 10:01 cake-pie

My understanding is that the hard coded faction camp pieces are for the (now very) old hard coded faction camp that can't be generated (but was retained for save compatibility, I presume). Newer JSON faction camp usage relies on the water_well token instead, and so haven't been added as direct entries here. Given that the hard coded faction camp wasn't present at all in 0.F (I don't know if it was removed in 0.E or even earlier than that), it's probably time to remove support for it.

Note that I'd want to see the gradual removal of various faction camp tokens in favor of scanning for and detection of functionality providing assets in the base camp to allow them to be available regardless of whether they're produced via faction camp blueprints or player (ordered) construction.

PatrikLundell avatar Jan 23 '23 09:01 PatrikLundell

Well, this change doesn't preclude termination of support for the old pieces and would actually facilitate it; one less thing to have to deal with in addition to just dropping them from JSON.

cake-pie avatar Jan 23 '23 20:01 cake-pie

Agree. I was trying to provide some context.

The basecamp.md file seems to retain outdated, and possibly erroneous, information. The chop shop has been removed, for instance. I'm not sure how correct the others are. The added description for "water_well" doesn't actually say what the token does, only point to a function that JSON editors aren't supposed to need to know what it's for. What it does provide (from a JSON crafter perspective) is unlimited amounts of water for faction camp crafting and other usages of water (and it can be noted that the water isn't clean, although I don't know if companions are affected by unpurified water).

Changing the code in basecamp.cpp causes existing (if any) instances of the old hard coded basecamp to cease to function. Modifying the JSON for the construction of these won't retroactively add the new token to them; it would only make a started but not finished base camp capable of (finally) adding these constructions with retained functionality, which is an extremely unlikely niche case.

I think it's time to get rid of the primitive_field files in version 1 of the hub, i.e. the recipe and blueprint ones (the ones in the garage and farm version 1 expansions should remain: they're primitive but should remain functional). As far as I can tell from a brief look they can't be accessed unless you started a basecamp at some time before 0.F, and I believe one version is the span obsolete functionality is to be supported.

PatrikLundell avatar Jan 23 '23 20:01 PatrikLundell

Okay, thanks for the elaboration, that was helpful. Are you going to make a PR that ends support for the old v1 camp pieces? In which case I'm happy to close this as superseded. Otherwise please advise how you'd like to proceed.

cake-pie avatar Jan 24 '23 04:01 cake-pie

This isn't really version 1, but rather version 0. It ended up in version 1 when version 2 was introduced and all that existed previously was moved into version 1, including the support for the obsolete version 0.

I'd recommend the removal of

  • .../data/json/mapgen/basecamps/base/modular_hub/version_1/primitive_field.json and
  • .../data/json/recipes/basecamps/base/recipe_modular_hub/version_1/recipe_primitive_field.json (the file you edited)

plus the code simplification you've made. My checks haven't found the tokens used outside of these files, with the exception of "faction_base_camp_0". This token is used in .../data/json/overmap/overmap_terrain/overmap_terrain_faction_base.json, but there it's used as the base from which camp symbols are inherited, so I don't think it would be a good idea to try to remove it from there. The token is also used in .../data/json/mapgen/faction_buildings.json. I think much of this file is obsolete, in which case the obsolete tokens in it ought to be possible to remove from overmap_terrain_faction_base.json. However, blueprints for existing expansions are in this file, so clearing out this file of obsolete entries requires searching each entry for usages and then removing the obsolete ones together with their obsolete overmap terrain entries. There's also a section for hide sites and remote wall construction that's still in use.

As you see, it isn't trivial to figure out what should be removed.

PatrikLundell avatar Jan 24 '23 13:01 PatrikLundell

Given that the hard coded faction camp wasn't present at all in 0.F (I don't know if it was removed in 0.E or even earlier than that), it's probably time to remove support for it.

I've tracked it down to b9665 experimental and 0.E stable; repurposing PR into end of support for the primitive field camp.

cake-pie avatar Jan 27 '23 19:01 cake-pie

I believe the fbmh_well_north support can be dropped as well, for two reasons:

  • While the base camp itself remains supported, that doesn't mean every iteration of it has to be. When sufficiently long time has passed, support can be dropped.
  • There is a scanning functionality in place that should pick up the presence of a well even when not explicitly added by a blueprint. It's primarily there to support the bare bones basecamp, but should cover that corner case as well. If it doesn't, there is something missing either in the scanning code or in the application of its results.

PatrikLundell avatar Jan 27 '23 19:01 PatrikLundell

"faction_base_camp_0" [...] is used in .../data/json/overmap/overmap_terrain/overmap_terrain_faction_base.json, but there it's used as the base from which camp symbols are inherited, so I don't think it would be a good idea to try to remove it

Oh, in any case we don't want to remove any overmap_terrain entries yet, doing so would make it impossible to load any save files with those overmap terrain ids. That'll happen in the final stage of EOL process when we cease to support even existence of legacy camps -- either save files with such camps will outright fail to load properly, or the camps get "deleted" during save game migration, with their overmap ids being migrated to something else (probably bare field). I think we should wait another version before forcibly removing any remaining camps in this manner.

For now, deletion of relevant basebuilding recipes and the related update/nested/regular mapgens would halt any further upgrades and expansions. Along with the C++ change that will cease support for automatic watering of followers, this should signal to anyone still playing on a save file that has been migrated from that long ago that it is time to abandon their legacy camps in favor of more modern variants. We could also do a bit more to alert players of the change, like mentioning it in the faction camp tutorial.

There is a scanning functionality in place that should pick up the presence of a well

For crafting perhaps, but definitely not going to work for automatic watering of followers [ 1, 2 ]


In BASECAMP.md under "adding basecamp expansions" it states that "../json/mapgen/faction_buildings.json also has to be updated to introduce an entry for the new expansion." @PatrikLundell you added this statement in #45572, but is it actually true? It looks like since #29523 basecamp construction has used update mapgens, which in turn may refer to nested mapgens -- these are "type": "mapgen" with update_mapgen_id or nested_mapgen_id json property respectively. I suspect the basic mapgen (with "om_terrain") are obsolete; plenty of expansions don't have corresponding entries in there either.

cake-pie avatar Jan 29 '23 14:01 cake-pie

I believe you're correct regarding the faction_buildings.json entry. I believe that was the only pattern in use at the time this was written, and if I remember correctly my attempts to use "normal" definitions for the starting definition failed to work for unknown reasons (I think it only works with "rows" and not point/line/square, and has to be exactly 24*24, although I can't swear on either being correct). Thus, I tried to mention all the places I've found you had to add stuff for it to work, as some of them weren't exactly intuitively obvious to find. I believe later someone (me or somebody else) figured out how to get rid of that pesky entry and move the data to where it belongs, but without any thought of updating the documentation (or that it might affect documentation at all).

PatrikLundell avatar Jan 29 '23 14:01 PatrikLundell

There's obviously more to be done; I've been turning up various bits of legacy support code while looking through the camp-related code. I'll stop here for now and defer further work on this to a subsequent PRs so that this one doesn't get too big and unmanageable. (See "future work" section in OP).

cake-pie avatar Jan 30 '23 09:01 cake-pie

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. Please do not bump or comment on this issue unless you are actively working on it. Stale issues, and stale issues that are closed are still considered.

github-actions[bot] avatar Mar 02 '23 02:03 github-actions[bot]

Spell checker encountered unrecognized words in the in-game text added in this pull request. See below for details.

Click to expand
  • Currently, there are three main types of camps that you can start building: bare bones camps, modular field camps, and established building camps which is a wide range of camps adapted to using specific buildings. The established building list includes fire lookout towers, helipads, lighthouses, light industry, mansions, military outposts, pottery cottages, radio towers, small sealabs, and fire stations. I may have forgotten something here, as my memory may not be completely up to date.\n The bare bones camp is the most flexible kind of camp as it can be built anywhere and doesn't mandate any construction. Modular field camps is the next most flexible kind of camp since you can build them on any field and can locate them to have plenty of space for expansions, but you start with nothing in a field and have to build every building, so they can require a lot of resources. Established building camps are faster to set up since you start in an existing building, but you have to establish them in a supported type of existing building and there may be limited or no space for expansions.\n Each camp location will have a variety of upgrade missions for it, except for the bare bones one where everything is up to you. The various missions have descriptions. In general, though, you'll need to establish housing if you want to expand, and some missions such as hunting or recruiting will require that you have some kind of kitchen or office that you can use to help schedule activities.\n Regardless of what kind of camp you make, you have to start it by asking a companion to build the camp. If there are more alternatives than just the bare bones camp you'll get a list of the available alternatives.

This alert is automatically generated. You can simply disregard if this is inaccurate, or (optionally) you can also add the new words to tools/spell_checker/dictionary.txt so they will not trigger an alert next time.

github-actions[bot] avatar Mar 02 '23 21:03 github-actions[bot]

Build failure unrelated; ef490d7 was fine and 48308a4 doesn't touch C++ code.

E: fixed by #64016

cake-pie avatar Mar 07 '23 05:03 cake-pie