BUGFIX: Remove Duplicate Application of Player Multipliers During Bladeburner Training / Field Analysis #1606
Conversation
…ining in Bladeburner. Fix stat gain discrepancy in terminal.
cmfrydos
changed the title
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Renaming Person.gainStats(retValue: WorkStats) to Person.gainBaseExp(baseExp: WorkStats) might help reduce some confusion, but that's beyond the scope of this PR.
There was a problem hiding this comment.
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.
cmfrydos
changed the title
… to Linear 5% from Previous Quadratic 2% (bitburner-official#1607)
|
I took a long look at this, because I'm not super familiar with the Bladeburner code. When I finished, this was my conclusion: I think this PR is going in the opposite direction of how we should address this. Specifically, The same thing applies with stamina_bonus: I feel like we should be adding the multiplier directly to the bonus, as opposed to multiplying it in later. This makes logging and everything much more clear. |
|
I'm happy with the proposed major change to revert completeAction to its original layout, prior to the introduction of Sleeves. However, this raises the issue of how Sleeves should receive their rewards, as this mechanic differs from how the player gains XP. To address this, I suggest making One option to solve this, is to move the multipliers back to (Note: I’d argue that having Sleeves log their XP gains to the Bladeburner console is not well thought out, since they effectively apply stats to all Sleeves and the Player.) Concerning Stamina: Adding the multiplier directly in completeAction would alter the CyberEdge effect. Instead of increasing max stamina, it would increase the max stamina gain rate from training, which would impact gameplay slightly. Introducing So, yeah, it's complicated. If desired, I’d be happy to create a new pull request / branch to demonstrate this solution to this issue without altering the current balance. |
|
Hmm, I see now, I hadn't realized the sleeves issue. That does make things more complicated, I'll have to think on it. |
|
To some degree this has been addresses for work in src/Work/WorkStats and Sleeves/Work. It also goes the extra mile to handle shock correctly. |
|
Venusaur just commented on discord:
Which currently there is Not. Formulas.exe doesn't support BB. This leads me to suggest an even more drastic "change," while keeping the current gameplay balance intact: Why not completely refactor completeAction / Bladeburner.ts into a more consistent approach? We could encapsulate all data from General, Contract, Operation, and Black Operation into "data files" with consistent typing. We could also write neat functions that access specific information, like getEXPFromAction, getRankGain, getHPChange, getStaminaChange, getMaxStaminaChange, getBounty, getEstimationChange, etc., bringing all current functionality together under one roof. This is in contrast to the quick "appendix" of some features, like when Sleeves were added. I currently have some free time and would love to work on a refactor. However, I must be honest: without extensive testing, there's a possibility that this could introduce subtle gameplay changes due to certain quirks or undiscovered aspects of the existing code. |
|
Go for it, I'll help you refactor/write tests for it. |
|
Sounds good to me, a formulas namespace was just added for blade but it's very small right now. |
|
Oh, also I strongly encourage you to do it one function/action at a time, instead of one giant PR that changes all of bladeburner. |
|
Ok, I'm mostly done with the refactor of bladeburner.completeAction, and I am quite happy about the new semantics. All effects of an Action are now split up, which makes it rather easy to expand Formula.exe to BladeBurner. Also there is no more special treatment in the code for whichever type an action has (General, Contracts, Operation, BlackOps), and all current Bugs are more clearly written out to understand them more easily. I'm not that happy about the syntax of the solution though, since it wants to be an enity-component system (with actions being the entities, the effects like HPChange, StatChange, ..., the components), without being a real one. I'm not so sure though whether creating such a system (with a separate small classes for each and every component), is the way to go here. I'll just append the new function here for a moment, maybe you can give your opinion about it. @Alpheus @d0sboots Offtopic: Speaking of probability, when thinking about an upcoming Formula.exe expansion, I noticed that many Bladeburner actions are luck based. Not just their success chance, which would be easy to pass to the player, but also when things happen, they often happen following a random distrubution. i.e when a Bounty Contract fails, the player takes some random amount of damage. Here is the current code for the new Bladeburner.CompleteAction: completeAction(person: Person, actionIdent: ActionIdentifier) {
const loggingConstants = {
General: this.logging.general,
"Black Operations": this.logging.blackops,
Operations: this.logging.ops,
Contracts: this.logging.contracts,
};
const action = this.getActionObject(actionIdent);
try {
const success = action.attempt(this, person);
const effect = success ? action.combinedSuccessEffect : action.combinedFailureEffect;
// Money gain
const earnings = effect.earnings(action) * this.getSkillMult(BladeburnerMultName.Money);
Player.gainMoney(earnings, person instanceof PlayerObject ? "bladeburner" : "sleeves");
// Stamina change
const previousStamina = this.stamina;
this.stamina = clampNumber(
0,
this.stamina + effect.staminaChange(this.maxStamina, action, person),
this.maxStamina,
);
// Stats change
let statChange = effect.statChange(action, this.getSkillMult(BladeburnerMultName.ExpGain));
// this if shouldn't exist, since it is a bug #1607 (duplicate application of exp mults)
if (effect.statChangeExtraMultsMultiply) {
statChange = multWorkStats(statChange, multToWorkStats(person.mults));
}
if (effect.statChangeTimeMultiply) {
var actionTotalSeconds = action.getActionTotalSeconds(this, person);
// This line shouldn't exist, since it reintroduced bug #295 again
actionTotalSeconds *= 1000;
statChange = scaleWorkStats(statChange, actionTotalSeconds);
}
const personStatChange = (stat: WorkStats) =>
person instanceof PlayerObject
? multWorkStats(stat, multToWorkStats(Player.mults)) // player apply exp mults
: scaleWorkStats(stat, (person as Sleeve).shockBonus()); // sleeve apply shock mult
const gainStats = (stat: WorkStats) =>
person instanceof PlayerObject ? person.gainStats(stat) : applySleeveGains(person as Sleeve, stat); // note: this actually applies some stat gains to ALL Sleeves + the Player
const oldStatChange = statChange;
statChange = personStatChange(statChange);
gainStats(statChange);
// this line shouldn't exist, since it is a bug #1607 (Logging part)
statChange = oldStatChange;
// Max Stamina change
var maxStaminaChange = effect.maxStaminaChange;
//this line shouldn't exist #1607 (Multiple application of stamina mults), look into getEffectiveMaxStaminaTrainingBonus
maxStaminaChange *= this.getSkillMult(BladeburnerMultName.Stamina);
this.staminaBonus += maxStaminaChange;
let maxStaminaChangeLog = this.getEffectiveMaxStaminaTrainingBonus(maxStaminaChange);
maxStaminaChangeLog = maxStaminaChange; // this line shouldn't exist, since it is a bug #1607 (Logging part)
// Rank change, also changes Skill Points and Faction Reputation
let rankChange = effect.rankGain(action);
if (rankChange > 0) {
rankChange *= currentNodeMults.BladeburnerRank;
}
this.changeRank(person, rankChange);
// Team size change
const teamChange = effect.teamMemberGain(action, this.sleeveSize, this.teamSize);
this.teamSize += teamChange;
if (teamChange < 0) {
this.teamLost -= teamChange;
}
// Chaos change
let chaosPercentChange = 0; // used for logging
const cities = effect.applyChaosToAllCities ? Object.values(this.cities) : [this.getCurrentCity()];
for (const city of cities) {
chaosPercentChange = effect.chaosPercentageChange(person);
city.changeChaosByPercentage(chaosPercentChange);
city.changeChaosByCount(effect.chaosAbsoluteChange(city.chaos));
}
// Health change
const previousHealth = person.hp.current;
const healthChange = effect.hpChange(action);
if (healthChange < 0) {
const cost = calculateHospitalizationCost(-healthChange);
if (person.takeDamage(-healthChange)) {
++this.numHosp;
this.moneyLost += cost;
}
} else {
person.regenerateHp(healthChange);
}
// Contract and Operation count change
for (const contract of Object.values(this.contracts)) {
contract.count += effect.contractCountChange(contract);
}
for (const operation of Object.values(this.operations)) {
operation.count += effect.operationCountChange(operation);
}
// Population absolute change
const absPopChange = effect.popChangeCount();
const relPopChange = effect.popChangePercentage();
const city = this.getCurrentCity();
city.changePopulationByCount(absPopChange, { estChange: absPopChange, estOffset: 0 });
city.changePopulationByPercentage(relPopChange.percent, {
changeEstEqually: relPopChange.changeEqually,
nonZero: relPopChange.nonZero,
});
// Population Estimate change
const abs = effect.popEstChangeCount() * this.getSkillMult(BladeburnerMultName.SuccessChanceEstimate);
const pct = effect.popEstChangePercentage(person) * this.getSkillMult(BladeburnerMultName.SuccessChanceEstimate);
if (isNaN(pct) || pct < 0) {
throw new Error("Population change calculated to be NaN or negative");
}
this.getCurrentCity().improvePopulationEstimateByCount(abs);
this.getCurrentCity().improvePopulationEstimateByPercentage(pct);
// Any other effects, like leveling up contracts/operations, counting BlackOps, or trigger migration
effect.furtherEffect(this, action, this.getCurrentCity());
// Logging
const logging = loggingConstants[action.type];
if (logging) {
const log = effect.log({
name: person.whoAmI(),
statChange: statChange,
staminaChange: this.stamina - previousStamina,
rankChange: rankChange,
chaosPercentChange: chaosPercentChange,
previousHealth: previousHealth,
stamina: this.stamina,
hpChange: healthChange,
maxStaminaChange: maxStaminaChangeLog,
action: action,
teamChange: teamChange,
earnings: earnings,
person: person,
});
if (log != "") {
this.log(log);
}
}
//this.resetAction(); // this was previously only needed for blackops
} catch (e: unknown) {
exceptionAlert(e);
}
}and this is how for example Training would look like: [BladeburnerGeneralActionName.Training]: new GeneralAction({
name: BladeburnerGeneralActionName.Training,
getActionTime: () => 30,
generalEffect: new ActionEffect({
staminaChange: () => 0.5 * BladeburnerConstants.BaseStaminaLoss,
statChange: () => newWorkStats({ agiExp: 30, defExp: 30, dexExp: 30, strExp: 30 }),
statChangeExtraMultsMultiply: true, // bug: should be false
maxStaminaChange: 0.04,
log: (params: ActionEffectLogParams) =>
`${params.name}: Training completed. Gained: ${formatExp(params.statChange.strExp)} str exp, ${formatExp(
params.statChange.defExp,
)} def exp, ${formatExp(params.statChange.dexExp)} dex exp, ${formatExp(
params.statChange.agiExp,
)} agi exp, ${formatBigNumber(params.maxStaminaChange)} max stamina.`,
}),
desc:
"Improve your abilities at the Bladeburner unit's specialized training center. Doing this gives experience for " +
"all combat stats and also increases your max stamina.",
}), |
|
[EDIT: Alpheus deleted their original response, which may make this harder to follow. The gist of their message was that the comments in the code I posted for bladeburne.completeAction suggest separate functionalities, which should be extracted into individual functions.] Thank you for the quick reply! It looks like you encountered the same issue I did - that the function does many (mostly) unrelated things in sequence. This is what I meant when I suggested that it could be refactored into smaller, more focused components. I like your idea of first creating separate functions within the bladeburner class. These can later be moved into separate classes if needed. I need a bit more time to consider your idea of exposing the log data. My initial thought for testing was to create before/after snapshots of the CompleteAction for all actions (like Training), "intersect" the calls to bladeburner.log, and include those in the snapshot. We could also "intersect" the calls to the random() functions to make them deterministic (or set the seed). Once we have the data from the current implementation, we can use it to test the refactor.
This is actually trickier than it sounds. The "blocks" that are neatly organized in the refactor are quite entangled in the original code. At the moment, I don't see a way to change, review, and merge them separately. I'll push my full refactor to my bb fork, but I won't open a PR yet because there are definitely some small discrepancies between the original and the refactor. So far, I've mostly noticed some different number formatting in the logs. I want to write a test suite to catch these issues before requesting a pull. :) |
|
Deleted old comment—I checked your code changes and got a bit of a better grasp. In short, I'm not a huge fan of where you took the design. The extra level of abstraction for the data-driven tasks is giving me more of a headache than the previous code. I'm only really invested into this change if we follow d0sboots' original direction and maintain functional composition without the data layer. What you designed is a rule engine and the tasks themselves are not rules, they are explicit behavior that differ quite a bit between each other. A better fit for your design would be 12 tasks modifying only 1 stat but changing how and when they do so. But 12 tasks modifying 12 different states and requiring further stat effects is a recipe for disaster in my experience. I wouldn't want to maintain this. I'd much rather a see a safe extraction of Training and see where it fits best, as it has consequences to sleeves (which was the original problem). |
|
I see your concerns, and need to think about it at some other time, I'm having an appointment soon, and will be back in a few hours. I hope to believe that they can be eliminated by my suggestion of moving the separate effects into their own components.
Can you explain into more detail, what you mean with data-driven task? For now, I'll push all changes to my repo, maybe this helps to eliminate some of your concerns. Here: dev...cmfrydos:bitburner-src:BladeCompleteActionRefactor |
The state-based constructor with the Action classes and Effects. Can we restart and start with a smaller PR, say cca. 50 lines to keep on track with your original? |
|
Hi @Alpheus, Thank you for your feedback. I appreciate the time you've taken to review my proposal. However, I feel that there's a bit of a disconnect in our communication, and I want to ensure we're on the same page moving forward. I proposed writing a test suite next, but it seems you disagreed. Could you clarify why this isn't the right direction in your view? Practical suggestions on how to proceed would be very helpful. Additionally, I suggested transitioning the system to an entity-component system, given that it's already functioning in a similar manner. I believe this approach would address the concerns you raised. If you think this would not resolve the issues, could you explain why? If there's any misunderstanding on my part, I'm open to hearing your thoughts. To illustrate my point, here’s how I envisioned the implementation: // Bladeburner.ts
function completeAction(action, person) {
for (const component of action.effects) {
// apply's parameters will be more explicit and can include setters/getters for stats like stamina, etc.
component.apply(action, person, this);
}
}For example, here’s how you could construct the Training action using component class constructors: // GeneralActions.ts
const trainingActionEffect = new ActionEffect([
new StaminaChangeComponent(() => 0.5 * BladeburnerConstants.BaseStaminaLoss),
new StatChangeComponent(() => newWorkStats({
agiExp: 30,
defExp: 30,
dexExp: 30,
strExp: 30
}), statChangeExtraMultsMultiply = true),
new MaxStaminaChangeComponent(0.04),
new LogComponent((params) =>
`${params.name}: Training completed. Gained: ${formatExp(params.statChange.strExp)} str exp,
${formatExp(params.statChange.defExp)} def exp,
${formatExp(params.statChange.dexExp)} dex exp,
${formatExp(params.statChange.agiExp)} agi exp,
${formatBigNumber(params.maxStaminaChange)} max stamina.`)
]);Here’s how the StaminaChangeComponent might be implemented, extending a base BladeburnerComponent: // BladeburnerComponent.ts
class BladeburnerComponent {
apply(action, person, context) {
// This method would be overridden by each specific component
throw new Error("apply method must be implemented in derived classes");
}
getEffect(...args) {
// This method would be overridden by each specific component
throw new Error("getEffect method must be implemented in derived classes");
}
}
// StaminaChangeComponent.ts
class StaminaChangeComponent extends BladeburnerComponent {
constructor(staminaChangeFunction) {
super();
this.staminaChangeFunction = staminaChangeFunction;
}
apply(action, person, blade) {
// Get the effective change as a tuple
const [oldStamina, staminaChange, newStamina] = this.getEffect(action, person, blade);
// Apply the new stamina value
blade.stamina = newStamina;
// Pass data to log component if it exists
// This one is the least figured out yet. I could also imagine that completeAction collects the changes
// and then passes them to the LogComponent if it exists.
const logComponent = action.getEffect('LogComponent');
if (logComponent) {
logComponent.changes.stamina = staminaChange;
}
}
getEffect(action, person, blade) {
// Calculate the stamina change
const oldStamina = blade.stamina
const staminaChange = this.staminaChangeFunction(blade.maxStamina);
// Ensure the new stamina value is within valid bounds
const newStamina = clampNumber(0, oldStamina + staminaChange, blade.maxStamina);
// Return the old value, the change, and the new value as a tuple
return [oldStamina, staminaChange, newStamina];
}
}I truly believe that this approach could solve most of the problems we’ve identified with Bladeburners, while also aligning with the criteria we previously agreed upon. However, I’m open to reconsidering this if you can provide more detailed reasons why you think this approach might fail, either in functionality or maintainability. If you have an alternative vision for the code’s architecture that you believe better addresses the issues at hand, I would greatly appreciate it if you could outline it more precisely. Thank you again for your time and for any further insights you can provide. I’m committed to finding the best solution, and I hope we can align on the best path forward. Looking forward to your response. |
|
I'm not opposed to any of your suggestions, in principle, as long as you can keep this PR to sub-50 lines of changes prior to new tests added. Your passion is on point, it's very appreciated. But I stand with everyone else's sentiment: take it in small steps. This is touching a lot of older parts of the codebase and you'll likely make mistakes along the way. Look at the major refactors going on in open PRs, all blocked for several months due to their scale. Keep it small and you'll get your things merged. If you want to do an all-in-one huge refactor and add tests on top while also fixing bugs, I'm afraid it's not going to pass review any time soon. |
I think there might be some misunderstanding about the new design we're implementing. Let me clarify further: Let me give you some pseudocode. Here is a breakdown of the current behaviour: switch(action):
case action_1:
modify(stat_1), log(stat_1)
modify(stat_2), log(stat_2)
// ...
// sometimes modifying stats again AFTER logging
modify(stat_1)
// ...
modify(stat_m)
break;
case action_2:
// ... similarly modifying some of the stats
case action_nProblems:
Refactor Designaction_attempt = action.attempt() // Yields all effects based on success or failure
for each affected_stat in action_attempt:
stat_change = getEffectOn(affected_stat)
applyEffect(stat_change)
log.append(stat_change)
log()Benefits:
Thank you for your prompt response, especially this late in the evening. Regarding your last reply:
I hope you can see that boiling a 353-line function into 6 lines would yield a super-50-line PR. You have previously expressed your support for a refactor of this function. Do you have any suggestions on how we might break down the 700-line PR into 14 50-line PRs? What about having one PR, but doing one Action (like GeneralAction.Training) at a time / commit? It would look horrible in the code until all Actions are refactored, but it might be easier to review one at a time. I still doubt that it will get merged in steps, but it might help the process of reviewing. |
|
Yes, I'm 100% for the one action at a time refactor. Put in just as much abstraction is needed for everything that has been migrated to-date and keep everything test covered as you move. Don't get me wrong—you can do as you wish. It's just that I won't support you if your PR will be huge by choice. |
|
Ok, that sounds good, thank you again for your support. |
|
You won't need to build PRs on top of PRs.
That's it. That in itself is valuable if the refactoring makes the current architecture easier to understand. We can keep doing this in small steps until the code is very clear in regards to modifiers, logs, double-dipping and their side effects onto sleeves (and any double dipping that may occur there). Once we have transparency and visibility, THEN we can decide if the bug is worth fixing. |
|
Here, let me help. Look at your PR for example at https://github.com/bitburner-official/bitburner-src/pull/1606/files#diff-4d95b76d4df7dedcb50e350d2afb8901d87780532239de973e1e0db6deded91bR1077 Your code changes are well-contained to a continuous segment of code and they are all Training-specific. That is a simple enough function to highlight that these values should be tested pre-multiplier. You can then write a handful of tests using only what you extracted, for example:
Deeper Intution Here is what FTA says about this file (https://ftaproject.dev/playground): Notably Cyclomatic complexity and difficulty. These two are proxy metrics for nesting and control structures. You'll get the best bang-for-bug by taking things out of that double-nested looped switch statement and extract it in a manner that allows you to work at a low level of nesting with 1-2 parameters passed in. As long as you don't deal with this issue, any changes you make will make things worse |
|
I'm not a huge fan of metrics, but I know it's in shambles just by looking at the codebase, especially bladeburner.completeAction. I mean, it took me two full days to entangle it. No judgement, it just grew to that by many different contributors writing in their own style extending existing behaviour. I hope you are aware that our new plan, of extracting Action after Action, will worsen those metrics though, until everything is in the new structure. Again, I'm not a huge fan of metrics, so that's no concern to me. |
As you said, you're not huge on metrics. Your intuition is off and it's understandable given the circumstance. But you're incorrect. Slowly extracting the pieces will improve on complexity and control structures at a small cost to line count. Once everything's in place. You'll spot the underlying designs, such as:
Most actions give BB XP. Some change population. |
|
I already went through this, remember :)? I've noticed that only after extracting a few actions, like all of the Operations, you start to loose branches, thus cyclomatic complexity. I also extracted them in one go before (play-)testing. If you want a running version you need to have a guard that protects you from executing (parts) of the actions twice. Pseudocode: I'm not 100% sure if that's really needed for the first PR, writing this from my mobile, but at the latest when refactoring contracts or operations, since there is some code that gets executed regardless of the type of contract or operation. Also creating the entity-component system creates some initial overhead, that might introduce complexity. IMO, whenever you have multiple systems/algorithms in place, and treat your data either or, that requires some cognitive capacity to first understand both systems and then decide on which applies for which object. What I meant by being not a fan of code metrics is that they just provide numbers that have no intrinsic value. Only when you start to interpret them, like saying programs with less cyclomatic complexity are less "complex" and easier to understand, they start to become useful. But that's often not the truth. Instead we tend to start optimizing for a metric, and this often leads to worse code. Also, what does something like LOC actually mean? There are many different definitions of what a line of code is, like whether it includes empty lines, or ones with a comment. Also, the average "impact" of a line differs alot between programming languages and styles. So a 1000 line Haskell program might be more complex or powerful than a 2000 line Java or I'm currently writing some more tests and hadn't had the time I hoped for today, but will finish tomorrow. Maybe this will then show why I doubt there will be a significant loss of complexity by extracting only a single action. |
|
This is all hyperbole. Yes, optimising for metrics can have bad impacts. But having no metrics at all is just as careless. In this case what we want to look for is trends. You have the baseline from my screenshot. Do a couple refactors. There are a lot of benign examples of getting rid of branches. Some of which are the logging guards which are always cohesive to their action type along with their actual content—again, fully cohesive with the specific action. As for components, we already have action objects to capture each type. If you want to play with it I suggest subclassing it further to capture each specific action. That's what inheritance is good for: subtype polymorphism. Subtype polymorphism is the solution to the code smell of having identity checks with if/else meshes or switches. That's exactly our use case. |
|
Here's another analysis of this file, in case you want to use some more metrics (this is from the dev version): Creating opportunities to capture duplications in population change, hp loss and team size. |
|
I am really sorry, but I'd like to end this here. I am not getting anything out of this conversation. It seems like we don't find a common ground, and this communication is spoiled. I've tried to get a discussion going, but I don't believe you're either taking my arguments nor my follow up questions seriously. Furthermore, I don't see a clear line of thought in your messages, since it seems to me that you are abreviating from previous statement's you've made. This is not worth both our time. Were I'd like to leave this is as follows: For anybody who'd like to go from here: I encapsulated all Actions behaviour into a system where their effects are composed together. What's missing, imo, is a refactoring step to break the application of all components into a separate getEffect and applyEffect functionality. |
|
Sad to see it, but probably for the best. |
|
Sorry I wasn't around when this conversation was happening. Here's a collection of semi-related thoughts:
|
|
I still believe that my approach would suit the needs of bladeburner best, until convinced otherwise ;), but it will not be about creating 14 or so new singleton classes. I think composition works in this case way better than polymorphism. So I would not bury this approach completely yet. I first need to get an idea about how to introduce this refactor in small pieces that it can be reviewed easily. Until I figured this out, this will be on pause. The refactor is not hard, it's just a lot. And I'm kind of new to PR's and code reviews, as I previously worked directly on repositories. There is always much to learn. |
|
This is a success in that it has brought the three-four of us together. I apologise to setting a standard without really checking for buyin or providing example. Your passion has inspired me to make things easier as well. I'll be opening up a few smaller PRs. Hopefully you will use them as inspiration on how to keep PRs small and maybe learn something new on refactoring. |
BUGFIX: Remove Duplicate Application of Player Multipliers During Bladeburner Training
Closes #1607
Closes #yyyy
This pull request addresses and fixes the issue of duplicate application of player multipliers during Bladeburner Training (#1607). Previously, the Bladeburner Terminal showed a discrepancy in Stamina and Skill increases compared to the expected values, as detailed in the bug ticket (#yyyy).
Changes Made:
Testing:
The fix was tested by comparing the output in the Bladeburner Terminal with the Stamina and Skill increases. The results now align with the expected values as outlined in the initial bug report.