Base camp reorganization

[PatrikLundell]

Summary

SUMMARY: Infrastructure "Reorganized base camp hub and expansions to support new versions in parallel"

Purpose of change

The purpose is to prepare the ground for the introduction of new base camp versions as well as new expansion versions.
This is part of PR #45560, which was considered too large to be practical to review as a whole.

Describe the solution

• Untangled dependencies between different base camps, expansions, and versions of those. This is only possible to a minor extent, due to backwards compatibility issues, but it's possible to get new uses use structures with lesser inter dependencies than legacy ones.
• Little is visible for the base camp. The structure prepares for the introduction of new versions, though, which is why the single modular one has been pushed down a folder level into a version_1 folder.
• The changes are more visible for expansions. Like base camps, the existing expansions are pushed down one level in preparation for the introduction of additional versions.
• A yet completely unnecessary step with a zero time recipe has been introduced, where the player has to select the single available version for construction. This is in preparation for multiple versions, where this recipe would let the player select which version, potentially among may, to construct. The intention is that this level would also provide some kind of description of each variant to help guide the selection. See the next section for alternatives.
• It should also be mentioned that a small number of minor adjustments have been made to existing base expansions. An extra cooking token for the canteen cooking facility (2 stoves), a well token for the kitchen well, an east-west copy paste error, "forage"->"forge". There might be something else along these lines as well.
• Differs from PR Base camp expansion #45560 in that the top level expansion palette used has been factored out in a cleaner implementation. Also, the Saltworks, which PR Base camp expansion #45560 doesn't touch, has been modified to use the same preparation logic as all the other expansions.

Describe alternatives you've considered

• A very clear alternative for expansion version selection implementation is to use the same logic as for the base camp, i.e. to use the top level recipe group to list all alternatives. The main reason this option was not chosen is that the list isn't sorted in any way that makes sense to humans (probably the order in which the recipes happen to be stored), which means the player will get a very poor overview of the available options. There's also no support for a descriptive text along the line of the recipe selections further down (where there's one "recipe name" line and a separate description line presented along with the recipe requirements). Sorting such that variants of the same expansion would end up grouped together would be sufficient to change the balance in favor of this implementation and prompt an adjustment of the logic.

Testing

The main testing has been performed as part of PR #45560. However, the base camp has been partially constructed and each of the expansions have been surveyed for and verified that they provide expected construction option once they've been selected as the version to use. As the Saltworks has been modified, it was fully built from alien resin (as that's the building material that's easiest to spawn and use).

Additional context

https://discourse.cataclysmdda.org/t/field-base-camp-adjustments/24924 is the thread leading up to PR #45560, and thus provides the background for what the aim of these changes are.

[Xaleth]

Yeah there needs to be 2 spaces between the sentences..

[PatrikLundell]

I'll change it, of course, but the comment is still confusing, as JSON_STYLE.md doesn't seem to say anything about it, so I don't know why I would be expected to know about it. Also, out of curiosity, why? I assume it does have some technical reason, but what, and how does it manifest itself?

Edit: Done. The second sentence was removed because it shouldn't be there in this PR (it will return with two spaces when a second expansion that can be compared against is introduced).

[anothersimulacrum]

I assume it does have some technical reason, but what, and how does it manifest itself?

It's a standard for monospaced text. You may or may not have been told when you were younger to put two spaces between sentences, and that is why. Today we don't tend to use monospaced text, so it is not done in many contexts.

This standard is documented in doc/MANUAL_OF_STYLE.md

Use double sentence spacing after periods. This means that a period that ends a sentence should be followed by two spaces. If the sentence is the last in the block of text, there should be no spaces following it.

[PatrikLundell]

Thanks for the explanation. I've never heard of any such rule, despite definitely being old enough: the only thing I believe I've heard is to put exactly one space in between sentences (and not anything about exception for typewriters, which are inherently fixed size, though that education was both very brief and compressed further by circumstances). Anyway, those are the (local) rules, and so they should be followed.

[actual-nh]

You can actually find switches for "one or two spaces after a period" in a lot of spelling/grammar checkers, BTW.

[curstwist]

Excited to see this start going in! I may have more comments later but at first glance,

Why do you separate all the recipes groups out by location? They are all duplicating each other while using unique ID's. I'm not sure if that's necessary but it will add even more bloat so I wanted to ask.

e.g.
fbmc_shelter_2_cooking_recipes_1
fbmc_firestation_cooking_recipes_1

it seems like the recipes offered in factions can quickly get out of sync this way.

[PatrikLundell]

My reasoning was that separating them allows each base to be developed separately without the developer having to figure out that any recipe changes will affect everyone tied to the set and manually find out which ones those are. If there is an intention to keep recipes tied together as a group that's a bad approach, but if you want to update one without having to touch all of them you either remove "your" recipes from the group hug first so you can modify them, or have the recipes already factored out.

[curstwist]

My reasoning was that separating them allows each base to be developed separately without the developer having to figure out that any recipe changes will affect everyone tied to the set and manually find out which ones those are. If there is an intention to keep recipes tied together as a group that's a bad approach, but if you want to update one without having to touch all of them you either remove "your" recipes from the group hug first so you can modify them, or have the recipes already factored out.

Yeah, I'm unsure which is a better approach, I can see benefits to both. So, unless someone else objects, I'm ok with adding the change to the documentation.

[PatrikLundell]

Note that if #45646 is approved, this PR should be adjusted along the lines of the alternatives from the description, i.e. bring the expansion selection back up to the top (expansion) level.

Edit: #45646 has been approved, and so this PR has been updated accordingly.

[PatrikLundell]

I fail to understand the "Conflicting files
data/json/recipes/basecamps/recipe_modular_shelter_2/recipe_modular_shelter_2_common.json" message. There is no such file in the repo as it has been removed (or, actually moved). Pulling master into the local source of the PR did not cause the file to reappear. I've pushed the master updates back up to the PR in a vain hope of the incomprehensible message not appearing, and a correspondingly vain hope of the continuous integration checks not failing on completely unrelated things.

[PatrikLundell]

• General build matrix / varied_builds (g++-8, 0, 1, address) (pull_request) fails because of internal reasons unrelated to this (JSON only) PR.

"557 Error: (continued from above) ERROR : src/character.cpp:10753 [Character::migrate_items_to_storage(bool)::<lambda(const item*)>] ERROR: Could not put telescoping umbrella into inventory. Check if the profession has enough space."

I guess either the modular container code changes aren't complete, or the save used for testing needs to be to be replaced with one compatible with the code implementation when used as a test object.

[PatrikLundell]

A long time after all checks had been passed this garbage appeared:
"
Conflicting files
data/json/recipes/basecamps/recipe_groups.json
data/json/recipes/basecamps/recipe_modular_saltworks/recipe_modular_saltworks_common.json
data/json/recipes/basecamps/recipe_modular_saltworks/recipe_modular_saltworks_log.json
data/json/recipes/basecamps/recipe_modular_saltworks/recipe_modular_saltworks_metal.json
data/json/recipes/basecamps/recipe_modular_saltworks/recipe_modular_saltworks_migo_resin.json
data/json/recipes/basecamps/recipe_modular_saltworks/recipe_modular_saltworks_rammed_earth.json
data/json/recipes/basecamps/recipe_modular_saltworks/recipe_modular_saltworks_stone.json
data/json/recipes/basecamps/recipe_modular_saltworks/recipe_modular_saltworks_wad.json
data/json/recipes/basecamps/recipe_modular_saltworks/recipe_modular_saltworks_wood.json
"
The first "conflict" consists of the refusal to accept that legacy recipes had been moved to legacy_recipe_groups.json, while the other "conflicts" consists of a refusal to accept that the files have been moved to data/json/recipes/basecamps/expansion/recipe_modular_saltworks, i.e. one step further down into the hierarchy.

Edit:
Found that the silent (no notification) retroactive backstab of this PR was caused by #46278.

[PatrikLundell]

General build matrix / varied_builds (g++-8, 0, 1, address) (pull_request) failure:
"

cata_test is a Catch v2.13.0 host application.
Run with -? for options

Randomness seeded to: 1610366929

-------------------------------------------------------------------------------
suffering from sunburn
      Given: avatar is in sunlight with the solar sensitivity trait
       When: torso and arms are 90%% covered
       Then: damage to torso is 90%% less than other parts
-------------------------------------------------------------------------------
../tests/char_suffer_test.cpp:270
...............................................................................

../tests/char_suffer_test.cpp:276: FAILED:
  CHECK( bp_hp_lost[bp] == Approx( 6 ).margin( 12 ) )
with expansion:
Error:   22 == Approx( 6.0 )
with message:
  bp.id().str() := "torso"

Log messages during failed test:
12:00:00PM: The sunlight burns! your eyes.
"
does not have anything to do with this PR.

[PatrikLundell]

ontinuous-integration/travis-ci/pr — The Travis CI build could not complete due to an error
"
$ sudo -E apt-add-repository -y "ppa:ubuntu-toolchain-r/test"

Cannot add PPA: 'ppa:~ubuntu-toolchain-r/ubuntu/test'.

ERROR: '~ubuntu-toolchain-r' user or team does not exist.

The command "sudo -E apt-add-repository -y "ppa:ubuntu-toolchain-r/test"" failed and exited with 1 during .

Your build has been stopped.
"
does not have anything to do with this PR.

[actual-nh]

ontinuous-integration/travis-ci/pr — The Travis CI build could not complete due to an error
"
$ sudo -E apt-add-repository -y "ppa:ubuntu-toolchain-r/test"

Cannot add PPA: 'ppa:~ubuntu-toolchain-r/ubuntu/test'.

ERROR: '~ubuntu-toolchain-r' user or team does not exist.

The command "sudo -E apt-add-repository -y "ppa:ubuntu-toolchain-r/test"" failed and exited with 1 during .

Your build has been stopped.
"
does not have anything to do with this PR.

Quite. I note it happened with two builds that were happening in parallel - unless it pops up again, probably something went wrong with the Travis system.

[PatrikLundell]

As far as I can tell building fails due to the usual broken tests, which this time seems to be caused by:
"Mods-(~[slow] [.])=> 21:53:40.610 ERROR : src/generic_factory.h:442 [const T &generic_factory<mutation_branch>::obj(const string_id &) const [T = mutation_branch]] invalid trait id "PARKOUR"0.210 s: npc-movement
Mods-([slow] [.])=>
Mods-([slow] ~[.])=> 21:53:41.982 ERROR : src/generic_factory.h:442 [const T &generic_factory<mutation_branch>::obj(const string_id &) const [T = mutation_branch]] invalid trait id "PARKOUR"1.300 s: npc_can_target_player"

At a guess, tests weren't updated to match changes made when changing traits into "hobbies".

[PatrikLundell]

Ping: @kevingranade This PR is a result of your requests regarding #45560, but for whatever reason it's never been evaluated by the core team in any way visible to the author (first it lay fallow for a couple of months, then stamped with "don't process until 0.F", and then just had that label removed without any visible processing).
Once this PR has been resolved (either approved, potentially after comments, or rejected) it would be possible to continue the #45560 work through a series of PRs (one for the base camp itself, and one for each expansion).

[kevingranade]

I'm very sorry about that, partially I'm just overwhelmed with review in general and partially I kept getting this PR confused with #45560, I'm looking at it now.