change(next) confuses users: it does not skip dead pets
bloerwald opened this issue · 2 comments
Given the script change(next), currently the behavior is as follows
| pet 1 | pet 2 | pet 3 | active | result |
|---|---|---|---|---|
| 🟢 | 🟢 | 🟢 | 1 | ➡️ 2 |
| 🟢 | 🟢 | 🟢 | 2 | ➡️ 3 |
| 🟢 | 🟢 | 🟢 | 3 | ➡️ 1 |
| 🟢 | 🟢 | ❌ | 1 | ➡️ 2 |
| 🟢 | 🟢 | ❌ | 2 | 🌀 freeze |
| 🟢 | 🟢 | ❌ | 3 | ➡️ 1 |
| 🟢 | ❌ | 🟢 | 1 | 🌀 freeze |
| 🟢 | ❌ | 🟢 | 2 | ➡️ 3 |
| 🟢 | ❌ | 🟢 | 3 | ➡️ 1 |
| ❌ | 🟢 | 🟢 | 1 | ➡️ 2 |
| ❌ | 🟢 | 🟢 | 2 | ➡️ 3 |
| ❌ | 🟢 | 🟢 | 3 | 🌀 freeze |
| ❌ | ❌ | 🟢 | 1 | 🌀 freeze |
| ❌ | ❌ | 🟢 | 2 | ➡️ 3 |
| ❌ | ❌ | 🟢 | 3 | ❄️ freeze |
| ❌ | 🟢 | ❌ | 1 | ➡️ 2 |
| ❌ | 🟢 | ❌ | 2 | ❄️ freeze |
| ❌ | 🟢 | ❌ | 3 | 🌀 freeze |
| 🟢 | ❌ | ❌ | 1 | ❄️ freeze |
| 🟢 | ❌ | ❌ | 2 | 🌀 freeze |
| 🟢 | ❌ | ❌ | 3 | ➡️ 1 |
The results marked 🌀 are confusing to me and at least one user. Our expectation would have been
| pet 1 | pet 2 | pet 3 | active | result |
|---|---|---|---|---|
| 🟢 | 🟢 | ❌ | 2 | ➡️ 1 |
| 🟢 | ❌ | 🟢 | 1 | ➡️ 3 |
| ❌ | 🟢 | 🟢 | 3 | ➡️ 2 |
| ❌ | ❌ | 🟢 | 1 | ➡️ 3 |
| ❌ | 🟢 | ❌ | 3 | ➡️ 2 |
| 🟢 | ❌ | ❌ | 2 | ➡️ 1 |
i.e. "skip to the next alive pet", not "skip to the next pet if it is alive".
The current implementation is a dumb index = index % C_PetBattles.GetNumPets(Enum.BattlePetOwner.Ally) + 1, so it is working exactly as intended, but I claim that's not what users expect.
There are two options here: Change the behavior of change(next), potentially breaking some user script (but I really doubt people actually expect and rely on this behavior). The alternative is to introduce change(nextusable) which would check whether the new choice actually can be used, only freezing if there is no alternative.
The implementation for either should look somewhat like
local function nextAfter(index)
return index % C_PetBattles.GetNumPets(Enum.BattlePetOwner.Ally) + 1
end
local function canUse(index)
return C_PetBattles.GetHealth(Enum.BattlePetOwner.Ally, index) ~= 0 and
C_PetBattles.CanPetSwapIn(index)
end
index = nextAfter(active)
while not canUse(index) and index ~= active do
index = nextAfter(index)
endThe and C_PetBattles.CanPetSwapIn(index) condition already exists within this action, but as "if that's the case, don't actually change". It is up for debate whether the pets active, forced-to-not-be-active, alive should switch to alive or fail as forced-to-not-be-active is alive but unusable (the current behavior).
I claim that 99% of change(next) are either in scripts just mindlessly using a filler pet, or in scripts where the expectation is that one pet dies one after another 1, 2, win style.
I'd be happy with both options, but feel that there is no need to keep the old behavior (and educating people is harder than the effects of breaking it).
I think the former implementation is correct here.
I can inform those in the pet community (+ xufus) that this may potentially break some scripts, but I agree it shouldn't be an issue.