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

Allow mutations to affect cardio #53603

Merged
merged 2 commits into from
Jan 1, 2022

Conversation

wapcaplet
Copy link
Contributor

@wapcaplet wapcaplet commented Dec 19, 2021

Summary

Bugfixes "Allow mutations to affect cardio"

Purpose of change

Ever since #51880, the max_stamina_modifier mutation attribute (intended as a multiplier on maximum stamina) has been wrongly applied to the cardio fitness calculation (being added rather than multiplied), making the "Languorous", "Indefatigable" and "Hyperactive" mutations have virtually no effect on maximum stamina.

Fixes #52980

Describe the solution

  • Replace the max_stamina_modifier mutation attribute with cardio_multiplier
  • Rebalance multiplier so "Languorous", "Indefatigable", and "Hyperactive" can run approximately the same distances as they could before the cardio system
  • Adjust Character::get_cardiofit() to apply it as a multiplier to cardio fitness, instead of multiplying stamina
  • Document and refactor Character::update_stamina()
  • Add test cases to cover this and related aspects of cardio

Describe alternatives you've considered

The original mutation field max_stamina_modifier is presently only used by these three mutations in vanilla, and one mutation in CRIT ("Persistent Body"). Could keep the field around in deprecated form for a while before removing it entirely, but with so few usages, it seemed simpler to just change it.

Keeping the original name of the field would be misleading, since it affects more than just max stamina now, and it was never just a "modifier" but a "multiplier".

Testing

Run tests/cata_test '[cardio]'

Find some pavement, and run back and forth; count how many steps you can take before getting winded. Try with different mutations.

My measured in-game barefoot running distances on pavement were as follows:

  • Before cardio (20aaf46):
    • Normal: 96
    • Languorous: 70
    • Indefatigable: 126
    • Hyperactive: 145
  • After cardio, before this PR (9da956a):
    • Normal: 84
    • Languorous: 81
    • Indefatigable: 88
    • Hyperactive: 90
  • After this PR:
    • Normal: 84-85
    • Languorous: 66-67
    • Indefatigable: 103-105
    • Hyperactive: 125-127

Additional context

I would like to restore something close to the original balance on these traits, in terms of:

  • maximum stamina (relative to a "normal" character)
  • stamina regen (likewise)
  • how far you can run before getting winded (dependent on the first two)

Maximum stamina used to be exactly 10,000. However, with the new cardio system, maximum stamina depends on cardio fitness level; for a default unmutated character, cardio fitness level is 1738, and maximum stamina is 3500 + 3 * 1738 = 8714.

These mutations used max_stamina_modifier to adjust maximum stamina by these amounts:

  • Languorous: 0.75 * 8714 = 6535.5
  • Indefatigable: 1.25 * 8714 = 10892.5
  • Hyperactive: 1.4 * 8714 = 12199.6

So these (approximate) stamina maximums will be the target for the new cardio_multiplier equivalent.

Since the most measurable in-game effect of maximum stamina (and regen) is how far you can run before getting winded, I would like to also make that as close as feasible to its former (pre-cardio) behavior.

Note See the new test cases for verification on this front.

@wapcaplet wapcaplet added [C++] Changes (can be) made in C++. Previously named `Code` [JSON] Changes (can be) made in JSON Code: Tests Measurement, self-control, statistics, balancing. Mechanics: Character / Player Character / Player mechanics Mutations / Traits / Professions/ Hobbies Mutations / Traits / Professions/ Hobbies labels Dec 19, 2021
@Photoloss
Copy link
Contributor

While we're at it why should either version of the "max stamina" modifier intrinsically increase stamina regen? We have a dedicated mutation property for that (not sure whether that one plays nice with cardio) so it would be trivial to remove this hidden effect and instead spell it out explicitly in the mutation data.

@wapcaplet
Copy link
Contributor Author

While we're at it why should either version of the "max stamina" modifier intrinsically increase stamina regen?

There's detail, discussion, and math at #39994 where it was added. The gist is, we want total stamina regen time to be the same, even if you have more total stamina. So with a higher max, you need more regen to reach the max in the same amount of time.

@wapcaplet
Copy link
Contributor Author

wapcaplet commented Dec 21, 2021

After further testing with various builds (pre-cardio master, post-cardio master, and variations of this branch), I am convinced that the cardio and stamina/regen calculations are definitely broken, or at least wildly skewed from their original behavior with regard to size-altering and cardio-altering mutations.

For instance, HUGE characters can somehow run 23 steps further than normal characters in a post-cardio world, and SMALL characters run out of breath 6 steps earlier than normal characters. This should not be happening.

image

I am reconsidering the cardio_multiplier field now, and trying a more direct transformation using the original max_stamina_modifier. I suspect we may want to keep both fields.

@Terrorforge
Copy link
Contributor

Terrorforge commented Dec 21, 2021

I don't think they're "broken" so much as this is the inevitably result of tying CARDIO heavily to base metabolic rate. A Huge character has a BMR about twice that of an average-sized character, which results in both a significant immediate increase in stamina, and a long-time increase from CARDIO_ACCUMULATOR due to increased calorie consumption. The reverse would be true of mutations like Tiny.

e: and of course a significantly increased maximum cardio (and therefore stamina) as well

For the same reason, Fast Metabolism is now a colossal buff to stamina - and mutations like Heat Dependant and Light Eater are significant nerfs.

If you're curious about the effect sizes at play: I've been playing max-height, Fast Metabolism characters that mutate into Ursine or Cattle lately. That puts my starting BMR at about 3500 and my final BMR somewhere between 6500 and 7000. Such a character starts with about 12k stamina and immediately jumps to about 24k when they mutate Huge. By my calculations, they would eventually hit 50k+ stamina solely as a result of their natural base metabolism, although it would take something like an entire season to get there.

And aside from the degree of difference perhaps being a bit extreme, I think that mostly makes sense? The tradeoff cold-blooded creatures make is that they're less active, but require fewer resources. The "Tiny" thing seems weird on the surface, but the relevant lines also have mutations like Rapid Metabolism and Hyperactive that, once they're fully functional again, should more than compensate. And idk about the biology if this one, but at least aesthetically it makes sense that enormous bear person could just keep going and going forever.

@Photoloss
Copy link
Contributor

Not really, although most of it is just the square cube law which the game happily ignores otherwise. I am not sure what actually determines the short-term endurance we call "stamina" across the entirety of the animal kingdom but there are many small factors at play. Oxygenation, sugar(/ATP?) supply and heat dissipation definitely are important but many animals are built more around their niche lifestyle (grazers, ambush predators, foragers) than what their broader lineage is physically capable of.

Another part of the issue is that we don't really distinguish how exertion scales with character size. The act of dragging a crate of rocks on the ground represents a fixed exertion of force and energy, which means it becomes proportionately easier for larger characters. A similar point could be made for swinging a fixed size sledgehammer although for something lighter like a mace you also need to consider the acceleration of the wielding arm. And running or swimming means you should feel the full impact of square cube scaling because all the energy is deposited within your own body.

And then you add body temperature to the mix where larger bodies benefit from both proportionately lower surface area and higher thermal mass (take longer to cool down) and how much temperature management dictates short-term metabolism in different animals.

TL;DR: it's complicated, we need a Ph. D in biology for this.

@wapcaplet wapcaplet force-pushed the w-cardio-workout branch 2 times, most recently from 01d622c to 43fbac7 Compare December 26, 2021 03:42
@github-actions github-actions bot added astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels Dec 26, 2021
@wapcaplet wapcaplet force-pushed the w-cardio-workout branch 2 times, most recently from e1f193f to f16d274 Compare December 26, 2021 05:06
@github-actions github-actions bot added BasicBuildPassed This PR builds correctly, label assigned by github actions and removed BasicBuildPassed This PR builds correctly, label assigned by github actions labels Dec 26, 2021
@wapcaplet
Copy link
Contributor Author

wapcaplet commented Dec 26, 2021

After spending the vast majority of my time on this PR trying to get the test case in a fairly robust condition, and between that and tedious manual testing (running back and forth and counting steps like a bloody human FitBit), I am happy with how this balanced out in the end, at least for the three mutations I was most concerned about (Languorous, Indefatigable, and Hyperactive). See test results in the PR description.

The test case's running_turns function gives a reasonable approximation, but is still pretty far off from the actual in-game running steps I measured. For example, with a baseline normal character, in-game I can run 98 steps (barefoot on pavement) before becoming winded, but the running_turns function gives them only 87. (they are much closer now). Still, I've found it useful for comparison and balancing.

I noted several FIXMEs in the test case where some mutations got a significant buff, possibly too much. For example, despite having lower stamina, all the COLDBLOOD traits can run at least 20 steps further than they could before Cardio, and an "Extreme Metabolism" gives more than 30 extra steps. These tests should make them easier to rebalance, but I'm not doing that part in this PR.

@wapcaplet wapcaplet marked this pull request as ready for review December 27, 2021 03:07
tests/cardio_test.cpp Outdated Show resolved Hide resolved
@wapcaplet wapcaplet marked this pull request as draft December 28, 2021 16:30
New cardio_multiplier field is applied as a scaling factor to total
cardio fitness, similar to the max_stamina_modifier field it replaces.

Total balance in terms of running distance is now about the same as it
was before cardio.

- Implement cardio_multiplier in get_cardiofit
- Refactor and document update_stamina
- Convert CARDIO mutations to cardio_multiplier
- CRIT mod: Convert Persistent Body to cardio_multiplier
- Update mutation descriptions to reflect cardio effects.
- Replace Debug Stamina mutation with Debug Cardio mutation
- Document stamina_regen_modifier and cardio_multiplier in JSON_INFO
- Remove max_stamina_modifier entirely
This test covers several aspects of cardio fitness, stamina, and running
distance, especially related to mutations affecting cardio and stamina,
and in particular the three CARDIO traits affected by CleverRaven#52980.

Some FIXMEs are noted, where trait buffs have changed significantly
since the introduction of cardio, and may need to be rebalanced.

Several stub test cases are included to invite future collaboration.
@wapcaplet wapcaplet marked this pull request as ready for review December 31, 2021 21:11
@kevingranade kevingranade merged commit 0d20a08 into CleverRaven:master Jan 1, 2022
@BrettDong
Copy link
Member

The numbers seem to be different on current master branch: https://github.com/CleverRaven/Cataclysm-DDA/runs/4677585598?check_suite_focus=true

@BrettDong
Copy link
Member

BrettDong commented Jan 1, 2022

I tried the new cardio test locally and it passes most of the time, but sometimes (2 out of 30 runs) the actual running_step() value fluctuates from what is expected and fails the test.

An example:
image

@wapcaplet
Copy link
Contributor Author

I tried the new cardio test locally and it passes most of the time, but sometimes (2 out of 30 runs) the actual running_step() value fluctuates from what is expected and fails the test.

Ah, darn, I was afraid of this. Each step drains stamina by a randomized amount, so there's a chance of some discrepancy, but that's an awfully big difference - more than could reasonably be handled by a larger margin.

In most of my testing, I got away with margin( 1 ) and changed it to margin( 3 ) to be generous just before merge, but I guess there's something else I missed.

Thanks for pointing this out, I will investigate.

@wapcaplet wapcaplet deleted the w-cardio-workout branch January 1, 2022 15:53
@wapcaplet
Copy link
Contributor Author

wapcaplet commented Jan 1, 2022

The numbers seem to be different on current master branch: https://github.com/CleverRaven/Cataclysm-DDA/runs/4677585598?check_suite_focus=true

Confirmed running locally with the same --rng-seed 1641011549. These are only off by 5-7, so a larger margin could tolerate them better:

Given: trait: HUGE
../tests/cardio_test.cpp:132

../tests/cardio_test.cpp:136: FAILED:
  CHECK( running_steps( they ) == Approx( expect_run_tiles ).margin( 3 ) )
with expansion:
  99 == Approx( 106.0 )

Given: trait: GOODCARDIO
../tests/cardio_test.cpp:132

../tests/cardio_test.cpp:136: FAILED:
  CHECK( running_steps( they ) == Approx( expect_run_tiles ).margin( 3 ) )
with expansion:
  98 == Approx( 103.0 )

Edit: It failed on one of my own PRs with random seed 1641057818 - these are off by more like 6-8:
https://github.com/CleverRaven/Cataclysm-DDA/runs/4680207404?check_suite_focus=true

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions [C++] Changes (can be) made in C++. Previously named `Code` Code: Tests Measurement, self-control, statistics, balancing. [JSON] Changes (can be) made in JSON json-styled JSON lint passed, label assigned by github actions Mechanics: Character / Player Character / Player mechanics Mutations / Traits / Professions/ Hobbies Mutations / Traits / Professions/ Hobbies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Indefatigable and Hyperactive do not raise maximum stamina
5 participants