pf2e icon indicating copy to clipboard operation
pf2e copied to clipboard

Apply priority for AdjustModifiers across different selectors

Open xyzzy42 opened this issue 4 months ago • 2 comments

When AdjustModifiers are applied, they are sorted first on the selector, then on priority. This means that, e.g., all AdjustModifiers that select {item|id}-damage will be applied (in order of ascending priority) before any AdjustModifiers that select strike-damage.

Example:

{  "key": "FlatModifier",  "label": "Test",  "selector": "strike-damage",  "value": 1,  "slug": "test" }
{  "key": "AdjustModifier",  "selector": "strike-damage",  "mode": "override",  "value": 2,  "priority": 10,  "slug": "test" }
{  "key": "AdjustModifier",  "selector": "{item|id}-damage",  "mode": "override",  "value": 3,  "priority": 20,  "slug": "test" }

It might be expected that the final value of test will be 3, but it will be 2. This is because the list of domains for the damage roll has {item|id}-damage before strike-damage, so the 3rd RE is found first, then the 2nd RE later.

This could be fixed by: A) Adding priority to the objects placed into actor.synthetics.modifierAdjustments[domain], then sorting the extracted list of modifiers created by extractModifierAdjustments() by priority. B) Adding the selector to the same objects, and putting them into a single, priority sorted, list in modifierAdjustments. Then extract them by filtering on the selector membership in the Set() of domains for the roll. This will preserve overall priority order.

With method A, extracting adjustments is O(n log n) time, while method B is O(n), assuming a Set() implementation with constant time lookup, i.e. a hash based Set. Which is what is used.

xyzzy42 avatar Apr 06 '24 17:04 xyzzy42

I'd have to see how big the pulled data is (since we pull before testing) but if the lists are small A is actually faster because of lower overhead. It should also be possible to delay said filtering into the moment to the adjustments are actually applied (after testing), further reducing how many items are being sorted.

CarlosFdez avatar Apr 07 '24 22:04 CarlosFdez

For a strike damage roll, there are about 13 domains to iterate over. Typical actor has maybe 5 modifierAdjustments.

So it's the smaller list being processed to filter all adjustments based on selector than than to iterate over all domains and join the arrays of pre-sorted adjustments. I think that alone would make the single list method (B) faster. But the multiple list method (A) then requires sorting the joined arrays, which single list avoids.

Data structure is smaller too. modifierAdjustments is an array of adjustments. Vs modifierAdjustments being an object that contains about a half dozen arrays of adjustments. I.e., it saves one level of object nesting.

There's another limitation of the current system. AdjustModifiers with multiple selectors don't work. They get joined into an single domain, e.g. "selector":["master-damage", "legendary-damage"] is added to the field modiferAdjustments["master-damage,legendary-damage"] and nothing finds it. With the method A, this is trivial to support multiple selectors, by making modifierAdjusetments[x].selectors an array (or a Set).

xyzzy42 avatar Apr 07 '24 23:04 xyzzy42