ability.duration > .... bug still exists

Question

ability.duration > .... bug still exists

cosinussinus opened this issue 2 years ago · 11 comments

cosinussinus commented 2 years ago

erigul · Answer 1 · 2022-11-19T16:24:30.000Z

Is there ever a case where argParse returning nil is valid, which i think is what happens here?
I'm thinking something more fundamental where RunCondition returns false before it even gets to calling fn if argParse is provided on the condition but it returned nil

axc450 · Answer 2 · 2022-11-19T13:43:18.000Z

@bloerwald is the expert on non-existence and default values. Does DengSir/tdBattlePetScript#26 answer your question? I guess we may need to change this to just error instead

cosinussinus · Answer 3 · 2022-11-19T13:55:35.000Z

I'm not a programming expert and it might be bad practice, but how about:

Extension/Conditions.lua:

Addon:RegisterCondition('ability.duration', { type = 'compare', argParse = Util.ParseAbility }, function(owner, pet, ability)
    local isUsable, currentCooldown, currentLockdown = C_PetBattles.GetAbilityState(owner, pet, ability)
    return ability and currentCooldown or infinite
end)

-->

Addon:RegisterCondition('ability.duration', { type = 'compare', argParse = Util.ParseAbility }, function(owner, pet, ability)
    local isUsable, currentCooldown, currentLockdown = C_PetBattles.GetAbilityState(owner, pet, ability)
    return currentCooldown
end)

Core/Condition.lua:

function Condition:RunCondition(condition)
    local owner, pet, cmd, arg, op, value = self:ParseCondition(condition)

    local fn  = self.apis[cmd]
    local opts = self.opts[cmd]
    if not fn then
        error('Big Bang !!!!!!')
    end

    local res = fn(owner, pet, arg)
    return opTabler[opts.type][op](res, value)
end

-->

function Condition:RunCondition(condition)
    local owner, pet, cmd, arg, op, value = self:ParseCondition(condition)

    local fn  = self.apis[cmd]
    local opts = self.opts[cmd]
    if not fn then
        error('Big Bang !!!!!!')
    end

    local res = fn(owner, pet, arg)

    if res == nil and cmd == 'ability.duration' then
        return false
    end

    return opTabler[opts.type][op](res, value)
end

bloerwald · Answer 4 · 2022-11-20T13:47:05.000Z

I sadly just don't think there is a generic "correct" solution to this question, but I feel like "if condition invalid, false" is wrong. Given

use(#1) [ self.ability(nonexistent).duration > 1 ]
use(#2)

the old implementation always did 1, the new one always does 2. Given

use(#2) [ self.ability(nonexistent).duration <= 1 ]
use(#1)

the old implementation also always did 1, the new one always does 1.

Which one does the user want? The user writing script A apparently wants always-2, but what does the user want given they wrote the script the other way around? Surely they also want 2, not 1?

This means that you can't write generic scripts like that if the condition evaluates to false. The old implementation might be unexpected, but it is consistent at least. a = not a is a bad thing to do imo, even in an error case. I'd rather add support to make scripts error out at that point…

erigul · Answer 5 · 2022-11-20T13:52:56.000Z

It seems fairly consistent to me if a condition is always false for an ability your pet doesn't have (so doesn't matter if your comparison is > 1 or <= 1, both would fail the condition as it never does the comparison with something like #18 ).
That's pretty much be same behavior use has where it just skips if you specify the name of a spell the current pet doesn't have.

cosinussinus · Answer 6 · 2022-11-20T14:32:38.000Z

Does a nonexistent ability have a duration? I don't think so. At least we can't prove it. So it should be false or an error.

bloerwald · Answer 7 · 2022-11-20T18:57:44.000Z

.ability([^)]*).duration exists in 798 out of 5895 scripts on xufu but only 109 scripts compare with < or >. There are only 47 scripts using .ability([^)]*).duration *>, the only case that would change with the suggested patch. Whatever decision you take, I suggest to not do it quickly and discuss it with users, even if these are fringe cases, as it affects all other ability conditions.

In reality, most scripts I have actually looked at will not be influenced by this change: They all already guard against that case or they use duration < x which doesn't change.

The following scripts are probably influenced (only relevant lines of script copied):

https://wow-petguide.com/Strategy/13027/Rogue_Azerite-Dragonkin

standby [ self.ability(only-with-pet1).duration > 1 & self.ability(only-with-pet1).duration > 1 ]

This is wrong in my definition of duration: It will always fire, while the suggested change only fires with pet1. Your change would fix it (or defining no-cooldown = 0).

https://wow-petguide.com/Strategy/8639/Snakes_on_a_Terrace

ability(Prowl:536) [enemy.ability(Dive:564).duration>2]
ability(Prowl:536) [enemy(#3).active & self(#1).active]

This is fine: only enemy 2 has Dive, so no change at all whether duration = inf or 0.

https://wow-petguide.com/Strategy/15167/Snakes_on_a_Terrace-Elemental

ability(Blazing Yip:1359) [self(#2).dead & self.ability(Puppies of the Flame:1354).duration>3]

Condition will be true, but only the pet having Puppies has Blazing, so no change.

https://wow-petguide.com/Strategy/10758/Airborne_Defense_Force-Dragonkin

use(Arcane Storm:589) [ !enemy(#2).active & enemy.round<4 ]
use(Arcane Storm:589) [ !weather(Arcane Winds:590) & enemy.ability(Toxic Fumes:2349).duration>1 ]

Only enemy 2 has Toxic Fumes. This condition might be true for Enemy 1&3 wrongly. Currently, it is true if [weather is not up, enemy #1 round < 4, enemy #3 round < 4], with change it is true if [enemy #1 round < 4, enemy #3 round < 4]. That's a change, but it is irrelevant to the strategy if I understand correctly.

https://wow-petguide.com/Strategy/10828/Extra_Pieces-Critter

use(Stampede:163) [ self.ability(Dodge:312).duration>1 ]

Only pets with both abilities exist.

https://wow-petguide.com/Strategy/9603/Cursed_Spirit

-- 9.01 duration bug repair
if [enemy(#1).active]
use(Locust Swarm:1007) [enemy.ability(Ghostly Bite:654).duration>1]
endif
--
use(Scratch:119)

This script assumes that .ability(nonexistent).duration = 0 rather than inf. Your change would be fine, but defining no-cooldown = 0 would do the same. Alternatively,

use(Scratch:119) [enemy.ability(Ghostly Bite:654).duration <= 1]
use(Locust Swarm:1007)

would do the same.

https://wow-petguide.com/Strategy/5940/Crypt_Fiend

if [ self.ability(Breath:115).usable ]
    use(Decoy:334) [ enemy.ability(Nightmare:1731).duration<2 ]
    use(Breath:115)
endif

dead code, no pets have Breath.

Of course a non-existent ability doesn't have a cooldown, so that would be an error. I would instantly agree with an error since that's safe. My goal with the old ticket was making it semantically sane and somewhat-how-people-expect-it, while still holding the requirement of basic logic, i.e. a = a => a != not a. I'd also agree on changing the definition to non-existent = 0 if users confirm that's what they expect. non-existent = silent ignore does not seem to be what people want, most of the time.

From a programming perspective, I find a line being silently ignored completely weird. I don't feel like that's the same as with use (nonexistent) since that's action vs condition. An action not working is different from the condition for using that action not working. x [true] can still fail to execute. x [true] should not not execute if the current pet doesn't have true though.

cosinussinus · Answer 8 · 2022-11-20T22:24:12.000Z

What people expect:
[self.ability(nonexistent).duration>1] same as [self.ability(nonexistent).exists & ...duration>1] (implicit) --> false & error --> False comes first. So I think most people would expect a false.
What they don't expect:
[self.ability(nonexistent).duration>1] --> [self.ability(nonexistent).exists & ...duration>1] --> true & ('inf' > 1) --> True (#17 (comment))

bloerwald · Answer 9 · 2022-11-21T01:47:27.000Z

[self.ability(nonexistent).duration>1] --> [self.ability(nonexistent).exists & ...duration>1] --> true & ('inf' > 1) isn’t what is happening. [self.ability(nonexistent).duration>1] --> 'inf' > 1. [self.ability(nonexistent).exists & self.ability(nonexistent).duration>1] --> FALSE & ('inf' > 1).

i have seen the script

use(ultimate) [ enemy.ability(dodge).duration > 0 ]
use(filler)

which surely wants to use ultimate if the enemy can’t ever dodge.

I‘ll remove myself from this discussion and let @axc450 decide.

cosinussinus · Answer 10 · 2022-11-21T10:09:28.000Z

I know under the hood is "[self.ability(nonexistent).duration>1] --> 'inf' > 1" happening. But I would expect that the ability will be checked first if the ability exists. Maybe it's wrong but that's what I would expect.

use(ultimate) [ enemy.ability(dodge).duration > 0 ]
use(filler)

--> I don't want to get the information whether the enemy can dodge or not. It's clear, I want to know the duration (>0). Big difference. And when the ability doesn't exists then I can't get duration informations --> error

Anyway, thanks for the time and effort to analyze the scripts from xufu.

axc450 · Answer 11 · 2023-05-07T19:08:22.000Z

Ultimately, I think the correct answer here is that things need to be redesigned internally so that functions stop returning bools and return actual errors which will halt the scripts on the offending line. There are good points on both sides but when you have something invalid, I dont think it should return a bool at all.

Leaving this issue open until we have had a proper think about how a redesign would work.

Share to