arkayenro / arkinventory

A World of Warcraft Inventory mod for Retail, Burning Crusade, and Classic
108 stars 14 forks source link

[BUG] ItemRack outfit rule doesn't work for rune slots in Season of Discovery #1864

Open nuiva opened 7 months ago

nuiva commented 7 months ago

What version number you are using? v31024 ItemRack v3.77

What game client are you playing? Retail, Wrath, Classic / Live, PTR, Beta Classic, Season of Discovery Live

What language is the game client set to? English

Describe the bug The ItemRack outfit rule provided by ArkInventoryRules (specifically ArkInventoryRules.lua:1000) doesn't work for armor items, which the player can equip with a Season of Discovery rune. The rule function always returns false for such armor items, even if they are assigned to an ItemRack set.

The root cause is that ItemRack (specifically ItemRack.lua:683) appends :runeid:#### to the item ID, so ArkInventoryRules.lua:1017 will compare something like "item:12345:0:0:0:0:0:0" == "item:12345:0:0:0:0:0:0:runeid:0", which always evaluates to false, even if the item should match.

My quick fix was to change ArkInventoryRules.lua:1015 to osd = ArkInventory.ObjectStringDecode( string.format( "item:%s", string.gsub(setitem, ":runeid:%d+", "" ) ) ), which removes the runeid appended by ItemRack. This does mean that the rule function cannot distinguish copies of the same item with different runes. Another approach would be to append runeID to ArkInventory's h_rule the same way as ItemRack does, which would allow the rule function to distinguish items with different runes.

To Reproduce

  1. Log on a Season of Discovery character that is able to engrave gloves with a rune.
  2. Create a rule named "Outfit", whose formula is "outfit()".
  3. Enable the rule and assign it into an ArkInventory bar.
  4. Get any gloves and assign them in an ItemRack set.
  5. Refresh the ArkInventory bag view.
  6. Observe that the gloves of step 4 are still in the default bar, instead of the bar created in step 3.

Screenshots N/A

Additional context N/A

Ageous27 commented 1 month ago

Issue still exists in v31100.Alpha.4

The suggested workaround also still works.

Deamp commented 1 month ago

Issue still exists in v31101, workaround moved to ArkInventoryRules.lua:1016.

arkayenro commented 1 month ago

are runes on the slot, or on the item?

ie can you have different say rings with a different rune on each one and swap those around or is the rune slot specific and you can put any item in there to activate the rune?

as far as i can tell (and i could be wrong) the itemrack code doesnt appear to be checking the item for a rune, its checking the slot, so no matter which item you give it, its always going to return the same rune value (whatever is on the slot / or active at the time), so im not sure how that helps to differentiate between two identical items that are engraved differently.

Ageous27 commented 1 month ago

They are on the item. You can quickly change runes by having 2 rings for example with different runes, and just swap rings. To change the rune on the ring invokes a cast time bar.

arkayenro commented 1 month ago

ok, if its not too costly could you grab any item that is in your bags (not wearing it), enable edit mode, click on the item, select debug, note down the cache id (rule) value, disable edit mode

apply any rune to the item, repeat the steps above and note if the cache id (rule) value changed in any way or not

apply a different rune to the item, repeat the steps above and note if the cache id (rule) value changed in any way or not

Ageous27 commented 4 weeks ago

I had to find a new item that I had never applied a rune to, once you apply one I don't know how to clear it completely. But anyway, it didn't change, from no rune, to applying a rune, to applying a different rune.

Cache ID (Rule): 1:3:5:3:1001:item: 13526:1:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0

That's what it showed and it didn't change. I even closed my bags and reopened them to make sure everything updated in between.

arkayenro commented 4 weeks ago

adding runeid like that does horrible things to itemstring decoders.

i've changed the way the itemrack outfit rule function works and it should pickup runes now.

should be fixed in the next alpha release

arkayenro commented 3 weeks ago

try alpha 4 when it turns up and let me know if that works or not

Deamp commented 1 week ago

This situation gets a little tricky, there are currently 2 ItemRack versions for SoD/Era.

  1. https://www.curseforge.com/wow/addons/itemrack-classic // https://github.com/Rottenbeer/ItemRack

    • this version adds runeID to the item
    • this version has way more downloads and i know a lot of SoD players using this version
    • this version does not consider runeIDs when changing to an itemSet, which means if I for example have a ring with runeA saved to the ItemSet and change the rune of the ring to runeB it is still in the same itemSet when i equip that item
    • this version does not support items with a suffix like "of Frost Resistance" or "of Fire Resistance": image
  2. https://www.curseforge.com/wow/addons/itemrack-classic-sod // https://github.com/UpG-Labs/ItemRack

    • this version adds runeID to the item
    • this version is not that widely used (13k dls on curse)
    • this version supports runeIds for itemSets, but has no option to disable this functionality (use-case for disabling the feature: i only have one item and sometimes i need runeA and sometimes i need runeB but in general i just want the item equipped)
    • this version does support items with a suffix like "of Frost Resistance" or "of Fire Resistance"

This situation leads to: users of "1" will now have their item depending on what rune is equipped in different lists in the inventory

Considering that this is a feature not a lot of people are using, the focus should be on "2" which the new version of arkInventory supports.

As for what i will be doing going forward is: using "2" to get the functionality of suffixes removing all "runeID" related stuff in ItemRack.lua removing all "runeID" related stuff in ArkInventoryRules.lua this lets me have my itemSets clustered in my inventory and my desired functionality with ignoring runes on itemSets in ItemRack

@Ageous27 & @nuiva what are your thoughts on this and what itemRack version you guys using? kind of a mess because ItemRack updates/support is not that good for SoD

arkayenro commented 1 week ago

ive updated the code in the itemrack rule function to check for the ItemRack.AppendRuneID function, if found it will check against the rune variant, if not it wont (it expects itemrack to not use them either).

this should allow you to strip it out of the itemrack version you want to use without having to muck around with arkinventory.