Basecamp: Cull competing constructions from bulletin board lists

[PatrikLundell]

Is your feature request related to a problem? Please describe.

Bug or feature request?

Currently alternatives to currently in-progress constructions are available for selection on the bulletin board. While I haven't tested it, I assume it is possible to start multiple variants in parallel and the end result will be the one that's accepted for completion last (replacing the previous one), resulting in wasted man-power and material.

Examples of such alternatives are the construction of a cooking place in the form of a stove/fire place/brazier and the construction of "the same" buildings from different construction materials.

Solution you would like.

When a construction is started, "competitors" should not be available. If the job is cancelled all alternatives should be returned (including the one cancelled).

Describe alternatives you have considered.

Continue to be careful to verify that I'm not accidentally starting the "same" construction twice.

Additional context

I suspect this would make the population of the bulletin board entries even slower, but possibly not by much. I suspect it would be a matter of recording all properties in-progress constructions would remove as the in-progress tasks are entered into the lists and then make a second pass over all entered entries and cull those that wouldn't have been added if the additional restrictions had been in place, except, of course, for the in-progress entries themselves.

[silvadomi]

Maybe can be fixed simply by checking the basecamp_upgrade.in_progress flag in basecamp::get_available_missions_by_dir. This is assigned by basecamp::available_upgrades but never looked at in basecamp::get_available_missions_by_dir.

As far as I understand the in_progress data is assigned when an upgrade is started. Might need to verify that it's being reset correctly though when an upgrade mission is cancelled without being completed (emergency recall).

[PatrikLundell]

The flag has to be reset for the task to become available again after having been cancelled.

The problem is to examine the in progress task to temporarily apply the state changes the completed job would apply (provides and whatever the disabling of provide is called) and evaluate all potential tasks against this future state rather than the current state. It would also have to take multiple concurrent tasks into consideration, as it's possible to work on multiple tasks in parallel.

[silvadomi]

There's also #36365 which seems to be related and can probably be fixed together.

[silvadomi]

I wanted to look further into this, but it is unclear what the attribute in_progress of the basecamp expansion data structure (https://github.com/CleverRaven/Cataclysm-DDA/blob/cdda-experimental-2024-06-12-1214/src/basecamp.h#L57) is meant to represent. One would think it tracks ongoing upgrades, but it is only changed when starting an upgrade, not when completing one, nor when cancelling one through emergency recall.

In addition, the entries in provides seem to represent counts of basecamp features that are added during an upgrade path. In a way it can be understood as which "level" of basecamp features has been achieved through upgrades, I think.

Due to that, I'm unsure how to interpret the in_progress attribute. Should it be the amount (level) of basecamp features that is achieved when the current upgrades are completely? So after the upgrades, we expect provides to be the same as in_progress for the given feature? Or should it be the amount that is added to the level of basecamp features after completion of the current upgrades? I think the second interpretation would make more sense, but that requires that the in_progress amounts are reduced after completion / cancellation of an upgrade mission.

Generally, is there a document / discussion that summarizes the logic behind the blueprint provides / excludes values and how they are tracked in the saved expansion data during an upgrade path? So far I haven't seen values larger than 1 being relevant with the current game data, but such a case would be interesting to look at for this issue.

[PatrikLundell]

Yes, the in_progress one looks odd.

The "provides" is a sort of counter, and it used to add beds to the camp for evaluation against whether camps could get expansion (you needed 4 beds, rounded down, for each expansion). I don't think anything else is counted beyond present or not (and it can be noted that some of these cases were implemented erroneously, so the number of "beds" was far higher than the actual number added).

I believe the "excludes" value is used to block finished tasks from being done again (so farming should presumably do something to ensure it's repeatable), because you don't want your finished building to be built again.
It can also be noted that the recipe name is added automatically and excluded automatically on completion, so the explicit doing of it should be redundant (but it's a lot clearer than having it "randomly" dependent on whether the values used for progress determination are the recipe one or not.

I don't know how well documented this is, but any documentation should reside in the doc folder (in the .md format I don't know how to display in a useful fashion, but text works as a fallback).

Edit:
basecamp.cpp line 383 might be useful:

                // track buildings that are currently being built
                if( e_data.in_progress.find( bp_exclude.first ) != e_data.in_progress.end() ) {
                    if( e_data.in_progress[bp_exclude.first] >= bp_exclude.second ) {
                        in_progress = true;
                        break;
                    }
                }

[silvadomi]

Yes, I've been puzzled by exactly that code piece. The bp_exclude there loops over all elements of the blueprint_excludes list from the recipe definition. However, it doesn't seem to make a lot of sense to exclude camp functionality before the corresponding upgrade is completed. For example, if I added a camp feature that adds a spinning wheel I might want to exclude previous recipes for making ropes by hand, but only after that upgrade is complete.

Instead it should be better to just check whether the currently considered recipe id is among the in_progress list. I'll have to think about how amounts should be handled there, especially since some upgrade recipes spuriously put their own id in the blueprint_provides field.

Anyways, the current game behaviour should not be affected at all by it, because the in_progress variable that is defined there is never used afterwards.

(and it can be noted that some of these cases were implemented erroneously, so the number of "beds" was far higher than the actual number added)

Might have been because all upgrade recipes already provide their own id with an amount of 1 without anything being written in the json definition, so if there's an additional entry with the same id in the blueprint_provides field, it would be added with a one higher amount (e.g., 2 instead of 1). That is still the case, so one might want to check that no blueprints have their own id in the blueprint_provides field to avoid issues like that down the line.

Also, to me it seems that many basecamp json definitions are not aware of that, and simply use the blueprint_provides and blueprint_excludes field to define a possible upgrade path. This would not at all be necessary if one takes into account that the recipe id is already registered in provides and excludes. For example the modular farm version 2 uses a lot of spurious provides and excludes, whereas the farm version 1 only puts the required information in that regard (which are the prerequisites for each recipe).

[PatrikLundell]

No, I don't remember exactly what was done wrong, but they ended up using the wrong number, possibly along the line of beds += beds + 4, so it was clearly wrong, and then copied to various places.

We won't have issues with counts until someone finds a reason to make use of that functionality (again), and then it's up to the implementors to make sure they know what they do (or ask for assistance).

Yes, I deliberately added the extra instances to make it explicitly clear what the variables in use were. I'm not a fan of hidden functionality that's hard to understand unless you've read the fine print (assuming said fine print even exists). If it was up to me I wouldn't have implemented the automatic addition and blocking of blueprint ids.

[silvadomi]

Yes, I deliberately added the extra instances to make it explicitly clear what the variables in use were. I'm not a fan of hidden functionality that's hard to understand unless you've read the fine print (assuming said fine print even exists). If it was up to me I wouldn't have implemented the automatic addition and blocking of blueprint ids.

Ok, that makes sense. I actually just have a patch in my local repository that would only add the recipe id automatically if it's not yet explicitly in the json. I'll probably integrate it into a PR that tries to fix this issue (and some related ones) over the next days. Sometimes the id's are also different though between the recipe result and the blueprint_provides, but that probably does not really matter.

With that the amounts should be interpretable in a clearer way, and then I'd probably just go ahead and interpret the in_progress attribute as only the upgrades that are currently under construction, meaning they get removed from there upon completion or cancellation of the mission.

[silvadomi]

Regarding the constructions of the same buildings with alternative materials, this is also reported in #39070.