-
Notifications
You must be signed in to change notification settings - Fork 266
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
BLADEBURNER: Move bladeburner team losses to Casualties #1654
Changes from 8 commits
8336961
549ee37
22c2bf0
0ade0db
4ebfdc3
dffaf95
5a3ba3a
435b6f9
f1e4f5b
d7e988a
926ca15
4e70585
0a3a453
6f5c191
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
export enum CasualtyFactor { | ||
LOW_CASUALTIES = 0.5, // 50% | ||
HIGH_CASUALTIES = 1, // 100% | ||
} | ||
|
||
export interface OperationTeam { | ||
/** teamSize = Human Team + Supporting Sleeves */ | ||
teamSize: number; | ||
teamLost: number; | ||
/** number of supporting sleeves at time of action completion */ | ||
sleeveSize: number; | ||
|
||
killRandomSupportingSleeves(sleeveDeaths: number): void; | ||
} | ||
|
||
export interface TeamActionWithCasualties { | ||
teamCount: number; | ||
|
||
getTeamCasualtiesRoll(low: number, high: number): number; | ||
|
||
getMinimumCasualties(): number; | ||
|
||
resolveTeamCasualties: typeof resolveTeamCasualties; | ||
} | ||
|
||
/** | ||
* Some actions (Operations and Black Operations) use teams for success bonus | ||
* and may result in casualties, reducing the player's hp, killing team members | ||
* and killing sleeves (to shock them, sleeves are immortal) * | ||
*/ | ||
export function resolveTeamCasualties(this: TeamActionWithCasualties, team: OperationTeam, success: boolean) { | ||
const severity = success ? CasualtyFactor.LOW_CASUALTIES : CasualtyFactor.HIGH_CASUALTIES; | ||
const radius = this.teamCount * severity; | ||
const worstCase = severity < 1 ? Math.ceil(radius) : Math.floor(radius); | ||
/** Best case is always no deaths */ | ||
const deaths = this.getTeamCasualtiesRoll(this.getMinimumCasualties(), worstCase); | ||
const humans = this.teamCount - team.sleeveSize; | ||
const humanDeaths = Math.min(humans, deaths); | ||
const damagedSleeves = deaths - humanDeaths; | ||
|
||
/** Supporting Sleeves take damage when they are part of losses, | ||
* e.g. 8 sleeves + 3 team members with 4 losses -> 1 sleeve takes damage */ | ||
team.killRandomSupportingSleeves(damagedSleeves); | ||
|
||
/** Clamped, bugfix for PR#1659 | ||
* "BUGFIX: Wrong team size when all team members die in Bladeburner's action" */ | ||
team.teamSize = Math.max(team.teamSize - humanDeaths, team.sleeveSize); | ||
team.teamLost += deaths; | ||
|
||
return { deaths, team, damagedSleeves }; | ||
d0sboots marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,7 +37,6 @@ import { Settings } from "../Settings/Settings"; | |
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 { getEnumHelper } from "../utils/EnumHelper"; | ||
import { PartialRecord, createEnumKeyedRecord, getRecordEntries } from "../Types/Record"; | ||
|
@@ -50,10 +49,12 @@ import { GeneralActions } from "./data/GeneralActions"; | |
import { PlayerObject } from "../PersonObjects/Player/PlayerObject"; | ||
import { Sleeve } from "../PersonObjects/Sleeve/Sleeve"; | ||
import { autoCompleteTypeShorthand } from "./utils/terminalShorthands"; | ||
import type { OperationTeam } from "./Actions/TeamCasualties"; | ||
import { shuffleArray } from "../Infiltration/ui/BribeGame"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This probably should go into its own helper, but that can also wait for later if you want. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. I'd prefer to not touch non-BB refactors at the moment. Next pass? |
||
|
||
export const BladeburnerPromise: PromisePair<number> = { promise: null, resolve: null }; | ||
|
||
export class Bladeburner { | ||
export class Bladeburner implements OperationTeam { | ||
numHosp = 0; | ||
moneyLost = 0; | ||
rank = 0; | ||
|
@@ -750,32 +751,20 @@ export class Bladeburner { | |
} | ||
} | ||
|
||
public killRandomSupportingSleeves(n: number) { | ||
Alpheus marked this conversation as resolved.
Show resolved
Hide resolved
d0sboots marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const sup = [...Player.sleevesSupportingBladeburner()]; // Explicit shallow copy | ||
shuffleArray(sup); | ||
sup.slice(0, Math.min(sup.length, n)).forEach((sleeve) => sleeve.kill()); | ||
Alpheus marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
completeOperation(success: boolean): void { | ||
if (this.action?.type !== BladeburnerActionType.Operation) { | ||
throw new Error("completeOperation() called even though current action is not an Operation"); | ||
} | ||
const action = this.getActionObject(this.action); | ||
|
||
// Calculate team losses | ||
const teamCount = action.teamCount; | ||
if (teamCount >= 1) { | ||
const maxLosses = success ? Math.ceil(teamCount / 2) : Math.floor(teamCount); | ||
const losses = getRandomIntInclusive(0, maxLosses); | ||
this.teamSize -= losses; | ||
if (this.teamSize < this.sleeveSize) { | ||
const sup = Player.sleeves.filter((x) => isSleeveSupportWork(x.currentWork)); | ||
for (let i = 0; i > this.teamSize - this.sleeveSize; i--) { | ||
const r = Math.floor(Math.random() * sup.length); | ||
sup[r].takeDamage(sup[r].hp.max); | ||
sup.splice(r, 1); | ||
} | ||
// If this happens, all team members died and some sleeves took damage. In this case, teamSize = sleeveSize. | ||
this.teamSize = this.sleeveSize; | ||
} | ||
this.teamLost += losses; | ||
if (this.logging.ops && losses > 0) { | ||
this.log("Lost " + formatNumberNoSuffix(losses, 0) + " team members during this " + action.name); | ||
} | ||
const { deaths } = action.resolveTeamCasualties(this, success); | ||
if (this.logging.ops && deaths > 0) { | ||
this.log("Lost " + formatNumberNoSuffix(deaths, 0) + " team members during this " + action.name); | ||
} | ||
|
||
const city = this.getCurrentCity(); | ||
|
@@ -992,9 +981,7 @@ export class Bladeburner { | |
this.stamina = 0; | ||
} | ||
|
||
// Team loss variables | ||
const teamCount = action.teamCount; | ||
let teamLossMax; | ||
let deaths; | ||
|
||
if (action.attempt(this, person)) { | ||
retValue = this.getActionStats(action, person, true); | ||
|
@@ -1004,7 +991,8 @@ export class Bladeburner { | |
rankGain = addOffset(action.rankGain * currentNodeMults.BladeburnerRank, 10); | ||
this.changeRank(person, rankGain); | ||
} | ||
teamLossMax = Math.ceil(teamCount / 2); | ||
|
||
deaths = action.resolveTeamCasualties(this, true).deaths; | ||
|
||
if (this.logging.blackops) { | ||
this.log( | ||
|
@@ -1028,7 +1016,8 @@ export class Bladeburner { | |
this.moneyLost += cost; | ||
} | ||
} | ||
teamLossMax = Math.floor(teamCount); | ||
|
||
deaths = action.resolveTeamCasualties(this, false).deaths; | ||
|
||
if (this.logging.blackops) { | ||
this.log( | ||
|
@@ -1042,26 +1031,10 @@ export class Bladeburner { | |
|
||
this.resetAction(); // Stop regardless of success or fail | ||
|
||
// Calculate team losses | ||
if (teamCount >= 1) { | ||
const losses = getRandomIntInclusive(1, teamLossMax); | ||
this.teamSize -= losses; | ||
if (this.teamSize < this.sleeveSize) { | ||
const sup = Player.sleeves.filter((x) => isSleeveSupportWork(x.currentWork)); | ||
for (let i = 0; i > this.teamSize - this.sleeveSize; i--) { | ||
const r = Math.floor(Math.random() * sup.length); | ||
sup[r].takeDamage(sup[r].hp.max); | ||
sup.splice(r, 1); | ||
} | ||
// If this happens, all team members died and some sleeves took damage. In this case, teamSize = sleeveSize. | ||
this.teamSize = this.sleeveSize; | ||
} | ||
this.teamLost += losses; | ||
if (this.logging.blackops) { | ||
this.log( | ||
`${person.whoAmI()}: You lost ${formatNumberNoSuffix(losses, 0)} team members during ${action.name}.`, | ||
); | ||
} | ||
if (this.logging.blackops && deaths > 0) { | ||
this.log( | ||
`${person.whoAmI()}: You lost ${formatNumberNoSuffix(deaths, 0)} team members during ${action.name}.`, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I always wondered... is this double-space intentional? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd assume it is, although if it bugs you I wouldn't mind if you changed it. Honestly, I'd be OK with completely unifying the logging between the two versions and ironing out the minor differences in wording, to help factor out more stuff. But the 0 vs 1 discrepancy that Marvin brought up throws a big wrench into things. (OK, I guess it wasn't that big.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By the looks of it we may have a second pass to go over things after this is merged. I'd rather get this one into dev and then do another smaller pass. The PR is getting too stale. |
||
); | ||
} | ||
break; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more interface complexity that exists only for testing.
getTeamCasualtiesRoll
obfuscates the intent a fair amount (it gets plumbed through a class and an interface to ultimately be a simple call togetRandomIntInclusive
. However, this is balanced by the fact that it would be a bit trickier to mock around without it.AFAICT,
resolveTeamCasualties
isn't even used as a test injection point, and is thus completely unneeded. If you got rid of both of these variables, you could also completely remove theTeamActionWithCasualties
interface andresolveTeamCasualties
could be a simple function that takesteamCount
as a number instead of semi-awkwardly throughthis
. It would also be just as straightforward to mock, if you ever needed/wanted to in the future.EDIT: Just saw your change to fix the 0/1 issue. I guess the interface has a stay on execution. XD You could pass that in as another free parameter to the function, but that's getting more ugly... it tips the balance back towards it being worth it to have an interface. I still don't think you need the instance variable for the function here, though. It can just be a free function that takes a
TeamActionWithCasualties
as first parameter. (Normally you would just put it on the base class, but that doesn't work here when you don't have a base class for just this pair of classes.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It won't work. It needs teamCount, sleeveSize and teamSize (not to be confused with teamCount). I tried it and this is the minimal complexity I've been able to scale it down to. It's so easy to confuse teamCount and teamSize and I wanted it to be handled explicitly. A narrower type (the interface) but generously wide instance (Bladeburner, Action) to make it user-friendly on the developer side who's working on CompleteAction.
I tried mocking it. But ended up with a lot more complexity (and automocking on the module level which made the tests much slower).
However, if you insist I could make it a public module export on TeamCasualties and then mock that, but then the tests get exposed to much more "implementation detail" that isn't captured by the interface. It's a hard balance. Anything touching entropy needs to be controlled.
Okay, a bit more context
I need to be able to control the random range to test it properly. If we don't want this interface shenanigans, then I'd rather throw away the tests that are flaky when random. But that means we lose 2/3rds of the tests. The issue isn't Math.random or something similar. The problem is that our numbers cause the rolls to be exhaustive at random. It's exhaustive if casualties are 0% or 100%, and the ranges can randomly range to 50%-100% or 0%-100% depending on what the attempt roll was. This makes it incredibly difficult to test, even with monte-carlo simulations. I either need control over an non-exhaustive range or a forced scope to the roll (MIN,MAX). I opted for the latter, but it requires one injection point in production scope.
Now, I'm aware you may consider injection scope on an interface more invasive than on a package module, but from the perspective of production code, there will be a public function somewhere to roll the range (either exported, or on a class/interface). There is no way avoiding having it be "mentioned" in the final output. If this is a huge blocker, I'd much rather remove the tests, than try to pretend we're not affecting public scope. As I mentioned in the earlier round of reviews, this is one of those problems that cannot be without compromise. That's the nature of random elements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edit: As a compromise if you want I can put the roll on Bladeburner instead (on the OperationTeam interface). That way it requires less care on the Action data side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, as you said it's a compromise but I like this compromise better.
However, I feel like you didn't see the other part of my comment? Specifically, regarding the
resolveTeamCasualties
function itself, that it doesn't need to be part of the interface but can be a free function that takes an interface object as a param. And that should be just as easy to mock (you can mock out the free function in the tests, if needed.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I misunderstood. I thought you meant the resolve as being part of the interface, not the function declaration being referenced and obsolete after the 0/1 fixed gave legit purpose for the namespace. Let me re-read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did comment on it, quoted below your original. You mean to have it be a global function? Doesn't that make the invocations more verbose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, you mean it can stay on the class, but not be part of the interface? Yeah it seems we lost that structural need somewhere during refactoring. I'll try it, see how it looks.
Edit: Yeah, that's completely benign. Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still see you referring to an object with an interface but I'm not quite sure what that's highlighting. Could you give me a quick example or code pin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I got what you meant, let me know!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, that was what I was talking about!