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

Refactor how nutrients are stored and processed #35889

Merged
merged 5 commits into from
Dec 9, 2019

Conversation

jbytheway
Copy link
Contributor

Summary

SUMMARY: Infrastructure "Refactor how nutrients are stored and processed"

Purpose of change

While working towards a fix for #28180, I found the code working with nutrients to be difficult to follow and with different aspects mixed in the same function. This is an attempt to separate concerns to make it easier to work on #28180.

Describe the solution

  • Existing struct nutrients renamed food_summary and a new nutrients struct introduced for a subset of its members.
  • Use that new nutrients struct in islot_comestible.
  • Overload arithmetic operators on nutrients to make it easy to perform nutrient-related calculations.
  • Refactor the code which computes item nutrients and deals with digestion to use this new struct.
  • In particular, separate out the logic for default base nutrients from the code which combines nutrients for components of crafted food.

Most of this is intended to have no observable change to game behaviour, with one exception: previously the tapeworm parasite halved all vitamins a Character consumed. Now it halves both the vitamins and the calories.

While testing, I discovered that the stave_test was buggy -- it didn't actually test anything, and it had the potential to get stuck in an infinite loop. I fixed it, but in so doing found that it was already failing on master. Players are starving faster than the test indicates they ought to (30 days vs 46). This may be a problem, but it's orthogonal to this PR. @KorGgenT might want to look into that.

Describe alternatives you've considered

Could have included water content in the nutrients struct, but currently water is not calculated based on components so it didn't really fit with the code structure.

Testing

Unit tests only.

@Davi-DeGanne
Copy link
Contributor

Davi-DeGanne commented Dec 6, 2019

Regarding this:

The only intended observable behaviour change from this commit is that
tapeworms now consume half of the calories you ingest, as well as half
the vitamins (they used to only affect vitamins)

I actually had the exact same line of reasoning when I did the stomach refactor (#35143) but I discovered I was wrong -- tapeworm does effect calories AFAICT, so make sure this refactor doesn't make tapeworms twice as effective as intended.

Other than that, I think this change is awesome. Adding arithmetic functions to nutrients was a great idea, it will really help my efforts when I implement my planned changes to stomach processing speeds.

@ZhilkinSerg ZhilkinSerg added [C++] Changes (can be) made in C++. Previously named `Code` Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style Items: Food / Vitamins Comestibles and drinks labels Dec 6, 2019
src/consumption.cpp Outdated Show resolved Hide resolved
src/consumption.cpp Outdated Show resolved Hide resolved
src/consumption.cpp Outdated Show resolved Hide resolved
@jbytheway
Copy link
Contributor Author

I actually had the exact same line of reasoning when I did the stomach refactor (#35143) but I discovered I was wrong -- tapeworm does effect calories AFAICT, so make sure this refactor doesn't make tapeworms twice as effective as intended.

I'm fairly sure hunger_chance only affects how hungry the player feels, not how much calories they get. Hard to say for sure because the relevant code is quite abstracted. I'll see if I can easily add a test for this.

@jbytheway
Copy link
Contributor Author

Thanks for the review BevapDin. I was only moving that code from one place to another, but indeed there was a lot worth improving in it.

@jbytheway jbytheway force-pushed the nutrient_struct branch 3 times, most recently from 1837243 to 719b267 Compare December 7, 2019 12:46
@jbytheway
Copy link
Contributor Author

I think I've now resolved all the above comments.

@Davi-DeGanne
Copy link
Contributor

Well, your test as written wouldn't have properly discovered whether my concern was valid or not, since effects are processed over time, and you don't pass any time at all in the test.

That said, I have read through the code and determined that unless there's something really weird going on, you are right -- hunger_chance and hunger_min don't effect calorie intake/absorption.

@jbytheway
Copy link
Contributor Author

Yes, I realise it was a bit of a weak test. Thanks for taking the time to check and reassure us both.

- Ensure the test doesn't run forever.
- Make the test actually do something.
- Update expected_day to match current behaviour.
Add a new nutrients struct, which consists of some calorie and vitamin
info.  Use this struct in food item definitions (specifically,
islot_comestible) and in stomach.

Turned out stomach already contained a nutrients struct, which contained
the above together with water and total volume.  That is renamed
food_summary, and uses nutrients internally.

The main advantage of this separate struct is that it simplifies the
arithmetic used to compute aggregate food nutrient values based on
components used to craft that food.  That code was also refactored and
split up to make it easier to understand and helps separate out the
things which will need to be separate to work on CleverRaven#28180.

The only intended observable behaviour change from this commit is that
tapeworms now consume half of the calories you ingest, as well as half
the vitamins (they used to only affect vitamins).
This function was unnecessarily messy for no particularly good reason.
src/consumption.cpp Outdated Show resolved Hide resolved
@kevingranade kevingranade merged commit 5014a90 into CleverRaven:master Dec 9, 2019
@jbytheway jbytheway deleted the nutrient_struct branch December 10, 2019 02:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[C++] Changes (can be) made in C++. Previously named `Code` Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style Items: Food / Vitamins Comestibles and drinks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants