Merge pull request xiv-gear-planner#191 from xiv-gear-planner/fix-buf…

…f-applicability

Fix applicability of buffs on start vs snapshot

packages/core/src/sims/buff_helpers.ts

@@ -0,0 +1,38 @@
+import {Buff} from "./sim_types";
+
+function nonZero(number: number): boolean {
+    return (number ?? 0) > 0;
+}
+
+/**
+ * Whether a buff is relevant at the start of an action.
+ *
+ * e.g. for haste/swiftcast-style buffs, this would be true. It is also true
+ * of the buff has any 'beforeAbility' hook.
+ *
+ * @param buff
+ */
+export function buffRelevantAtStart(buff: Buff): boolean {
+    return nonZero(buff.effects?.haste) || (buff.beforeAbility !== undefined);
+}
+
+/**
+ * Whether a buff is relevant at the point where an action snapshots. This
+ * is true for any buff with damage-increasing effects, or with either a beforeSnapshot
+ * or modifyDamage hook. In addition, it {@link buffRelevantAtStart} returns false,
+ * this will return true to ensure that at least one of the two functions returns true
+ * in either case.
+ *
+ * @param buff
+ */
+export function buffRelevantAtSnapshot(buff: Buff): boolean {
+    return nonZero(buff.effects?.critChanceIncrease)
+        || nonZero(buff.effects?.dhitChanceIncrease)
+        || nonZero(buff.effects?.dmgIncrease)
+        || (buff.effects?.forceCrit)
+        || (buff.effects?.forceDhit)
+        || (buff.beforeSnapshot !== undefined)
+        || (buff.modifyDamage !== undefined)
+        // As a fallback, also force this to return true if the buff is not relevant at start
+        || !buffRelevantAtStart(buff);
+}

packages/core/src/sims/cycle_sim.ts

@@ -31,6 +31,7 @@ import {abilityEquals, appDelay, completeComboData, FinalizedComboData} from "./
 import {abilityToDamageNew, combineBuffEffects} from "./sim_utils";
 import {BuffSettingsExport} from "./common/party_comp_settings";
 import {CycleSettings} from "./cycle_settings";
+import {buffRelevantAtSnapshot, buffRelevantAtStart} from "./buff_helpers";
 
 /**
  * CycleContext is similar to CycleProcessor, but is scoped to within a cycle. It provides methods
@@ -786,6 +787,13 @@ export class CycleProcessor {
         const dmgInfo = this.modifyDamage(abilityToDamageNew(this.stats, ability, combinedEffects), ability, buffs);
         const appDelayFromSnapshot = appDelay(ability);
         const appDelayFromStart = appDelayFromSnapshot + snapshotDelayFromStart;
+        const finalBuffs: Buff[] = Array.from(new Set<Buff>([
+            ...preBuffs,
+            ...buffs]))
+            .filter(buff => {
+                return buffRelevantAtStart(buff) && preBuffs.includes(buff)
+                    || buffRelevantAtSnapshot(buff) && buffs.includes(buff);
+            });
         const usedAbility: UsedAbility = ({
             ability: ability,
             // We want to take the 'haste' value from the pre-snapshot values, but everything else should
@@ -796,7 +804,7 @@ export class CycleProcessor {
                 ...combinedEffects,
                 haste: preCombinedEffects.haste,
             },
-            buffs: Array.from(new Set<Buff>([...preBuffs, ...buffs])),
+            buffs: finalBuffs,
             usedAt: gcdStartsAt,
             directDamage: dmgInfo.directDamage ?? fixedValue(0),
             dot: dmgInfo.dot,

packages/core/src/sims/sim_types.ts

@@ -405,7 +405,10 @@ export type UsedAbility = {
      */
     ability: Ability,
     /**
-     * The buffs that were active either when the ability started casting, or when it snapshotted (the union of both).
+     * The buffs that were active either when the ability started casting, or when it snapshotted, depending on what
+     * bonuses the buff provides.
+     *
+     * TODO: consider adding buffsAtStart and buffsAtSnapshot to clearly delineate these.
      */
     buffs: Buff[],
     /**

packages/core/src/test/buff_helpers_tests.ts

@@ -0,0 +1,145 @@
+import {Buff} from "../sims/sim_types";
+import {expect} from "chai";
+import {buffRelevantAtSnapshot, buffRelevantAtStart} from "../sims/buff_helpers";
+
+describe("Buff Helpers", () => {
+    describe('simple damage buff', () => {
+        const buff: Buff = {
+            duration: 15,
+            selfOnly: true,
+            name: "Test buff",
+            effects: {
+                dmgIncrease: 0.5
+            },
+        };
+
+        it('is not relevant at start', () => {
+            expect(buffRelevantAtStart(buff)).to.be.false;
+        });
+
+        it('is relevant at snapshot', () => {
+            expect(buffRelevantAtSnapshot(buff)).to.be.true;
+        });
+
+    });
+
+    describe('simple haste buff', () => {
+        const buff: Buff = {
+            duration: 15,
+            selfOnly: true,
+            name: "Test buff",
+            effects: {
+                haste: 10
+            },
+        };
+
+        it('is relevant at start', () => {
+            expect(buffRelevantAtStart(buff)).to.be.true;
+        });
+
+        it('is not relevant at snapshot', () => {
+            expect(buffRelevantAtSnapshot(buff)).to.be.false;
+        });
+
+    });
+
+    describe('no-op buff', () => {
+
+        const buff: Buff = {
+            duration: 15,
+            selfOnly: true,
+            name: "Test buff",
+            effects: {},
+        };
+
+        it('is not relevant at start', () => {
+            expect(buffRelevantAtStart(buff)).to.be.false;
+        });
+
+        it('is relevant at snapshot', () => {
+            expect(buffRelevantAtSnapshot(buff)).to.be.true;
+        });
+    });
+
+    describe('damage + haste buff', () => {
+        const buff: Buff = {
+            duration: 15,
+            selfOnly: true,
+            name: "Test buff",
+            effects: {
+                haste: 10,
+                dmgIncrease: 0.1
+            },
+        };
+
+        it('is relevant at start', () => {
+            expect(buffRelevantAtStart(buff)).to.be.true;
+        });
+
+        it('is relevant at snapshot', () => {
+            expect(buffRelevantAtSnapshot(buff)).to.be.true;
+        });
+    });
+
+    describe('buff with beforeAbility hook', () => {
+        const buff: Buff = {
+            duration: 15,
+            selfOnly: true,
+            name: "Test buff",
+            beforeAbility() {
+            },
+            effects: {},
+        };
+
+        it('is relevant at start', () => {
+            expect(buffRelevantAtStart(buff)).to.be.true;
+        });
+
+        it('is not relevant at snapshot', () => {
+            expect(buffRelevantAtSnapshot(buff)).to.be.false;
+        });
+
+    });
+
+    describe('buff with beforeSnapshot hook', () => {
+        const buff: Buff = {
+            duration: 15,
+            selfOnly: true,
+            name: "Test buff",
+            beforeSnapshot() {
+            },
+            effects: {},
+        };
+
+        it('is not relevant at start', () => {
+            expect(buffRelevantAtStart(buff)).to.be.false;
+        });
+
+        it('is relevant at snapshot', () => {
+            expect(buffRelevantAtSnapshot(buff)).to.be.true;
+        });
+
+    });
+
+
+    describe('buff with modifyDamage hook', () => {
+        const buff: Buff = {
+            duration: 15,
+            selfOnly: true,
+            name: "Test buff",
+            modifyDamage() {
+            },
+            effects: {},
+        };
+
+        it('is not relevant at start', () => {
+            expect(buffRelevantAtStart(buff)).to.be.false;
+        });
+
+        it('is relevant at snapshot', () => {
+            expect(buffRelevantAtSnapshot(buff)).to.be.true;
+        });
+
+    });
+
+});

packages/frontend/src/scripts/sims/components/ability_used_table.ts

@@ -184,7 +184,7 @@ export class AbilitiesUsedTable extends CustomTable<DisplayRecordFinalized> {
             {
                 shortName: 'buffs',
                 displayName: 'Buffs Active',
-                getter: used => used['buffs'] ?? [],
+                getter: (used: DisplayRecordFinalized) => used['buffs'] ?? [],
                 renderer: (buffs: Buff[]) => {
                     return new BuffListDisplay(buffs);
                 },