Roeshambo / MonolithDKP

DKP Interface for Monolith (WoW Classic Guild) (Development Repository)
28 stars 71 forks source link

Cancel bid does not cancel all bids but some weird subset of them #307

Open f90 opened 4 years ago

f90 commented 4 years ago

When under-cutting is allowed and multiple bids are made by one person, and the person then uses !bid cancel, instead of deleting ALL bids from the list of bids shown to the PM, it deletes SOME bids while not deleting others, without any apparent logic.

Roeshambo commented 4 years ago

There should only be 1 bid up for a person at any given time. If they bid higher, it removes their last bid and adds the new one. Canceling a bid just cancels that individuals bid.

f90 commented 4 years ago

Ah makes sense. But then the bug is that I can still see all the other bids made by the same person that are lower than his/her current one, they should be deleted right away!

lantisnt commented 4 years ago

I can confirm Roe that's true, we also see all bids and its impossible to say which is the newest

Roeshambo commented 4 years ago

That's unusual. When a new bid is received the lower bid should be immediately removed from the bid table. I'll check why it might not be.

lantisnt commented 4 years ago

@Roeshambo We have also noticed that not always bids are actually removed when bid >= than the new incoming. I think this is the faulty line: Bidding.lua:163

if (mode ~= "Zero Sum" and MonDKP_DB.modes.ZeroSumBidType ~= "Static") and Bids_Submitted[i] and Bids_Submitted[i].player == name and Bids_Submitted[i].bid < cmd then

The first conditional actually requires both to be true, so if you have mode other than ZeroSum but ZeroSumBidType is still Static (probably value not cleared when changing from zerosum static to min bid value, etc.) then this will not execute. Excerpt from my Variable:

    ["modes"] = {
        ["SameZoneOnly"] = false,
        ["ZeroSumBidType"] = "Static",
                ...
        ["mode"] = "Minimum Bid Values",

Other thing is that in lines 134-136 you iterate over

Bids_Submitted
          if Bids_Submitted[i] and Bids_Submitted[i].player == name then
            table.remove(Bids_Submitted, i)
            if MonDKP_DB.modes.BroadcastBids then

AND at the same time you are editing the bids_submitted table. This is actually undefined behavior (not safe) in LUA language. this can be issue for @f90 . This is because you change table size while iterating over it. I would suggest doing a copy of the table, while iterating, but with only values not from the player and then substitute. e.g. change:

        for i=1, #Bids_Submitted do           -- !bid cancel will cancel their bid
          if Bids_Submitted[i] and Bids_Submitted[i].player == name then
            table.remove(Bids_Submitted, i)

to

Bids_SubmittedNew = {}
        for i=1, #Bids_Submitted do           -- !bid cancel will cancel their bid
          if Bids_Submitted[i] and Bids_Submitted[i].player ~= name then
            table.insert(Bids_SubmittedNew, Bids_Submitted[i])
...
Bids_Submitted = Bids_SubmittedNew

You can also use dual iteration https://forums.coronalabs.com/topic/11057-how-can-i-delete-objects-from-a-table-while-iterating-through-it/

lantisnt commented 4 years ago

Fixed in https://github.com/Roeshambo/MonolithDKP/pull/338

f90 commented 4 years ago

Closing this for now assuming pull request #338 fixed it, can only verify once it's on live though

f90 commented 4 years ago

Actually noticed that the faulty code (iterating over table while deleting its entries) is still not changed, @lantisnt you suggested a code change, do you want to implement that as well?