Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend vitamins implementation [RDY] #16577

Merged
merged 11 commits into from
May 18, 2016
Merged

Conversation

mugling
Copy link
Contributor

@mugling mugling commented May 8, 2016

Each deficiency now has three intensities. The first is intended as a warning state without penalties, followed by a second penalty state then a third more serve penalty state.

The UI is considerably enhanced in that modifying vitamins (via food consumption, multivitamins or innate requirements) results in immediate status changes. Appropriate decay_messages are provided to further guide the player with a small extension required to effect::set_intensity to ensure their display.

NPC's continue to track vitamin levels but are incapable of acquiring vitamin diseases.

The standard vitamin consumption rate is 1 unit/hr. Deficiency times are adjusted based upon initial feedback with the below values expressed in days. It would be helpful if someone could link this post to the forum until more complete documentation is written.

vitamin warning penalty severe
calcium 10 12 12 14 15 16
iron 10 12 12 14 15 16
vitA 7 10 9 12 12 14
vitB 4 7 5 9 7 11
vitC 4 7 5 9 7 11

Excluding players with food intolerances the only major difficulty is obtaining calcium. The larger storage pool for this mimics reality whereby you can consume in excess of your daily requirements. milk and it's powdered substitutes are an excellent source with the latter providing in excess of 5 days supply from a single spawn. Additional sources from bone as suggested in #16393 could be a good idea.

Currently hyervitaminosis only occurs for excess of vitA and iron. It never occurs from less than two doses of multivitamins and is improbable from less than four. A future PR will add severity levels and adjust the balance further dependent upon feedback.

The next stage of this PR will handle display of player vitamin levels. These need to be in reasonably chosen units. #16505 added RDA values for foodstuffs. I want to implement both suggestions from @kevingranade and @Coolthulhu for display via the self aware trait and blood analysis bionic. Possibly we could use time until next state (in days/hours)?

For balance I want to stick with the original outline which is to enforce some variation in diet and not to simulate the minutiae. The average player eating multiple food groups should rarely become deficient whereas selecting intolerance traits should make vitamins challenging but possible. The traditional cooked meat/oatmeal diet would be heavily penalised so players wanting to subsist on that will want to use a no-vitamins/simplified nutrition mod.

We should focus our efforts on balancing the foodstuffs and an example of how I'd like to undertake this via inheritance groups is given in #16568. Further vitamins have been suggested but I'd like to avoid this as I the current 5 were chosen as they distributed well across our existing food groups. The implementation would however very much support this as mod content.

As to durations and storage pool sizes I want to go with the split of daily (water soluble) vitamins and storage minerals. I agree with @kevingranade that this shouldn't vary with season length. A lot of in-game values are scaled proportional to reality out of necesssity (gun ranges being the best example). Suggestions for different durations can be considered but the outline goals of enforcing some variety in diet and not simulating minutiae have precedence.

@Coolthulhu has some interesting suggestions as to penalties (#16393) and we need to now consider those. IRL some vitamin deficiencies have permanent/irrecoverable effects and we certainly don't want to implement those. Possibly the first penalty (intensity 2) should always exclude significant combat penalties?

Obsoletes #16402
Would fix #16549 although similar to @i2amroy I cannot reproduce that issue

@mugling mugling changed the title Add support for intensity to vitamin deficiencies Extend vitamins implementation May 8, 2016
@mugling
Copy link
Contributor Author

mugling commented May 8, 2016

aa0b805 adds a mod disabling the vitamins implementation

@mugling mugling changed the title Extend vitamins implementation Extend vitamins implementation [RDY] May 8, 2016
@illi-kun
Copy link
Contributor

illi-kun commented May 8, 2016

For me, a week for getting serious penalties is way too fast. Initial assumption was "to penalize one who eat only cooked meat for months" (as I got it) but now vitamin system affects everyone, so player have to start thinking about vitamins deficiency starting day 0. I'd multiply all values you shown in your table by x3, but it's only my judgement.

@mugling
Copy link
Contributor Author

mugling commented May 8, 2016

@illi-kun The problem with longer periods is that it allows exploits such as a months worth of cooked meat followed by a 1 hour binge on eggs or vitamins.

The above values presume you eat absolutely none of the vitamin whereas IRL many foodstuffs contain at least trace amounts of vitamins so that markedly extends the times.

Durations are already compressed in-game (1 game day ~ 1 IRL week) so it's the relative proportion between vitamins that I want to try and preserve as opposed to the specific times. If the durations are however a problem from a gameplay perspective that needs addressing.

One way of empirically addressing balance might be to use the stats-dumper from #16378 to work out a typical early-game diet. For reference you currently need 24 vitamins per day (1vit/hr) to avoid depleting your vitamin pool.

@mugling mugling mentioned this pull request May 8, 2016
@illi-kun
Copy link
Contributor

illi-kun commented May 8, 2016

The problem with longer periods is that it allows exploits such as a months worth of cooked meat followed by a 1 hour binge on eggs or vitamins.

We need to find a golden mean between this and current state.

Durations are already compressed in-game (1 game day ~ 1 IRL week)

I'd prefer do not use that as a solid ratio of in-game time -to- IRL time. Time in cdda is just different and not comparable with IRL one (especially, with all of these settings of season time, construction time, etc.).

My initial comment was driven by common sense (which causes incorrect conclusions sometimes), so it's more for bringing more attention to the possible balance issue. BTW, it's a very good idea to try to gather actual (objective) data about early game diet and avoid subjective guesses about it.

@i2amroy
Copy link
Contributor

i2amroy commented May 10, 2016

We could bump the minimum penalty time for the fastest vitamins (B and C) up to a week and increase everything else correspondingly. That would meant that you've got at least a week before you need to really start worrying about vitamins (not including any additional time bought by actually eating things), which seems pretty fair to me, and has the advantage of being a nice round number.

That said it would definitely be awesome if some measures of what exactly an "early game" diet generally consists of, at least to get a better idea of the numbers involved.

@kevingranade
Copy link
Member

I haven't looked up durations for the other nutrients, but according to the source I found (linked in that forum thread), vitC deficiency starts being noticeable at 2-3 months. I could see cutting that down to a month or so for the first stage in the name of making the timescale reasonable for the game, but a week from game start is rather excessive.
I don't see how delayed onset implies that a small dosage should immediately cure the problem, if anything it should set the timer back a bit, and you're still in a pretty deep hole. Also, a massive infusion of vitC WILL in fact cure scurvy rapidly, though it takes a little while for physical symptoms to abate. The point is it needs to be a large amount of it, so while something with a minimal amount of vitC will stave off scurvy, you need a better source of it to push the clock back enough to give you real breathing room.

@Mecares
Copy link
Contributor

Mecares commented May 10, 2016

The problem with longer periods is that it allows exploits such as a months worth of cooked meat followed by a 1 hour binge on eggs or vitamins.

Perhaps a cap that limits how much vitamins the body can extract from food per day could limit this problem. Something like even if you eat more, your body can only absorb up 3 to 5 times the daily dosis per day.

@mugling
Copy link
Contributor Author

mugling commented May 10, 2016

We could bump the minimum penalty time for the fastest vitamins (B and C) up to a week and increase everything else correspondingly

This sounds reasonably balanced and is actually what was outlined in my original issue:

Deficiencies occur after 7 days of entirely deficient food (IRL scurvy takes just under 4 weeks)

-- (see #16118)

Something like even if you eat more, your body can only absorb up 3 to 5 times the daily dosis per day.

Implementation of this would be complex, brittle to exploits and somewhat non-intuitive.

vitC deficiency starts being noticeable at 2-3 months

Sure, but we already scale season length proportional to RL days and using the same factor a week for vitC is broadly comparable.

Also, a massive infusion of vitC WILL in fact cure scurvy rapidly, though it takes a little while for physical symptoms to abate.

I'm not sure if I follow how that changes the modeling? What's the definition of cured with respect to the in-game status effect scurvy?

@kevingranade
Copy link
Member

Something like even if you eat more, your body can only absorb up 3 to 5 times the daily dosis per day.

Implementation of this would be complex, brittle to exploits and somewhat non-intuitive.

It's very analogous to eating, when you consume nutrients, they fill up a buffer that has a certain capacity (analogous to a stomach), over time that buffer feeds into the actual value tracking the nutrient. This means you can occasionally binge and get the full benefit, but it automatically caps the size of a binge. I don't think it's a good model for vitC in particular, from what I understand megadoses of vitC work just fine, but it might make sense for other nutrients.

vitC deficiency starts being noticeable at 2-3 months

Sure, but we already scale season length proportional to RL days and using the same factor a week for vitC is broadly comparable.

The explicit reason for season scaling is "so people can experience the full range of seasons". With setting the starting date as an option, there is basically no reason to worry about that anymore, the only reason I haven't proposed setting season lengths to earth normal and removing the option is because I don't care enough to deal with the inevitable prolonged argument that would result. Every time someone uses it as a rationale for making effects super fast, I care more :/

tl;dr "season lengths are short" is not a sufficient rationale for making everything else short. I haven't heard a good rationale for making nutrient deficiency onsets any faster than they are in reality.

Also, a massive infusion of vitC WILL in fact cure scurvy rapidly, though it takes a little while for physical symptoms to abate.

I'm not sure if I follow how that changes the modeling? What's the definition of cured with respect to the in-game status effect scurvy?

Cured in this context means if you take no further actions the symptoms will go away.

@mugling
Copy link
Contributor Author

mugling commented May 14, 2016

I'm reluctant to add more complexity to the model. The design goal from the original issue was to provide a simple mechanism to enforce some variation in diet. IRL nutrition is complex with a lot of disagreement between sources and a more detailed simulation would add be unlikely to add much to gameplay.

I've increased the times in 93e2d4d starting with the suggestions from @i2amroy whilst maintaining the division between short and long-term vitamins. I think further adjustments should be based upon either data about the early player diet and/or player feedback as this is likely to produce a much more balanced model and better meet the design goals.

Further empiric data about vitamin contents of foodstuffs is welcomed especially as part of concise suggestions such as #16677

"disease": [
[ -288, -336 ],
[ -337, -384 ],
[ -385, -720 ]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This format looks rather awkward. Essentially you have a continuous range, split in N pieces. Why not define it with min, max, and levels? You could also have a more meaningful name for a warning threshold than simply min.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

min and max are the lowest/highest levels a vitamin can be set to.

You could also have a more meaningful name for a warning threshold than simply min.

This isn't how the code functions. Each disease entry sets an increasing intensity for the deficiency status.

Copy link
Contributor

@codemime codemime May 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

min and max are the lowest/highest levels a vitamin can be set to.

Then proper names for disease range IMHO would be disease_min and disease_max.

This isn't how the code functions. Each disease entry sets an increasing intensity for the deficiency status.

I saw it. It's rather fragile. You make an assumption that number of levels in vitamin and in effect for diseases are equal (which others most likely will be unaware of. Besides, it's inconvenient). I'd call get_max_intensity() from diseases and use the value to split the range.

Edit: I liked your previous format BTW. It would work nicely with intensities:

"deficiency": [[ "anemia", -300 ]],
"excess": [[ "hypervitaminosis", 100 ]]   // but brackets look redundant

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then proper names for disease range IMHO would be disease_min and disease_max.

min and max are sufficient. There are values within that range that produce no diseases.

You make an assumption that number of levels in vitamin and in effect for diseases are equal

Yes, it's reasonable to expect anyone implementing further vitamins to provide matching severity and diseases levels.

Besides, it's inconvenient

I don't want to start performing micro-optimisations of the JSON until both the implementation is otherwise complete and there is evidence that we intend to add further vitamins.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

min and max are sufficient. There are values within that range that produce no diseases.

I didn't mean renaming. I meant thresholds for diseases, not possible ranges.

Yes, it's reasonable to expect anyone implementing further vitamins to provide matching severity and diseases levels. I don't want to start performing micro-optimisations of the JSON until both the implementation is otherwise complete and there is evidence that we intend to add further vitamins.

It's a matter of non-redundancy, not a micro-optimisation. Your current storage format is extremely redundant (didn't you use calculations to split the range manually?). That provides a room for various errors and slows down balancing, but if you're sure, that your current format is more convenient, I'm not going to argue.

Copy link
Contributor Author

@mugling mugling May 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not more convenient but it is more flexible whilst the design is still being worked on

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least It would make sense to leave only one boundary per segment (to eliminate a possibility of a "miraculous recovery" in the middle of a fully developed avitaminosis). So that instead of:

[[ -288, -336 ],
[ -337, -384 ],
[ -385, -720 ]]

you could write:

[ -288, -337, -385 ]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but until the design is finalised I don't want to rework the JSON again as part of this PR.

To put it into context there are only five vitamin ranges to specify so whilst such optimisations aren't incorrect they aren't sufficiently a priority to trigger at least another round of code review especially given the design of the JSON could quite conceivably be further changed.

@@ -12,6 +12,8 @@
#include "json.h"
#include "itype.h"

extern class game *g;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is g actually used in this header? If not, why is it declared here?

@mugling
Copy link
Contributor Author

mugling commented May 15, 2016

See changes in 03d02ff

@Coolthulhu Coolthulhu self-assigned this May 18, 2016
@Coolthulhu
Copy link
Contributor

When viewing the description of an apple, debugmsg "invalid vitamin VitC" happens.

@Coolthulhu Coolthulhu merged commit a9fd69c into CleverRaven:master May 18, 2016
@Coolthulhu
Copy link
Contributor

The debugmsg problem was introduced by a different PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vitamin deficiencies time out and reapply constantly
8 participants