[FISHING] Implement rings: Penguin, Pelican, Albatross, Noddy

[MowFord]

I affirm:

• [x] I understand that if I do not agree to the following points by completing the checkboxes my PR will be ignored.
• [x] I understand I should leave resolving conversations to the LandSandBoat team so that reviewers won't miss what was said.
• [x] I have read and understood the Contributing Guide and the Code of Conduct.
• [x] I have tested my code and the things my code has changed since the last commit in the PR and will test after any later commits.

What does this pull request do?

Penguin, Pelican, and Albatross correspond to mods, respectively. These mods are on/off style and coded in fishingutils.cpp. All that's needed is for the rings to properly apply the mod via a buff that lasts for 20 minutes

Noddy ring is a bit more in depth but luckily there was already example code for fisherman's apron. Emulated that logic. Also I see no evidence that "decreasing the chance of items/mobs" should simply shift that chance to getting nothing. As these pools are weights and a random number is generated based on their total, I've removed the addition of the weight to NoCatchWeight

I've adjusted the overwrite value for ENCHANTMENT to be 4. This means that it doesn't overwrite other effects with the same "power". This is, as far as I can tell, the only way to have the same status effect multiple times on an entity.

Finally, a video showing it wears on unequip and on zone:

https://imgur.com/76zvqVm

Steps to test these changes

Equip the 3 effect rings, use them to see that you can have multiple buffs. Unequip to see that only that particular buff wears off.

Noddy is hard to test effectively, but the code is very simple. I did test and confirm fishing still works.

[cocosolos]

Not sure if the functionality exists yet, but the ring enchantments should wear off if the ring is removed (also on logout/zone, but that should just be an effect flag that might already be set.)

[MowFord]

Not sure if the functionality exists yet, but the ring enchantments should wear off if the ring is removed (also on logout/zone, but that should just be an effect flag that might already be set.)

Oh interesting. Ok that makes things even more complicated... wear on zone is easily set as an effect flag, but I don't know how to make effects wear on removing gear

[Xaver-DaRed]

itemObject.onItemEquip = function(target, item) itemObject.onItemUnequip = function(target, item)

in this case:

itemObject.onItemUnequip = function(target, item)
    if target:hasStatusEffect(X)
        target:delStatusEffect(X)
    end
end

[TeoTwawki]

for enchantment effect last I knew attaching to the item was done via a subid and stats added in the item script, like with food.

unless someone broke that (its been years since I did one, but breath mantle/high breath mantle does same thing - attach to item)

[TeoTwawki]

https://github.com/LandSandBoat/server/blob/2377b2a3cee236fdad3357366762c9dd86a402ca/scripts/items/high_breath_mantle.lua#L22

you can have multiple unrelated enchantment effects from different items. this is why on retail you can stack the 2 different pelican rings if you have the old version from before SE replaced it.

[cocosolos]

itemObject.onItemEquip = function(target, item) itemObject.onItemUnequip = function(target, item)

in this case:

itemObject.onItemUnequip = function(target, item)
    if target:hasStatusEffect(X)
        target:delStatusEffect(X)
    end
end

I actually wonder what happens if you're wearing 2 Albatross, Duck, or Pelican rings on retail. Does the effect wear only when you remove both rings, or specifically the ring that was used? Would be funny and convenient if it removed the effect when you remove the first ring.

[TeoTwawki]

I actually wonder what happens if you're wearing 2 Albatross, Duck, or Pelican rings on retail. Does the effect wear only when you remove both rings, or specifically the ring that was used?

I wanna say on retail its specific to the actual item you used, but I'm not positive. And I think our implementation will act on item ID regardless of if you have 2 of the same ID :(

edit: actually I forgot - you can't activate 2 of the same item unless they are different IDs. thats why the double I mentioend can work, SE replaced the ring with a new one of diff ID

[Xaver-DaRed]

We could just use effect subpower to save the slot of the item used and THEN add a check for the ring slot placement before removing the effect

[MowFord]

i thought i tried using enchantment and it didn't remove one removing the gear. Also enchantment status effect overwrites other enchantment status effects due to overwrite == 0 on it

[TeoTwawki]

i thought i tried using enchantment and it didn't remove one removing the gear. Also enchantment status effect overwrites other enchantment status effects due to overwrite == 0 on it

oh boy. we have things to fix.. the only enchants that shouldn't be tied to item are if the item ID is not passed it (its optional).

At least the effects flags can be actually changed in script! https://github.com/LandSandBoat/server/blob/base/src/map/lua/lua_statuseffect.h#L68

[cocosolos]

edit: actually I forgot - you can't activate 2 of the same item unless they are different IDs. thats why the double I mentioend can work, SE replaced the ring with a new one of diff ID

You might not be able to activate them but you can still equip 2 since those ones are only Ex.

We could just use effect subpower to save the slot of the item used and THEN add a check for the ring slot placement before removing the effect

This sounds like it would work.

[MowFord]

At least the effects flags can be actually changed in script! https://github.com/LandSandBoat/server/blob/base/src/map/lua/lua_statuseffect.h#L68

afaik you can't set flags on an effect before applying it, and when you apply the overwrite check is done

[TeoTwawki]

afaik you can't set flags on an effect before applying it, and when you apply the overwrite check is done

That's true, but the second the effect object exists you can chain a owningObject:getStatusEffect(ENUM):setStuffAndThings() together to act on it - even immediately after its born. we do that in a few places. inside an effect script or an item script you don't even have to go that far. Remember all those effect:getPower() all over the repo? Same concept. And changes there will only apply to that specific instance of the effect.

[MowFord]

Got an example somewhere of that? I thought the flag checking was done within the addedfect binding

[TeoTwawki]

Got an example somewhere of that? I thought the flag checking was done within the addedfect binding

we actually stripped flags+add death flag immediately after adding spike effects (so right after effect was born) to a mob to prevent it being removed, so you can for sure change flags and they will correctly be checked outside of the addEffect binding.

[MowFord]

Also the issue being discussed is the overwrite value of an effect, not the flag specifically.

If overwrite is 4, then it lets two buffs with the same power coexist. I suspect we just need to make the enchantment buff have overwrite == 4, but that's a big change for a lowly dev like me

[MowFord]

Also linking to a related issue

https://github.com/LandSandBoat/server/issues/4785

[MowFord]

Allright I did the thing. Note that ENCHANTMENT being able to live alongside other of the same effect means this logic is now flawed:

I might have opened a can of worms on this one, but I think that in general ENCHANTMENT effects should be able to live alongside other ENCHANTMENT effects