[DONE] [CR] Root Cellar - food preserving option.

[nexusmrsep]

What does it do?

This PR introduces:

1. New constructable terrain: root cellar
2. Mechanics for root cellar to store food in temperature equal to underground temperature. See: PR #24073
3. Side implementation: Two constant temperatures used in calculation were moved to game constants.

Racionale:

Part 1: General idea

Discussion under PR #23986 has led to new ideas, one of which is need for more options to preserve food. It led to an obvious conclusion, that most common food preservation method, nowadays and in the past, is temperature control. Having no electric fridges however, people in the past ages used various methods to keep food cool, of which most common one was using actual ground's temperature in form of dugout shelters, root cellars, etc. Nowadays this form of food preservation still live in form of household basement food storages and wine cellars.

This PR introduces root cellars, and allow them to store food in reduced temperatures (equal to underground temperature = 43F / 6C, compare #24073). Combined with #23986 it offsets the introduction of wide-range of perishable comestibles, alowing long-term storage at reduced decay rates.

Part 2: Sources
Here is a very good Wikipedia article on its purpose.
Compare: here Page 101-102.
and here. Other good source: here
Also: here, and one more.

See intro's and discussion of #23986 and #24073.

Part 3: Why did i use those values?
Construction requires:

• skill 4 survivalist (to know how does it work) and skill 4 fabrication (to know how to build it without collapsing it on your head)
• digging tool and a previously dug deep pit - most important principle of the root cellar is to dig it into the ground, where it can't exchange temperature with earth
• rocks or bricks - they provide rudimentary support preventing dirt walls from collapsing and are a base/support for the insulating roof,
• hammering tool, 2x4's or heavy sticks (roof material, trapdoor), withered plants of piles of straw (insulation) - another principle is to insulate the top of the dugout from outside temperature, by making the roof; roof is then covered with dirt and earth (& rocks, sod), which provides additional insulation. Since dirt is not represented in game, a assume that what was dug out from the pit now lays on the roof, also some of withered plants and rocks may be considered as used for this purpose.
  Think about the final product as a miniature hobbit house, just not as fancy.

[source: https://www.ruralintelligence.com/images/food/root-cellar-prairie440.jpg]

Implementation:

1. Added new 'root cellar' entry to terrain.json
2. Added new construction recipe 'Build Root Cellar' to construction.json
3. This allowed me to modify decay calculators to check if food is placed in root cellar (terrain check), and if so - to calculate rot using underground temperature, as stated above.

[WARNING and HELP NEEDED] This however does NOT work, although it should be plain and simple. This is why I set [WIP] and [CR] on this PR and humbly ask for help. However I would write if ( g->m.tername( location ) == "root cellar" ) in weather.cpp in get_rot_since() function, it would not return proper value. I compiled few versions and added debug messages (not present in pushed code) that would 'print' string for g->m.name(location), g->m.tername(location), and few variants avalaible in map.h/map.cpp, but they returned text: "nothing". Not "" (null), without errors, just plain string "nothing". Did the same to print tripoint x.y.z values of argument location pushed to get_rot_since() function, and it seems to give some coordinates. Spent too many hours trying to crack it down. It must be something trivial that i cant see, or something convoluted, like coordinantes conversion that happen somewhere in between. Other parts of the code seem to use the same method to extract terrain name at a tripoint location, so I'm puzzled. Please help.

EDIT: Thanks @Night-Pryanik and @Coolthulhu for setting me on the right path to answer for this.

4 As a side-job I moved base annual temperature to game_constants.h, and did the same with starting temperature value. This may be an overkill but it seems rational and better then leaving naked number inside the code itslef.

[Leland]

Recommend using the pre_note to describe the purpose - as a player I'd be mystified.

[Night-Pryanik]

I think you should use something like this:

ter_id tid = g->m.ter( location );
if( tid ) = t_rootcellar { 
    ... ;
}

[Coolthulhu]

This is called often - at least once per item. This check may be expensive since it affects all such calls.

[nexusmrsep]

Doesn't work anyway in my test compilation, nor for @Night-Pryanik 's idea above.
Currently testing it to its limits to find what's going on.
I might have a lead...

The front of my test function now looks like this:

ter_id terrain = g->m.ter( location );
add_msg( m_info, "Debug TP X: %s", location.x);
add_msg( m_info, "Debug TP Y: %s", location.y);
add_msg( m_info, "Debug TP Z: %s", location.z);
add_msg( m_info, "Debug MS: %s", g->m.getmapsize() );
if ( terrain == t_rootcellar ) {
    add_msg( "Debug 1: PING" );
    add_msg( m_info, "Debug 1: T: %s", terrain.id().c_str() );
    add_msg( m_info, "Debug 1: RC: %s", t_rootcellar.id().c_str() );
    for( time_point i = start; i < end; i += 1_hours ) {
    ret += std::min( 1_hours, end - i ) / 1_hours * get_hourly_rotpoints_at_temp( AVERAGE_ANNUAL_TEMPERATURE ) * 1_turns;
    } 
    return ret;
} else { 
    add_msg( "Debug 1: PONG" );
    add_msg( m_info, "Debug 2: T: %s", terrain.id().c_str() );
    add_msg( m_info, "Debug 2: RC: %s", t_rootcellar.id().c_str() );
}
if ( terrain == t_pit ) { add_msg( "Debug 2: PIT PIT PIT" ); }

RESULT:
X = 678, Y = 1983, Z = 0
MS=11 (?)
Debug 2: T: t_null
Debug 2: RC: t_rootcellar

t-null --> this suggests that map::ter() returns t_null, and that might be becouse XYZ are !inbound(), but why? maybe the strange getmapsize() result? [edit: not at all strange, its a game constant...]

[Coolthulhu]

The huge coords mean that you're getting either absolute coords or coords in overmap. But map::ter expects local coords, which are not always available here (rot processing can happen when local map doesn't even exist).
This means that the approach with root cellar detection at rot calculation level can't work reliably.

[nexusmrsep]

I found the answer: 'tripoint location' argument pushed to the function should be first converted by map:getlocal() to local coordinates. I knew it, I knew it must have been coordinates conversion

[nexusmrsep]

This is called often - at least once per item. This check may be expensive since it affects all such calls.

How to avoid it then? I think it has to check where the item is located. Also for the sake of future PRs, if I follow Kevin's idea on storing food in proper furniture, I would use this method to discern where is it located, and if such location is valid.

[Coolthulhu]

You'd need to pass it from above, where item location in local coords is still available.
If you do it this way, make sure all paths go through the same functions, to avoid hard to find bugs where rot is different depending on how is it processed.

[Coolthulhu]

The special case of root cellar is different from items just stored underground. Instead, the root cellar always keeps the items at exact temperature, regardless of all environment.

[nexusmrsep]

Ok, after few hours I managed to narrow down and fix the problem. Map coordinates conversion is a pain! Thank you very much @Night-Pryanik and @Coolthulhu for reviewing and pointing me to the right direction on this.

[Leland]

This is helpful but not exactly what I was referring to.

A "root cellar's" purpose isn't evident in its name. A user would have to construct the terrain and then examine it (and read the description) in order to determine it's purpose.

The pre_note would be a good place to make that evident from the beginning.

For instance, You first need a deep pit to construct a root cellar, and enable the storage of food in a cold environment.

Wordy, but you get the gist :)

[nexusmrsep]

@Leland
No, it would'nt. The sole purpose of a pre-note is to give some info on extra costruction/deconstruction requrements.
What you ask is actualy true for all constructions. Solution to this would be expanding construction menu with the description of a constructed item, so you will get the idea of what actualy you are constructing.

This deserves a separate PR and i will try to approach it, as it bothers me too.

[nexusmrsep]

@Coolthulhu

You'd need to pass it from above, where item location in local coords is still available.
If you do it this way, make sure all paths go through the same functions, to avoid hard to find bugs where rot is different depending on how is it processed.

Following your statement I reworked functions above calc_rot() to push local coordinates (instead of global abs) down to it, and in consequence down to get_rot_since(). Then get_rot_since() either uses those coordinates or converts them to global by getabs() where needed.

This I believe solves the problem you mentioned.

[Coolthulhu]

You forgot at least one use case of calc_rot. Double check everything - changing coord type is one of the least safe operations, because the compiler won't warn that you're supplying the wrong type.

[Coolthulhu]

Extract the getabs to a variable to avoid calculating it in a loop if the compiler fails to notice that it doesn't change.

[Coolthulhu]

Scrap the loop. Just calculate it all in one go.
Also, indentation.

[nexusmrsep]

Extract the getabs to a variable to avoid calculating it in a loop if the compiler fails to notice that it doesn't change.
Scrap the loop. Just calculate it all in one go.

Done & done. Also I merged underground rot calculation into one sub-routine as this is more elegant way to do it, when the previous results would be the same anyway.

You forgot at least one use case of calc_rot. Double check everything - changing coord type is one of the least safe operations, because the compiler won't warn that you're supplying the wrong type.

I've double-checked and all calc_rot calls seems covered:

1. player::eat
2. player::process_food
3. map::has_rotten_away (with recursive use)
4. item::process_food

Where is the missing one?

[Coolthulhu]

Don't assign ret if you return it on the next line. Move the ret declaration below the block, in this block return the value you'd assign to ret without assigning it.

Also, check for location.z before checking for terrain type. If one check in if is orders of magnitude faster and more common than the other, it should be moved to front so that it is eliminated faster.

[nexusmrsep]

good point, thanks

[nexusmrsep]

I have also done some rudimentary testing of the compiled version: in summer season one pizza was dropped in the root cellar, another was dropped on adjacent tile. As expected one in the cellar was decaying in a slower pace that the one outside. No apparent errors occured.

[nexusmrsep]

Can I ask for the update on this PR? [edit: on the current state. Is there more I need to do on my side?]

[ZhilkinSerg]

I will try to test it on weekend.

[nexusmrsep]

Side note: if #24555 works, then I think @Coolthulhu 's advice for pushing coordinates "from above" to temperature calculations would become obsolete and I could trim this PR by half, using instead the new_game trick (root cellar check would be done only if new_game == false).

[nexusmrsep]

Word of comment here: the "revert revert revert" commits are actually the above-mentioned trim after #24555. Since the new walk-around method worked there, I discarded all changes that pushed location data from above to get_rot_since from calc_rot, while applying the new method in conflict resolving branch merging commit.

That is good news overall, as this PR is now not as risky, and in fact pretty straightforward.

[ZhilkinSerg]

Two nachos (one in root cellar and one in player inventory) rot at the same speed.

[nexusmrsep]

Two nachos (one in root cellar and one in player inventory) rot at the same speed.

I'll take a look in a minute.

[nexusmrsep]

Fixed. It seems in all this pursuit after current upstream I didn't noticed that one of tripoints were not in the right scale.

[ZhilkinSerg]

Looks good now.

[nexusmrsep]

Looks good now.

Glad to hear that. Hallelujah.

[sfsworms]

Mmmmh, there seems to be an issue with NPC: they don't recognize the root cellar as a furniture and when asked to clean the camp they remove the food from there.

[ZhilkinSerg]

That is because root cellar is terrain, not a furniture.

[sfsworms]

Oh, make sense. Slightly annoying though. Should I create an issue for it?