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

Fix performance factor breaking stamina recovery #6204

Closed
wants to merge 2 commits into from

Conversation

BaerMitUmlaut
Copy link
Member

@BaerMitUmlaut BaerMitUmlaut commented Mar 19, 2018

When merged this pull request will:

  • Fix performance factor breaking stamina recovery

From the original VBS documentation:


Acidosis accumulation
anaerobicFatigue += anaerobicDwork * maxPowerFatigueRatio * 1.1;
[...]
maxPowerFatigueRatio is a constant designed to scale the accumulation and recovery of anaerobicFatigue: maxPowerFatigueRatio = 0.057 / peakPower;


They might mean that it's a constant regardless of the actual peakPower, which is calculated from the VO₂max (which in turn is adjusted by the performance factor). This seems to fix issue #6163.

@BaerMitUmlaut BaerMitUmlaut added the kind/bug-fix Release Notes: **FIXED:** label Mar 19, 2018
@jonpas jonpas added this to the 3.13.0 milestone Mar 19, 2018
PabstMirror
PabstMirror previously approved these changes Mar 21, 2018
@ulteq
Copy link
Contributor

ulteq commented Mar 23, 2018

I don't think that this is correct. But if it really fixes a gameplay issue. Go for it.

They might mean that it's a constant regardless of the actual peakPower

But then we would also have to change this line:

- (_ae1PathwayPowerFatigued + _ae2PathwayPowerFatigued - _ae1Power - _ae2Power) * (0.057 / GVAR(peakPower)) * GVAR(anFatigue) ^ 2 * GVAR(recoveryFactor)

@BaerMitUmlaut
Copy link
Member Author

I don't think it's how it's meant either, this is more of a hotfix tbh. If you have an idea where else the problem might be let me know.

@ulteq
Copy link
Contributor

ulteq commented Mar 23, 2018

I think part of the problem is that we derive _perceivedFatigue off anReserve. Which is not how it is meant to be done.

Instead we should use anFatique directly: https://github.com/acemod/ACE3/pull/5723/files#diff-0bdf0f3e6417972d88363532f5354a5aR114

@BaerMitUmlaut
Copy link
Member Author

That's definitely not the underlying issue. The stamina bar displays only the anaerobic reserve which very visibly shows that it does essentially not recover at all. _perceivedFatigue is only used for feedback effects and has no influence on the recovery.

@ulteq
Copy link
Contributor

ulteq commented Mar 23, 2018

That's definitely not the underlying issue.

Right.

What if the performance factor would also modify ANTPERCENT.

GVAR(anFatigue) = GVAR(anFatigue) + _anPower * (0.057 / GVAR(peakPower)) * 1.1;
// 1264.64 = 0.057 / peakPower with default VO2 max
// See #6163
GVAR(anFatigue) = GVAR(anFatigue) + _anPower * 1264.64 * 1.1;
Copy link
Contributor

Choose a reason for hiding this comment

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

1264.64 seems to be very high. Where did you get peakPower 0.00004507211 from?

Copy link
Contributor

Choose a reason for hiding this comment

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

good catch, we just want to replace GVAR(peakPower) with the default value of 1264, not the whole term inside the ()

@PabstMirror PabstMirror modified the milestones: 3.13.0, 3.12.2 Apr 2, 2018
@PabstMirror
Copy link
Contributor

Just to clarify my changes GVAR(peakPower) = 1264 under default values.

But I'm not sure if this is correct, line 58 increases fatigue and line 69 handles recovery.
Before both lines used the term (0.057 / GVAR(peakPower) to scale; this goes down as performance goes up.
This means that you had more reserves, but it also took longer to recover.

With this PR we change fatigue increase to ignore peakPower, but recovery still is negatively effected by it.

@PabstMirror PabstMirror modified the milestones: 3.12.2, 3.13.0 Apr 11, 2018
@ulteq
Copy link
Contributor

ulteq commented Apr 23, 2018

I'm still not convinced by this PR. But I sadly don't have time to take a closer look.

One solution might be to finish this PR: #5723

It should be almost ready to go if we leave the Experimental movement speed limiter out.

@PabstMirror PabstMirror dismissed their stale review April 28, 2018 22:32

I don't think this will work

@jonpas jonpas modified the milestones: 3.13.0, Backlog Dec 7, 2019
@Mike-MF
Copy link
Member

Mike-MF commented Sep 10, 2023

Closing due to age and/or inactivity, unlikely to be finished or has to be reworked due to other changes over the years. May be re-opened if someone finds it useful, even better just open a new up-to-date PR.

@Mike-MF Mike-MF closed this Sep 10, 2023
@jonpas jonpas deleted the fix-perf-factor branch September 11, 2023 00:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug-fix Release Notes: **FIXED:** status/WIP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants