Xruptor / BagSync

BagSync tracks your characters items and displays it within tooltips.
http://www.wowinterface.com/downloads/info15351-BagSync.html
Other
32 stars 21 forks source link

[Cataclysm Classic] Equipped items are also counted to be located in a bag #337

Closed kanodiry closed 3 months ago

kanodiry commented 3 months ago

If you view items as another character, the equipped items will also count as in the bag, and the character will be considered to have two items at once: equipped and in the bag. I checked SavedVariables, and this confuses me: As you can see there is ["bag"][-2] contains all equipped items plus 2 bags (don't know why).

BagSyncDB = {
    ["Realm"] = {
        ["Character"] = {
            ["bag"] = {
                {
                    "37903;21", -- [1]
                    "57914", -- [2]
                    "56449", -- [3]
                    "62251;16", -- [4]
                    "62251;20", -- [5]
                    "58085;4", -- [6]
                    "62662;11", -- [7]
                    "9172;2", -- [8]
                    "53051;20", -- [9]
                    "33447;20", -- [10]
                    "57191;8", -- [11]
                    "57191;20", -- [12]
                    "43015;18", -- [13]
                    "62662;20", -- [14]
                    "62662;20", -- [15]
                    "73260;14", -- [16]
                }, -- [1]
                {
                    "8529;20", -- [1]
                    "8529;15", -- [2]
                    "6657;18", -- [3]
                    "68644;17", -- [4]
                    "71096;208", -- [5]
                    "64396", -- [6]
                    "52843", -- [7]
                    "58090;2", -- [8]
                    "60853", -- [9]
                    "6522;7", -- [10]
                }, -- [2]
                {
                    "45439", -- [1]
                    "42101", -- [2]
                    "19123", -- [3]
                    "792", -- [4]
                    "63388;5", -- [5]
                    "68646", -- [6]
                    "71083;17", -- [7]
                    "59477", -- [8]
                    "59491", -- [9]
                    "42420", -- [10]
                    "52078;9", -- [11]
                }, -- [3]
                {
                    "56278", -- [1]
                    "56330", -- [2]
                    "63458", -- [3]
                    "51832", -- [4]
                    "55797", -- [5]
                    "51879", -- [6]
                    "55260", -- [7]
                    "57260", -- [8]
                    "62350", -- [9]
                    "56129", -- [10]
                    "61472", -- [11]
                    "55270", -- [12]
                    "56393", -- [13]
                    "56458", -- [14]
                    "63787", -- [15]
                    "50459", -- [16]
                    "63206", -- [17]
                    "50361", -- [18]
                    "65905", -- [19]
                    "65908", -- [20]
                    "52252", -- [21]
                    "43155", -- [22]
                    "46874", -- [23]
                    "22999", -- [24]
                }, -- [4]
                [0] = {
                    "6948", -- [1]
                    "40110", -- [2]
                    "5507", -- [3]
                    "39769", -- [4]
                    "52251", -- [5]
                    "49040", -- [6]
                    "37863", -- [7]
                    "40772", -- [8]
                    "23821", -- [9]
                    "67494", -- [10]
                    "60223", -- [11]
                    "45631", -- [12]
                    "33820", -- [13]
                    "44050", -- [14]
                    "40769;20", -- [15]
                    "49633", -- [16]
                },
                [-2] = {
                    "59359", -- [1]
                    "57932", -- [2]
                    "56452", -- [3]
                    "14617", -- [4]
                    "58101", -- [5]
                    "55059", -- [6]
                    "58102", -- [7]
                    "62432", -- [8]
                    "56416", -- [9]
                    "58105", -- [10]
                    "58187", -- [11]
                    "56398", -- [12]
                    "55881", -- [13]
                    "56347", -- [14]
                    "62383", -- [15]
                    "64377", -- [16]
                    "56337", -- [17]
                    "69210", -- [18]
                    "51809", -- [19]
                    "51809", -- [20]
                },
            },
            ["equip"] = {
                "59359", -- [1]
                "57932", -- [2]
                "56452", -- [3]
                "14617", -- [4]
                "58101", -- [5]
                "55059", -- [6]
                "58102", -- [7]
                "62432", -- [8]
                "56416", -- [9]
                "58105", -- [10]
                "58187", -- [11]
                "56398", -- [12]
                "55881", -- [13]
                "56347", -- [14]
                "62383", -- [15]
                "64377", -- [16]
                "56337", -- [17]
                "69210", -- [18]
            },
Xruptor commented 3 months ago

Unless they changed something (which I wouldn't be surprised...) then it shouldn't be doing that. Blizzard has a habit of constantly changing their code, which is why as an addon author it's getting frustrating dealing with it.

Do me a favor, try a DB reset and see it it still happens.

/bgs resetdb

kanodiry commented 3 months ago

Sorry for not mention this, I did:

  1. /bgs fixdb
  2. /bgs resetdb
  3. Delete from SavedVariables BagSync.lua (and .bak)
Xruptor commented 3 months ago

Sorry for not mention this, I did:

  1. /bgs fixdb
  2. /bgs resetdb
  3. Delete from SavedVariables BagSync.lua (and .bak)

That means something else is going on and I'd need to investigate if it was STILL saving the equipment as part of the bags. Which it shouldn't.

Xruptor commented 3 months ago

The bag in question that is saving the equipment, should be the Keyring bag. I'm not entirely sure how the equipment is being stored there and it could possibly be a bug on the server itself. I'll review the code but here is the bag numbers in code. https://wowpedia.fandom.com/wiki/BagID

-2 is the KEYRING_CONTAINER

kanodiry commented 3 months ago

The bag in question that is saving the equipment, should be the Keyring bag. I'm not entirely sure how the equipment is being stored there and it could possibly be a bug on the server itself. I'll review the code but here is the bag numbers in code. https://wowpedia.fandom.com/wiki/BagID

-2 is the KEYRING_CONTAINER

Keyring was removed when raid dungeons became live.

kanodiry commented 3 months ago

Well, after you said that the bag with index -2 is a keyring, I decided to also look for a solution.

I made one change in ../BagSync/wireframe/events.lua

function Events:BAG_UPDATE_DELAYED(event)
    --[[    // *** //   ]]

        if Scanner:IsBackpack(bagid) or Scanner:IsBackpackBag(bagid) or Scanner:IsKeyring(bagid) then
            bagname = "bag"
        elseif Scanner:IsBank(bagid) or Scanner:IsBankBag(bagid) then
            --only do this while we are at a bank
            if Unit.atBank then
                bagname = "bank"
            end
        end

    --[[    // *** //   ]]
end

I removed or Scanner:IsKeyring(bagid).

And It seems work well now. Tell me if this appropriate fix, or this could break something else.

Xruptor commented 3 months ago

Well, after you said that the bag with index -2 is a keyring, I decided to also look for a solution.

I made one change in ../BagSync/wireframe/events.lua

function Events:BAG_UPDATE_DELAYED(event)
  --[[    // *** //   ]]

      if Scanner:IsBackpack(bagid) or Scanner:IsBackpackBag(bagid) or Scanner:IsKeyring(bagid) then
          bagname = "bag"
      elseif Scanner:IsBank(bagid) or Scanner:IsBankBag(bagid) then
          --only do this while we are at a bank
          if Unit.atBank then
              bagname = "bank"
          end
      end

  --[[    // *** //   ]]
end

I removed or Scanner:IsKeyring(bagid).

And It seems work well now. Tell me if this appropriate fix, or this could break something else.

Technically yes that would fix it. However, it's not a 100% fix as all you are doing is removing the scanning of the Keyring. The question we should be asking is WHY the equipment is being scanned as part of the keyring bag? Unless something is broken on their servers or Blizzard changed something, the equipment should not be scanned as part of the keyring bag. So there is something else going on that needs to be looked into.

Xruptor commented 3 months ago

The bag in question that is saving the equipment, should be the Keyring bag. I'm not entirely sure how the equipment is being stored there and it could possibly be a bug on the server itself. I'll review the code but here is the bag numbers in code. https://wowpedia.fandom.com/wiki/BagID -2 is the KEYRING_CONTAINER

Keyring was removed when raid dungeons became live.

Yes it was removed but the underlying code is still there and the keyring bag itself is actually still used to store some quest keys and a few other in game tokens they use. You just can't see it. In addition remember that BagSync is backwards compatible so it has to work with the previous expansions from Classic all the way to Retail.

kanodiry commented 3 months ago

The question we should be asking is WHY the equipment is being scanned as part of the keyring bag? Unless something is broken on their servers or Blizzard changed something, the equipment should not be scanned as part of the keyring bag. So there is something else going on that needs to be looked into.

My guess they're forgot to change GetContainerNumSlots(-2) output to 0.

From link you provided I got this:

The Keyring was removed in 4.2.0 but the constant still exists. On retail this "bag" holds the InventorySlotIds instead.

Also I got this:

In 2.0.3, the Key Ring(-2) always returns 32. The size of the bag displayed is determined by the amount of space used in the keyring.

I called C_Container.GetContainerNumSlots(-2) function on Retail and Cataclysm, and I got: Retail = 0; Cataclysm = 32;

Yes it was removed but the underlying code is still there and the keyring bag itself is actually still used to store some quest keys and a few other in game tokens they use. You just can't see it. In addition remember that BagSync is backwards compatible so it has to work with the previous expansions from Classic all the way to Retail.

If it's hidden, so items stored in it are inaccessible, do we really need to sync this?

I think we should wait for them to fix it.

Xruptor commented 3 months ago

I think I just need to do a check for the unlock for the KeyRing for classic and such. Try this version of BagSync and let me know if it works. I added checks for HasKey() function which should return false for Cataclysm and Retail.

BagSync.zip

kanodiry commented 3 months ago

It works fine.

Xruptor commented 3 months ago

It works fine.

So no duplicate issues or reporting errors? It works fine on Retail and Cataclysm?

Xruptor commented 3 months ago

I have added some additional checks and cleanup to the keyring processing.

Please try this version of BagSync with Classic, Cataclysm, and Retail. Let me know if it works properly.

BagSync.zip

kanodiry commented 3 months ago

I have added some additional checks and cleanup to the keyring processing.

Please try this version of BagSync with Classic, Cataclysm, and Retail. Let me know if it works properly.

Well I've tried all versions, and it seems everything works: No duplications on Retail and Cata and synced keys in keyring on Classic.

Xruptor commented 3 months ago

I have added some additional checks and cleanup to the keyring processing. Please try this version of BagSync with Classic, Cataclysm, and Retail. Let me know if it works properly.

Well I've tried all versions, and it seems everything works: No duplications on Retail and Cata and synced keys in keyring on Classic.

Thanks for testing! I'll make sure to push the changes today.