feat(balance,port): Mainline Nonperishable Overhaul's changes to item perishability

[RobbieNeko]

Purpose of change

I see no good reason why the changes made in Nonperishable Overhaul shouldn't be mainlined, as the balance of the game isn't exactly held together using the fact that hardtack spoils in 360 days as the glue.

Describe the solution

Mainlines the changes to perishability found in the mod Nonperishable Overhaul by Chaosvolt

Describe alternatives you've considered

• Continuing to let items that have no business spoiling spoil

• Continuing to wait for Chaosvolt himself to port it
  He has enough other things that he really ought to be mainlining (or at least in-repoing), I figured he'd appreciate me taking one of the more minor ones off of his plate.

Testing

Load tested, the game did not yell at me

Additional context

Permission gained from Chaosvolt via him dooting on my message declaring my intent

Original mod: https://github.com/chaosvolt/CDDA_Nonperishable_Overhaul/tree/master/Nonperishable_Overhaul_BN

[RoyalFox2140]

This is going to require a large balance audit and a player vote as per the rules of discussing large balance changes that we decided on. @scarf005 Needs to poll it. @KheirFerrum @chaosvolt pinging relevant parties that will be interested or are involved.

Personally, I've always used the mod and I think community sentiment is that it's effectively vanilla, but we have to see what people think.

[RoyalFox2140]

Overall this is fine if "spoils_in": "0 days", doesn't throw an error, but IIRC it usually does throw an error along the lines of not updating spoilage time or that it can't be 0." A second issue is that items without a spoil time defined can inherit from parent items, so some of these items may inherit. Every single change item with a wiped out field has to be sanity checked.

The JSON is trivial and will work if the above issue causes no errors, So I give it my approval once it's further load tested.

[RoyalFox2140]

I'm pretty confident spoils in 0 errors out. This needs to be load tested.

[RobbieNeko]

As described in the PR body, I did load test this. It does not error, and works just fine.

As you can see, no spoiling

[KheirFerrum]

I've never played with the mod myself. Food is honestly not an issue in base game, and I've heard of thoughts from various devs about making food more of a concern to players. I'll just let them weigh in on it, this kind of balance lies a bit far from my interests.

[chaosvolt]

Whoo boy, I figured you meant like make it an in-repo mod rather than vanilla, but guess this works. Checking for cases where it'd be better to omit spoils in rather than specifying explicit zeroes will likely be needed it seems.

I did have some plans aimed to make the mod basically no longer nessecary, but those have stayed far down on the list for now since I dunno how well they'd work so mainlining it as a bandaid may be reasonable. That said I'd probably suggest focusing on foods that last a default in-game year (14x4 or 56 days), some of the stuff made nonperishable in the mod may be better off resolved by other solutions like introduction of sealed plastic bags that preserve contents, or the code idea that's supposed to reset rot timing to counteract the main reason that prompted the mod being added.

If anyone wants to take a stab at it sometime, I saved the idea @Coolthulhu outlined in my todo list, idea was roughly:

crafting.cpp, complete_craft. const double relative_rot = craft.get_relative_rot(); would need to be changed to be 0.0 if making (the recipe) has a flag like "RESETS_ROT". For example,

const double relative_rot = making.has_flag("RESETS_ROT") ? 0.0 : craft.get_relative_rot();

Then the recipes with this flag would need to get the flag that makes them uncraftable with rotten stuff.

I hadn't tested implementation yet, ideally it would only reset relative rot to the last "stage" reached (i.e. (fresh) into perfectly fresh, no-tagtext into the exact point where (fresh) disappears, (old)` into the exact point where that tagtext shows up, etc), that might be more complex to add however.

[RobbieNeko]

I omitted spoils_in in every situation where I didn't see a copy-from, since omitting it completely would just use the copy-from's which is the exact opposite of desired (however, I recognize that I might have missed a situation where it could be omitted or missed one where it shouldn't be omitted for the time being)

[chaosvolt]

Ye, just basically avoid whenever it might cause a "overriding inherited value with same result" issue, I'll have to check it in a bit I guess.

[RobbieNeko]

Uh oh, I haven't a clue why those assertions are failing in the build. o.O

[RobbieNeko]

Ye, just basically avoid whenever it might cause a "overriding inherited value with same result" issue, I'll have to check it in a bit I guess.

Only ran into one of those, and that's what the "fix jerky error" was about lol

[RobbieNeko]

I've never played with the mod myself. Food is honestly not an issue in base game, and I've heard of thoughts from various devs about making food more of a concern to players. I'll just let them weigh in on it, this kind of balance lies a bit far from my interests.

Tbf, most of the stuff that was touched by the overhaul was stuff that already had very long times anyway. A lot of it was dried stuff that basically had a year of perish time already. So it's really more of a QoL for foods that shouldn't perish anyway.

[chaosvolt]

Should be reasonable then, lemme I guess check to see what errors are left and if they're related, then others can sort of if they'd prefer this or not. ^^

[RobbieNeko]

Overall this is fine if "spoils_in": "0 days", doesn't throw an error, but IIRC it usually does throw an error along the lines of not updating spoilage time or that it can't be 0." A second issue is that items without a spoil time defined can inherit from parent items, so some of these items may inherit. Every single change item with a wiped out field has to be sanity checked.

The JSON is trivial and will work if the above issue causes no errors, So I give it my approval once it's further load tested.

I have load tested, "spoils_in": "0 days" works absolutely fine and throws no errors (unless it's the same as the inherited value, in which case it just errors to let you know that it'd be best to just not have it there)

[RobbieNeko]

Should be reasonable then, lemme I guess check to see what errors are left and if they're related, then others can sort of if they'd prefer this or not. ^^

Cool! But yeah, my apologies for making you initially think I meant to mainline this one haha. I just figured these changes were relatively simple and not likely to draw any objections due to how it mostly affects stuff with long timers anyway ^-^

[chaosvolt]

Looks like test failures, the only one that seems to be relevant is most likely testing stacking items and probably using an item that was previously perishable I assume. I'll glom onto the branch in a sec I guess and see what the test is doing XD

[RobbieNeko]

Finally, the tests are not yelling at me/us!

[RobbieNeko]

The poll for this PR is here

[chaosvolt]

Side note: looks like potted meat (in meat_dishes.json) still has its spoils_in of 60 days set.

[RobbieNeko]

Side note: looks like potted meat (in meat_dishes.json) still has its spoils_in of 60 days set.

... What with where it was placed in NPO and the fact that I couldn't find jerked offal either, I had assumed that potted meat was an MST thing. Oops, haha

[chaosvolt]

Side note: looks like potted meat (in meat_dishes.json) still has its spoils_in of 60 days set.

... What with where it was placed in NPO and the fact that I couldn't find jerked offal either, I had assumed that potted meat was an MST thing. Oops, haha

It used to be before it was mainlined, yeah ^^"

[RobbieNeko]

I suppose I'll do potted meat too once I get back from town ^-^

[scarf005]

LGTM.

I think people are positive regarding this change.

[RobbieNeko]

LGTM.

I think people are positive regarding this change.

Agreed, I highly doubt we're going to see a bunch of 'no' votes come from out of nowhere at this point. Seems people like the idea quite a lot ^-^
Closed the poll as resolved.