Allow multiple different slot type items in single moveevent

[NRH-AA]

Pull Request Prelude

• [x] I have followed proper The Forgotten Server code styling.
• [x] I have read and understood the contribution guidelines before making this PR.
• [x] I am aware that this PR may be closed if the above-mentioned criteria are not fulfilled.

Changes Proposed

Iterate over all id's linked in onEquip and onDequip to allow multiple slot types to be registered with the same event. Currently it only selects the first id and assumes all other ids will use the same slot type.

Issues addressed:

[ranisalt]

I think it's fine to duplicate some lines to solve the issue. Is the problem deeper than I'm thinking?

[EvilHero90]

I think it's fine to duplicate some lines to solve the issue. Is the problem deeper than I'm thinking?

There is already an existing workaround which can be fully done in lua, which consists of looping over the ids and then dynamicaly create the moveevents in every loop cycle where we can just set the slot accordingly example:

local equipmentData = {
    [2161] = {slot = "necklace"},
    [2170] = {slot = "necklace"},
    [2172] = {slot = "necklace"},
    [2173] = {slot = "necklace"},
    [2197] = {slot = "necklace"},
    [2198] = {slot = "necklace"},
    [2199] = {slot = "necklace"},
    [2200] = {slot = "necklace"},
    [2201] = {slot = "necklace"},
    [7887] = {slot = "necklace", level = 60},
    [7888] = {slot = "necklace", level = 60},
    [7889] = {slot = "necklace", level = 60},
    [7890] = {slot = "necklace", level = 60},
    [8266] = {slot = "necklace"},
    [10218] = {slot = "necklace", level = 80},
    [10219] = {slot = "necklace", level = 80},
    [10220] = {slot = "necklace", level = 80},
    [10221] = {slot = "necklace", level = 80},
    [11374] = {slot = "necklace"},
    [15403] = {slot = "necklace", level = 120},
    [18402] = {slot = "necklace", level = 150},
    [18407] = {slot = "necklace", level = 150},
    [21691] = {slot = "necklace", level = 150},
    [23541] = {slot = "necklace", level = 75},
    [23554] = {slot = "necklace", level = 75, vocation = {{"Sorcerer", true}, {"Master Sorcerer"}, {"Druid", true}, {"Elder Druid"}}},
    [24717] = {slot = "necklace"},
    [24790] = {slot = "necklace"},
    [24851] = {slot = "necklace", level = 60},
    [25423] = {slot = "necklace", level = 100},
    [25424] = {slot = "necklace", level = 100},
    [26182] = {slot = "necklace", level = 100},
    [26183] = {slot = "necklace", level = 100},
    [26184] = {slot = "necklace", level = 100},
    [26198] = {slot = "necklace", level = 150, vocation = {{"Paladin", true}, {"Royal Paladin"}}},
    [26199] = {slot = "necklace", level = 150, vocation = {{"Sorcerer", true}, {"Master Sorcerer"}, {"Druid", true}, {"Elder Druid"}}},
    [26200] = {slot = "necklace", level = 150, vocation = {{"Knight", true}, {"Elite Knight"}}}
}

local deEquipData = {
    2161, 2170, 2172, 2173, 2197, 2198, 2199, 2200, 2201, 7887, 7888, 7889, 7890, 8266, 10218, 10219, 10220, 10221, 11374, 15403, 18402, 18407, 21691, 23541, 23554, 24790, 24851, 25423, 25424, 26182, 26183, 26184, 26186, 26188, 26190
}

for id, data in pairs(equipmentData) do
    local equip = MoveEvent()
    equip:type("equip")
    equip:id(id)
    equip:slot(data.slot)

    if data.level then
        equip:level(data.level)
    end

    if data.vocation then
        for _, vocation in pairs(data.vocation) do
            equip:vocation(unpack(vocation))
        end
    end

    equip:register()
end

for _, id in pairs(deEquipData) do
    local deEquip = MoveEvent()
    deEquip:type("deequip")
    deEquip:id(id)
    deEquip:slot("necklace")
    deEquip:register()
end

This obviously just lists up necklaces in this case but it could obviously be a mixed use case aswell.

I had plans to refactor this entirely in #4629 that it's always dependant on the items.xml slot type, that way we wouldn't have to add it manualy anyway.

[NRH-AA]

Are you sure it will only grab the last one? I am currently using this and it fixed the problem for me. All items are in the same table and I do not have to specify slots nor do I have to do the lua work around you showed. It seems to work as expected. Could you please explain why it is the case you believe it only grabs the last slot? @EvilHero90

edit: nvm I see the issue but its weird because it literally works just fine but clearly there is some magic happening

I don't see anywhere in the code you linked that assigns a slot to the moveevent.

[ranisalt]

Are you sure it will only grab the last one?

Yes, it does. When you run setSlot, it overrides the slot value, it does not add to the existing slots.

[EvilHero90]

I've been researching this for a while now, actually this is quite interesting, overall we wouldn't need to add a slot function at all we would have to just change one line and it should work directly from items.xml getting rid of a lot non necessary functionality which actualy doesn't even work as it was expected. https://github.com/otland/forgottenserver/blob/a4471edb3aebd3e1eaf41d9584f59deab391a414/src/movement.cpp#L365

if (item->getSlotPosition() == slotp) {

This should already do the trick, if someone cares to test.

[EPuncker]

I've been researching this for a while now, actually this is quite interesting, overall we wouldn't need to add a slot function at all we would have to just change one line and it should work directly from items.xml getting rid of a lot non necessary functionality which actualy doesn't even work as it was expected.

https://github.com/otland/forgottenserver/blob/a4471edb3aebd3e1eaf41d9584f59deab391a414/src/movement.cpp#L365

if (item->getSlotPosition() == slotp) {

This should already do the trick, if someone cares to test.

indeed it always bothered me that we had to declare the same stuff in two different places (items.xml and movements)

[NRH-AA]

I will check this out and report back

[NRH-AA]

Closing this for now. Going to focus on pathfinding until that hopefully gets merged.