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

Stomach code refactor #35143

Merged
merged 9 commits into from
Nov 10, 2019
Merged

Conversation

Davi-DeGanne
Copy link
Contributor

@Davi-DeGanne Davi-DeGanne commented Oct 29, 2019

Summary

SUMMARY: Infrastructure "Refactor stomach code for readability and reduced complexity"

Purpose of change

Overall, the stomach_contents class suffered from over-complexity. Let me explain some of the issues it has:

  • The most important and commonly performed task, digesting food every half-hour, requires calling four specific methods in a specific order. They are never called separately; there is no good reason not to combine these into one function.
  • There is a distinction between absorbing nutrients and passing nutrients. This distinction is unnecessary because any given stomach_contents object only performs one of the two (very technically, this isn't true, but it is true in terms of actual perceivable effect).
  • There is a lot of code that just shuffles numbers around without actually doing anything. For example, stomach_contents objects' absorption rates are zero if they are stomachs (as opposed to guts), but they still spend the same amount of time as guts-types calculating how many nutrients to absorb. (It doesn't matter how many times you multiply zero by zero, it's still zero.) Another example is the passing of material from the guts, without absorption; while the notion of coding bowel movements is, granted, rather amusing, the fact that it has no actual in-game effect (and likely never will) means it is wasted CPU cycles.
  • Users of the class require intricate knowledge of its implementation in order to use it properly.
  • The class is bimodal (stomach-type and guts-type) and handles it in convoluted ways like extremely similar method overloads, when a simple boolean member variable set by the constructor would be a much more efficient solution.
  • There are several methods that aren't used anywhere.

Refactoring this code will make it much easier to maintain imho.

Describe the solution

Changes (in the order they were applied):

  • Commit 63936e8: Removed all unused methods.
  • Commit aaf8a46:
    • Combined store_water, calculate_absorbed, store_absorbed, and bowel_movement into one method, digest. This new method just returns the nutrients that were processed, and leaves it up to the method caller to decide what to do with them.
    • Removed calories_absorbed and vitamins_absorbed, as these were no longer needed after the above change.
    • Combined pass rates and absorb rates: stomach-types never absorbed anything, and guts-types passed so slowly as to be inconsequential, so these concepts were combined into digest_rates along with their associated structs and construction methods.
    • Whether a stomach_contents is a stomach or guts is now passed to the object's constructor rather than being controlled via method overload.
    • These changes fix a bug where only something like 75% of vitamins consumed were absorbed into the body. Now, 100% DV truly means 100% DV.
  • In light of the fixed bug mentioned above, I updated the all_nutrition_starve_test in commit b006b3b. It will now fail if it detects abnormally low vitamin levels after 20 days of exactly 100% DV per day (it was previously far less strict).

Describe alternatives you've considered

I really wanted to additionally replace the player& argument that several methods have with a const member variable provided by the constructor, but I kept running into issues with that that I couldn't figure out.

Testing

It passes all the unit tests related to eating and hunger, which are quite specific and would probably catch if something was terribly wrong. I have also played a character for a week of in-game time with the changes, and haven't encountered any bugs or unexpected differences from the previous functionality.

Davi and others added 3 commits October 28, 2019 01:46
There were several issues with the previous implementation of the
stomach_contents class. For instance, using the class required intricate
knowledge of its implementation. Additionally, the most important and
commonly performed task, digesting food every half-hour, required
calling four specific methods in a specific order, when they could
easily be combined into one method. Overall, the class suffered from
over-complexity and I decided to fix this by making the following
changes:

Combined store_water, calculate_absorbed, store_absorbed, and
bowel_movement into one method, "digest".

Removed references to calories_absorbed and vitamins_absorbed, as these
are no longer used after the above change.

Combined pass_rates and absorb_rates: stomach-types never absorbed
anything, and guts-types passed so slowly as to be inconsequential, so
these concepts were combined into digest_rates.

Whether a stomach_contents is a stomach or guts is now decided when the
object is constructed; it was already that way in practice, I just made
it official.
Now that vitamins aren't disappearing, this test can be tuned to make
sure that vitamins are being absorbed into the body correctly.
src/stomach.h Outdated Show resolved Hide resolved
src/stomach.cpp Outdated Show resolved Hide resolved
src/stomach.cpp Outdated Show resolved Hide resolved
src/stomach.cpp Outdated Show resolved Hide resolved
src/stomach.cpp Outdated Show resolved Hide resolved
src/stomach.cpp Outdated Show resolved Hide resolved
@ZhilkinSerg ZhilkinSerg added [C++] Changes (can be) made in C++. Previously named `Code` Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style labels Oct 30, 2019
Davi-DeGanne and others added 2 commits October 30, 2019 07:30
- Replace auto with type specification.
- Add const where applicable
- Replaces 'player' type parameters with 'needs_rates'

Co-Authored-By: Curtis Merrill <curtis.r.merrill@gmail.com>
set_thirst( std::max(
-100, get_thirst() - units::to_milliliter<int>( digested_to_guts.water ) / 5 ) );
guts.ingest( digested_to_guts );
if( !is_npc() || !get_option<bool>( "NO_NPC_FOOD" ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if( !is_npc() || !get_option<bool>( "NO_NPC_FOOD" ) ) {
if( !( is_npc() && get_option<bool>( "NO_NPC_FOOD" ) ) ) {

IMHO this is more readable. "If it's not an NPC and we're using no NPC food" as opposed to "if it's not an NPC or we're not using no NPC food".

Copy link
Contributor Author

@Davi-DeGanne Davi-DeGanne Oct 31, 2019

Choose a reason for hiding this comment

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

"If it's not an NPC and we're using no NPC food"

No, this isn't a correct articulation of your version. That wording would represent

if( !is_npc() && get_option<bool>( "NO_NPC_FOOD" ) ) {

which isn't what we want. Your code itself is logically equivalent to mine, but I don't agree that yours is more readable.

I do agree that mine isn't particularly readable either though, I'll add a comment that explains it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah the parenthesis don't come across in the word version I wrote earlier. My code is more like "Unless it's an NPC and we're using No NPC Food". Instead of a comment it could move to a should_process_food method and then if( should_process_food() ) {.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I still have to disagree, I think my version is more readable, and I don't want to add a function to a class we're trying to deprecate. I did add a comment though. I'll leave this conversation unresolved though so whoever goes to merge this can let me know if they think a change needs to be made.

@Davi-DeGanne
Copy link
Contributor Author

Marking this as [WIP] until I manage to extirpate all references to the player class from stomach.cpp and stomach.h to help with #34721

@Davi-DeGanne Davi-DeGanne changed the title Stomach code refactor [WIP] Stomach code refactor Oct 31, 2019
The previous ingest function in stomach.cpp used the 'player' object,
which we are moving away from. Since the only call of that function was
in the 'player' scope, it was trivial to move that code there and call
the new, more generalized ingest function instead.
Stomach capacity was always based on the player's mutations, no matter
who owned said stomach. Now the capacity function takes a Character
reference as a parameter, and uses that Character's mutations instead.
@Davi-DeGanne Davi-DeGanne changed the title [WIP] Stomach code refactor Stomach code refactor Oct 31, 2019
@Davi-DeGanne
Copy link
Contributor Author

Okay I'm happy with this now.

@ZhilkinSerg ZhilkinSerg merged commit c7c63d0 into CleverRaven:master Nov 10, 2019
AMurkin pushed a commit to AMurkin/Cataclysm-DDA that referenced this pull request Nov 13, 2019
* Remove unused methods from stomach.cpp

* Combine several methods so stomach.cpp is simpler
There were several issues with the previous implementation of the
stomach_contents class. For instance, using the class required intricate
knowledge of its implementation. Additionally, the most important and
commonly performed task, digesting food every half-hour, required
calling four specific methods in a specific order, when they could
easily be combined into one method. Overall, the class suffered from
over-complexity and I decided to fix this by making the following
changes:

Combined store_water, calculate_absorbed, store_absorbed, and
bowel_movement into one method, "digest".

Removed references to calories_absorbed and vitamins_absorbed, as these
are no longer used after the above change.

Combined pass_rates and absorb_rates: stomach-types never absorbed
anything, and guts-types passed so slowly as to be inconsequential, so
these concepts were combined into digest_rates.

Whether a stomach_contents is a stomach or guts is now decided when the
object is constructed; it was already that way in practice, I just made
it official.

* Make all_nutrition_starve_test far more strict
Now that vitamins aren't disappearing, this test can be tuned to make
sure that vitamins are being absorbed into the body correctly.

* Implement suggestions from KorGgenT
- Replace auto with type specification.
- Add const where applicable
- Replaces 'player' type parameters with 'needs_rates'

Co-Authored-By: Curtis Merrill <curtis.r.merrill@gmail.com>

* Remove unused #includes

* Refactor ingest function
The previous ingest function in stomach.cpp used the 'player' object,
which we are moving away from. Since the only call of that function was
in the 'player' scope, it was trivial to move that code there and call
the new, more generalized ingest function instead.

* Fix bug with stomach capacity calculation
Stomach capacity was always based on the player's mutations, no matter
who owned said stomach. Now the capacity function takes a Character
reference as a parameter, and uses that Character's mutations instead.
AMurkin added a commit to AMurkin/Cataclysm-DDA that referenced this pull request Nov 13, 2019
@Davi-DeGanne Davi-DeGanne deleted the stomach-refactor branch November 23, 2019 04:56
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants