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

Show file tree
Hide file tree
Changes from all commits
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
57 changes: 31 additions & 26 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 @@ -38,7 +38,7 @@ import { formatTime } from "../utils/helpers/formatTime";
import { joinFaction } from "../Faction/FactionHelpers";
import { isSleeveInfiltrateWork } from "../PersonObjects/Sleeve/Work/SleeveInfiltrateWork";
import { isSleeveSupportWork } from "../PersonObjects/Sleeve/Work/SleeveSupportWork";
import { WorkStats, newWorkStats } from "../Work/WorkStats";
import { WorkStats, newWorkStats, multWorkStats } from "../Work/WorkStats";
import { getEnumHelper } from "../utils/EnumHelper";
import { PartialRecord, createEnumKeyedRecord, getRecordEntries } from "../Types/Record";
import { createContracts, loadContractsData } from "./data/Contracts";
Expand Down Expand Up @@ -1074,30 +1074,31 @@ 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;
retValue.agiExp = agiExpGain;
this.staminaBonus += staminaGain;

if (this.logging.general) {
// retValue contains the base EXP gains.
// Multiply by person EXP multipliers to predict the effective gain.
const effectiveGainPrediction = multWorkStats(retValue, person.mults);
// Predict effective stamina gain after applying Skill and Augmentation multipliers.
let effectiveStaminaGainPrediction = staminaGain * this.getSkillMult(BladeburnerMultName.Stamina);
effectiveStaminaGainPrediction *= person.mults.bladeburner_max_stamina;
this.log(
`${person.whoAmI()}: ` +
"Training completed. Gained: " +
formatExp(strExpGain) +
" str exp, " +
formatExp(defExpGain) +
" def exp, " +
formatExp(dexExpGain) +
" dex exp, " +
formatExp(agiExpGain) +
" agi exp, " +
formatBigNumber(staminaGain) +
" max stamina.",
`${person.whoAmI()}: Training completed. Gained: ` +
`${formatExp(effectiveGainPrediction.strExp)} str exp, ` +
`${formatExp(effectiveGainPrediction.defExp)} def exp, ` +
`${formatExp(effectiveGainPrediction.dexExp)} dex exp, ` +
`${formatExp(effectiveGainPrediction.agiExp)} agi exp, ` +
`${formatBigNumber(effectiveStaminaGainPrediction)} max stamina.`,
);
}
break;
Expand All @@ -1112,22 +1113,26 @@ export class Bladeburner {
if (isNaN(eff) || eff < 0) {
throw new Error("Field Analysis Effectiveness calculated to be NaN or negative");
}
const hackingExpGain = 20 * person.mults.hacking_exp;
const charismaExpGain = 20 * person.mults.charisma_exp;
const rankGain = 0.1 * currentNodeMults.BladeburnerRank;
this.getCurrentCity().improvePopulationEstimateByPercentage(
eff * this.getSkillMult(BladeburnerMultName.SuccessChanceEstimate),
);
const hackingExpGain = 20;
const charismaExpGain = 20;
retValue.hackExp = hackingExpGain;
retValue.chaExp = charismaExpGain;
retValue.intExp = BladeburnerConstants.BaseIntGain;
const rankGain = 0.1 * currentNodeMults.BladeburnerRank;
this.changeRank(person, rankGain);
this.getCurrentCity().improvePopulationEstimateByPercentage(
eff * this.getSkillMult(BladeburnerMultName.SuccessChanceEstimate),
);

if (this.logging.general) {
// retValue contains the base EXP gains.
// Multiply by person EXP multipliers to predict the effective gain.
const effectiveGainPrediction = multWorkStats(retValue, person.mults);
this.log(
`${person.whoAmI()}: ` +
`Field analysis completed. Gained ${formatBigNumber(rankGain)} rank, ` +
`${formatExp(hackingExpGain)} hacking exp, and ` +
`${formatExp(charismaExpGain)} charisma exp.`,
`${formatExp(effectiveGainPrediction.hackExp)} hacking exp, and ` +
`${formatExp(effectiveGainPrediction.chaExp)} charisma exp.`,
);
}
break;
Expand Down
4 changes: 2 additions & 2 deletions src/Bladeburner/data/Skills.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,10 @@ export const Skills: Record<BladeburnerSkillName, Skill> = {
}),
[BladeburnerSkillName.CybersEdge]: new Skill({
name: BladeburnerSkillName.CybersEdge,
desc: "Each level of this skill increases your max stamina by 2%",
desc: "Each level of this skill increases your max stamina by 5%",
baseCost: 1,
costInc: 3,
mults: { [BladeburnerMultName.Stamina]: 2 },
mults: { [BladeburnerMultName.Stamina]: 5 },
}),
[BladeburnerSkillName.HandsOfMidas]: new Skill({
name: BladeburnerSkillName.HandsOfMidas,
Expand Down