Add potion effect tier

[Ankoki]

Description

---

Adds an expression to get the tier/amplifier of a potion effect type.
Currently returns 0 if the player doesn't have that type, but could return an empty Number array instead if preferred (links to the next point)?
Another thing to note, as this returns the amplifier, if a player has the effect haste at tier 1, it'll return 0. Should this stay that way or would it be preferred to add an extra 1 to it so it makes more sense? As I made the pattern tier/amplifier, it could be confusing as you use apply tier %number%, lmk any thoughts ^-^
Target Minecraft Versions: any
Requirements: n/a
Related Issues: n/a afaik

[AyhamAl-Ali]

I would say let it start at 1 instead of 0 otherwise zero will be confusing whether that player doesn't have the potion or has tier 1 of it

[TPGamesNL]

I think it'd be better to return null instead of 0.
EDIT: reason for it being: if you apply a potion effect of tier 0, it'll still apply the potion but with tier 1

[Ankoki]

added both these suggestions, I returned an empty array instead of null to be consistent with earlier in the method, would you prefer I returned new Number[0] or null? I'm not quite sure if there's a difference

[TPGamesNL]

There's no difference

[AyhamAl-Ali]

I think it'd be better to return null instead of 0. EDIT: reason for it being: if you apply a potion effect of tier 0, it'll still apply the potion but with tier 1

Regarding that, would if tier of haste of player < 3 work if it returns null?

[Ankoki]

I think it'd be better to return null instead of 0. EDIT: reason for it being: if you apply a potion effect of tier 0, it'll still apply the potion but with tier 1

Regarding that, would if tier of haste of player < 3 work if it returns null?

I'd like to think so, it shouldn't error however it should always return false as the value isn't set, ill give it a go

@AyhamAl-Ali just gave it a shot, outcome is as expected.

[AyhamAl-Ali]

In this case it returns unexpected value, you would need to add another condition to check if entity has potion which for me this is just an extra unneeded condition

E.g. this code won't work as expected

on break:
    if tier of haste of player < 3: # this will *not* pass if tier returns null which shouldn''t happen
        send "You need higher haste level to mine here"
        cancel event

Regarding the case TP mentioned it would be better to change its behavior to not apply a potion of tier 0, what do you think @TPGamesNL ?

[Ankoki]

i see your point, i think having 0 instead of null makes more sense now that we've added 1 to the amplifier result

[APickledWalrus]

This is just a note for me. #4183 will need to account for this expression :)

[AyhamAl-Ali]

I forgot you're reworking potions :'D

[TPGamesNL]

In this case it returns unexpected value, you would need to add another condition to check if entity has potion which for me this is just an extra unneeded condition

E.g. this code won't work as expected

on break:
    if tier of haste of player < 3: # this will pass if tier returns null which shouldn't happen
        send "You need higher haste level to mine here"
        cancel event

That wouldn't be the case if tier returns null, as shown in the screenshot:

The condition would return false

Regarding the case TP mentioned it would be better to change its behavior to not apply a potion of tier 0, what do you think @TPGamesNL ?

My bad, I tested with the bukkit method not the skript effect and they use amplification, not tiers. Disregard that comment

[AyhamAl-Ali]

That wouldn't be the case if tier returns null, as shown in the screenshot: The condition would return false

Oh I meant this will *not* pass my bad

[TPGamesNL]

In that case it would indeed be better to return 0 instead of null

[Ankoki]

i'll change that as soon as i get access to my laptop again

[Moderocky]

A nice little addition.

In the future I think it would be nice if this supported changers (e.g. add/remove 1 from the tier) but since that would require some trickery it's not necessary now.

[Ankoki]

A nice little addition.

In the future I think it would be nice if this supported changers (e.g. add/remove 1 from the tier) but since that would require some trickery it's not necessary now.

might be something for @APickledWalrus to think about when he does his rework of potions

[APickledWalrus]

sure - I'll take a look into that when I next work on the PR

[TPGamesNL]

This should probably support %potioneffect% as well, but could also be something for potions rework

[APickledWalrus]

This should probably support %potioneffect% as well, but could also be something for potions rework

That might be better for a separate syntax since you wouldn't need a living entity. Maybe it could be like ExprPotionProperties and include other properties like duration

[TPGamesNL]

That might be better for a separate syntax since you wouldn't need a living entity. Maybe it could be like ExprPotionProperties and include other properties like duration

Yes, but then we'd also need an expression to get a PotionEffect of a certain PotionEffectType from an entity, and together with a property expression for a potioneffect's tier, this expression will be obsolete

[Ankoki]

i'll work on those changes as soon as i'm home! :)

[TheLimeGlass]

Requesting new review from @APickledWalrus due to decent amount of changes since last review not by me.

[bluelhf]

Syntactically, this looks a lot like an applied potion effect property on living entities followed by separate syntax for the tier of an applied potion effect... For example tier of player's haste effect isn't valid syntax here (even though arguably it should be).

Was it planned to replace this syntax with properly implemented support for (applied) potion effects in the future? If not, why?

[TheLimeGlass]

Syntactically, this looks a lot like an applied potion effect property on living entities followed by separate syntax for the tier of an applied potion effect... For example tier of player's haste effect isn't valid syntax here (even though arguably it should be).

Was it planned to replace this syntax with properly implemented support for (applied) potion effects in the future? If not, why?

I don't like how that sounds.

Potion support #4183

[bluelhf]

Syntactically, this looks a lot like an applied potion effect property on living entities followed by separate syntax for the tier of an applied potion effect... For example tier of player's haste effect isn't valid syntax here (even though arguably it should be).
Was it planned to replace this syntax with properly implemented support for (applied) potion effects in the future? If not, why?

I don't like how that sounds.

Potion support #4183

Ah, the potion support PR looks fairly good — I still think we should be utilising types more than we currently are: the currently proposed syntaxes emulate type-like behaviour anyways, and types would allow for more syntactical versatility and improved code readability and maintainability.

[TheLimeGlass]

Syntactically, this looks a lot like an applied potion effect property on living entities followed by separate syntax for the tier of an applied potion effect... For example tier of player's haste effect isn't valid syntax here (even though arguably it should be).
Was it planned to replace this syntax with properly implemented support for (applied) potion effects in the future? If not, why?

I don't like how that sounds.
Potion support #4183

Ah, the potion support PR looks fairly good — I still think we should be utilising types more than we currently are: the currently proposed syntaxes emulate type-like behaviour anyways, and types would allow for more syntactical versatility and improved code readability and maintainability.

The potions rework pull request has been stated to be completely recoded, if you have any continued suggestions for it, you can use that pull request discussion.