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.

Sign up for GitHub

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

Jump to bottom

BUGFIX: Remove Duplicate Application of Player Multipliers During Bladeburner Training / Field Analysis #1606

Closed
wants to merge 5 commits into from
Closed

BUGFIX: Remove Duplicate Application of Player Multipliers During Bladeburner Training / Field Analysis #1606

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
954874c
Remove effective duplicate application of player multipliers when Tra…
cmfrydos Aug 19, 2024
bbf4bab
Bladeburner Training: Explicitly indicate EXP gain prediction in logging
cmfrydos Aug 21, 2024
72c3d17
Bladeburner Field Analysis: Remove effective duplicate application of…
cmfrydos Aug 21, 2024
893dd8a
Bladeburner: Relocate EXP Gain Prediction Closer to Logging (#1607)
cmfrydos Aug 21, 2024
bc1449a
Bladeburner Max Stamina: Restore Balance by Changing CyberEdge Effect…
cmfrydos Aug 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 12 additions & 10 deletions src/Bladeburner/Bladeburner.ts
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

What can you add to make it explicit that the multipliers will be applied by usage at L1288 where retValue is passed to Player.gainStats? It seems odd to manually retrace the steps on the multipliers for the logging alone. That log is predicting what a line of code following this function ought to do and it's way out of context here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The predictive nature of the logging is not introduced by this pull request. The code explicitly logs to the player with the message "Training completed. Gained: " even when no EXP was actually gained yet.

One possible solution would be to move all logging after line 1288, where effective gains can be determined by calculating the difference between the old stats and the new stats. However, I disagree with this approach. The completeAction and processAction functions are highly cohesive and likely only separated to improve code readability and indentation.

Therefore, I propose making it more explicit that for player logging, the stat gains are predicted rather than actual. A proper solution to address your concerns would require a full refactor of these functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renaming Person.gainStats(retValue: WorkStats) to Person.gainBaseExp(baseExp: WorkStats) might help reduce some confusion, but that's beyond the scope of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree on most points except cohesion. gainStats looks like it should be cohesive with BladeBurner actions and it's way off in a different context.

Original file line number Diff line number Diff line change
Expand Up @@ -1074,11 +1074,11 @@ export class Bladeburner {
switch (action.name) {
case BladeburnerGeneralActionName.Training: {
this.stamina -= 0.5 * BladeburnerConstants.BaseStaminaLoss;
const strExpGain = 30 * person.mults.strength_exp,
defExpGain = 30 * person.mults.defense_exp,
dexExpGain = 30 * person.mults.dexterity_exp,
agiExpGain = 30 * person.mults.agility_exp,
staminaGain = 0.04 * this.getSkillMult(BladeburnerMultName.Stamina);
const strExpGain = 30,
defExpGain = 30,
dexExpGain = 30,
agiExpGain = 30,
staminaGain = 0.04;
retValue.strExp = strExpGain;
retValue.defExp = defExpGain;
retValue.dexExp = dexExpGain;
Expand All @@ -1088,15 +1088,17 @@ export class Bladeburner {
this.log(
`${person.whoAmI()}: ` +
"Training completed. Gained: " +
formatExp(strExpGain) +
formatExp(strExpGain * person.mults.strength_exp) +
" str exp, " +
formatExp(defExpGain) +
formatExp(defExpGain * person.mults.defense_exp) +
" def exp, " +
formatExp(dexExpGain) +
formatExp(dexExpGain * person.mults.dexterity_exp) +
" dex exp, " +
formatExp(agiExpGain) +
formatExp(agiExpGain * person.mults.agility_exp) +
" agi exp, " +
formatBigNumber(staminaGain) +
formatBigNumber(
staminaGain * this.getSkillMult(BladeburnerMultName.Stamina) * person.mults.bladeburner_max_stamina,
) +
" max stamina.",
);
}
Expand Down