disappear upturned dirt, unable to plant or plow

[blablase]

Describe the bug

unable to build new farm plot, plant or plowing the already existing build, which is disappeared

Attach save file

fucking new world-trimmed.tar.gz

Steps to reproduce

no idea how the upturned dirt got un-upturned then be unable to replant but build a farm survey 2 then build the max tilted soil then plant them

Expected behavior

to be able to have a re-plow-able experience

Screenshots

Versions and configuration

• OS: Windows
  • OS Version: 10.0.22621.3447 (22H2)
• Game Version: cdda-experimental-2024-04-23-0405 7f16ac8 [64-bit]
• Graphics Version: Tiles
• Game Language: System language []
• Mods loaded: [
  Dark Days Ahead [dda],
  TropiCataclysm [tropicata],
  Bionic Professions [package_bionic_professions],
  Aftershock [aftershock],
  DinoMod [DinoMod],
  Megafauna [megafauna],
  Extra Mutated Scenarios [extra_mut_scens],
  Translate Complex Dialogue [translate_dialogue],
  Tamable Wildlife [Tamable_Wildlife],
  Stats Through Kills [stats_through_kills]
  ]

Additional context

No response

[RenechCDDA]

The is_dirtmound lambda inside farm_action is always returning false - the fake_map it loads contains no t_dirtmound. I don't know what it is loading, though. Capturing the terrain names seems to suggest it's just a random field. Modifying the actual fbmf_2 mapgen produces no effect.

[RenechCDDA]

@PatrikLundell Can I bother you to look into this?

My suspicion is that casting the fake_map(tinymap) to a full map causes our 'relative coordinates' to be wrong (since a map is 132x132 whereas a tinymap is 24x24), but I have no idea how to verify that or what to do about it.

Cataclysm-DDA/src/faction_camp.cpp

Line 3581 in 140db31

mapgendata dat( omt_tgt, *farm_json->cast_to_map(), 0, calendar::turn, nullptr );

[PatrikLundell]

I'll try to get to it when done with my current activities.

Casting a tinymap to a map should be perfectly fine as long as all references used refer to that same map and you don't try to use coordinates converting to the bub coordinate system from the abs one.
Anyway, an investigation will either result in a failure to find what's wrong, or us learning something.

Edit:
I don't understand how that code is supposed to work. It loads something into the fakemap, and that something seems to be the OMT map. It iterates over every tile of the OMT (and generates the fakemap every time, which doesn't make sense either, but probably because it's only needed for the plow option, missing that it's inside the loop).
The code then checks for a dirt mound in each tile of the fakemap and that there isn't "furniture" (= plants) at the corresponding location on the "real" map and places a dirt mound there on the real map.
The problem is that I don't see how the map loaded should contain any dirt mounds. If you wanted the fakemap to contain that you'd have to construct it up to the current build state of the expansion, based on data from somewhere I don't know where.

It's sort of possible with farm expansion 1 if you can determine whether to load faction_base_farm_0, 1, 2, 3, or 4 from somewhere (and maybe the OMT type changes to match that for farm 1, in which case that one could actually work).

The only thing that's in faction_base_farm_2_0 is t_region_groundcover, which may match what was in the fakemap (more or less random values in the range 420 to 424 for the two rows of the first submap I looked at. Obviously I have no actual idea what these numbers are mapped to).

I tried hacking that definition by replacing the first 4 groundcover definitions with dirt mounds, started up the save, and saw that now 4 tiles were available for tilling.
The person who changed the farming stuff the last time doesn't seem to have tested it with expansion version 2...

I can see two solutions to the problem:

1. Change farming expansion 2 to use the archaic system used for the primitive bases and expansions (which are being phased out) and find out how to replace the OMT mapping when appropriate, as well as constructing OMT layouts matching what the expansion data produces (and somehow using telepathy to keep them synchonized, as JSON doesn't allow comments).
2. Introduce code that extracts dirt mound location info from expansion mapgen maps and stores it in the expansion data, and then compare that location info with the actual expansion state when it's a farming expansion, and make the tilling option available when there's something there. Making tilling available to other expansions may or may not be a good idea, depending on whether there are any "legitimate" reasons for other expansions to use dirt mounds.

And regarding the coordinates: both the fakemap and the "real" tinymaps are tinymaps and so their coordinate systems match perfectly. In addition to that, the mapgen stuff all works on the top left corner of the map, and so would use the same coordinate values regardless of the underlying type of the map used.

[silvadomi]

I've been looking into this issue, and think it can be resolved most easily by applying the mapgen updates of all the upgrades that have been built in the expansion to the fake_map, before doing the comparison to the actual state of the terrain. Also rotations / mirroring needs to be taken into account. I don't think anything additional has to be stored in the expansion data, because the locations of dirt mounds can just be extracted from the mapgen data for the existing upgrades each time it is needed.

I'll try to work on a solution along these lines and make a PR. Might need to make the farm_action function a method of the basecamp class though, because it will need access to the private expansions attribute.

[PatrikLundell]

You'd also have to deal with palette parameters. No current implementation depends on which value you select, but you still have to select a legal value.
Rotation/mirroring ought to take care of itself if the small fake map is fed the same parameters as the real expansions were (i.e. their relative locations). Note that some expansions depend on 3D support, including the farm 1 expansion (although the garbage result (the roof replacing the ground level terrain for the shack) you'd get from the affected tiles won't affect mound detection).

[silvadomi]

@PatrikLundell can you clarify the issue with the palette parameters? I'm not sure where this would affect the code I'm changing or the mapgen updates. I think if at all it is done somewhere in the functions that are being called, as I haven't seen it in the upgrade_return function (which I'm following with this fix) either.

[PatrikLundell]

It may be a non issue, but I'll explain my concern:
When you select an expansion construction to be built, you select one out of several recipes for the same construction where they differ only in what palette they're instantiated with. The expansion information, however, only specifies what was built, but I don't think it records the actual recipe selected (if that is indeed recorded there is probably no problem), thus, my concern is that you'd have to select a recipe from several that result in the recorded construction.
It can be noted that it's not really a palette problem as such, as you previously had to select a recipe from a range of recipes where only the construction materials differed. The difference now is that the range of recipes to select from is generated from the permitted palette parameters rather than being crafted manually.

If the actual recipes used are recorded (as opposed to discarded when the building construction is finished) my concerns should not be valid.

[silvadomi]

If the actual recipes used are recorded (as opposed to discarded when the building construction is finished) my concerns should not be valid.

Ok, I think this is the main point indeed. What seems to be recorded in the expansion data is the blueprint_provides identifier from the recipe json. What we need to know for mapgen is the recipe id itself.

In the modular farming extension, the recipe id's are the same as the blueprint_provides id's, so I'm just looking for recipe id's using that string. Also, the code just skips over id's that are not found as recipes. That means that any dirt mounds that are defined in a recipe which uses a different id compared to the blueprint_provides id will not be re-tilled. However, one could have different recipes using different palette parameters for example, and if there is a "common" recipe with the id of their blueprint_provides id that could be used to create a reference map for re-tilling.

Is there maybe a better way to get a recipe id, or would it make sense to store the recipe id itself in the expansion data? Or actually it may be an advantage that we can define a re-tilling map as a recipe, which can be accessed by a blueprint_provides identifier in the expansion.

[PatrikLundell]

Unfortunately, your assumption about the recipe being the same as the blueprint_provides entry holds true only for version 2. Version 1 uses the implicitly defined name of the "result" string, which differs from the contents of the "construction_blueprint" name.
Also note that I haven't checked other expansions that provide farming (I think the mansion does have such expansions, for example), and I believe the fire station provides a few plots as part of the base itself (but I don't know if that base provides base farming for those plots).
Relying on conventions being maintained isn't really robust regardless.

If you're going the route of recording blueprints you probably should extract them on the finishing of their implementation (upgrade_return), before the data goes out of scope. That info should contain all the info you need to reapply the recipe, including any parameter selections. The code called (however far down the chain) extracts the full blueprint name in order to find the blueprint JSON and apply any parameters to it., and that's the data you'd want.
It can be noted that this is also the point at which you'd record the individual mounds if you'd rather switch to that path instead.

I'm not a fan of re-tilling recipes, because you'd need one recipe for each possible set of previously applied plots (not an issue currently, I think, because the order in which you can add plots is set to be static, but there's nothing inherently stopping an expansion to let each segment be created individually, without any imposed order: that's a choice made by the expansion implementor, and my reason for making the order fixed is that you'd gain the flexibility of defining which order you'd add essentially sets of the same functionality where the order doesn't really matter for more clutter in the UI.

And I should mention that palette parameters probably aren't something that's going to be encountered, as neither of the general farm expansions make use of those (version 2 doesn't have any buildings, and version 1 does have a building, but it's neither supporting different materials, nor does it provide mounds in the same recipe as the building).
Still, I think it should be something handled if it occurred.

[silvadomi]

I can have another look at farming extension 1, though I did test this one also with the first set of plots that can be built. I'll also check the other basecamp types for farming upgrades and make sure that these work.

Recording individual mounds during basecamp upgrades is not really a thing I'd want to do, it would be rather cumbersome I think. Also, I'd prefer to find a fix for this that does not require additional saved states in the expansion data, to avoid savegame compatibility issues etc.

I don't think the re-tilling recipes (or maybe just mapgen update functions) would be a problem. Most upgrades relevant for this can just add their recipe id / mapgen id to their blueprint_provides list, if they don't already have it. Only the others (if there are any) would need an additional mapgen definition that includes their dirt mounds. Even if the build order is flexible it doesn't matter, because the upgrades that have already been built will just be applied sequentially to the farming reference map. We won't need individual explicit recipes for each possible build order.

[silvadomi]

Just for the record, these are the basecamp designs and expansions that use the farming id and are thus potentially affected by this issue:

basecamps/expansion/recipe_modular_farm/version_1/recipe_primitive_farm.json basecamps/expansion/recipe_modular_farm/version_2/recipe_modular_farm_common.json basecamps/base/fbmc_mansion/fbmc_mansion_+1.json basecamps/base/fbmc_mansion/fbmc_mansion_expansion_surveys.json basecamps/base/fbmc_mansion/fbmc_mansion_+4.json basecamps/base/recipe_modular_firestation_1/recipe_modular_firestation1.json basecamps/base/fbmc_outpost/recipe_modular_outpost_cross.json basecamps/base/fbmc_outpost/recipe_modular_outpost_normal.json

[PatrikLundell]

Explicitly adding the recipe names to the blueprint_provides would work when no parameters are involved. It should also be possible to add these retroactively by introducing a set of conversion recipes that take no time and which would add the recipe in question to the set if it's not present. Those recipes would show up only if a blueprint has been used but not included.

[silvadomi]

Looking further into it I realized that we can actually rely on the recipe id being put into the blueprint_provides list within the expansion data. This is even stated in the docs (https://github.com/CleverRaven/Cataclysm-DDA/blob/master/doc/BASECAMP.md#recipe-jsons) and seems to be done by code in recipe::load:

            // all blueprints provide themselves with needing it written in JSON
            bp_provides.emplace_back( result_.str(), 1 );

(it seems the comment has a typo with -> without)