brndd / vanilla-tweaks

Custom QoL patches to the 1.12.1 WoW client
MIT License
152 stars 24 forks source link

Quickloot doesn't work for chests #2

Closed burner1024 closed 2 years ago

burner1024 commented 2 years ago

Looted a few, not once it worked. (Maybe allow to choose between old and new patching methods, if the old one always worked)

brndd commented 2 years ago

In my experience it tends to only occasionally fail and then work on the second attempt. I haven't been able to pin down a consistent repro or figure out a cause for it beyond some internal state machine probably getting messed up.

I think for the next release I will revert back to the old method and add an optional flag for this new, experimental method for people who want to use it (presumably mostly rogue players).


Implementation musings below:

The old method works by modifying event handlers for OnRightClickUnit (0x60bea0; this gets called for corpse looting), OnRightClickObject (0x5f8660; this gets called for looting chests etc.) and a function I called open_container_item (0x5edc80) which gets called for lockboxes and other items in the player's inventory.

I didn't find an event handler like this for pickpocketing, probably because I found these by following calls to the autoloot function and pickpocketing currently never calls it under any circumstances. I'm sure there is a handler for pickpocketing (and disenchanting too) somewhere, but even after it's found patching it would involve adding some extra code to call the autoloot function, so it would be a nontrivial patch and could be impractical with static modifications of the executable. Would probably have to add a new memory region into the executable to hold the code. Live patching (i.e. injection) would be easier and is an avenue I may explore in the future, e.g. by building on Namreeb's wowreeb launcher.

edit: also, one of the reasons the state machine probably gets messed up is that the function we're patching currently gets called twice each time the player loots anything.

brndd commented 2 years ago

I changed the default to the old behaviour in v1.4.0 and added a flag (--alternative-quickloot) to use the less reliable behaviour, because it's still the only thing that works with pickpocketing. I'm leaving this issue open because I'd like to find a best-of-both-worlds solution down the line.

burner1024 commented 2 years ago

I'm afraid even the old method still doesn't work for insignia looting (battlegrounds). At least, on Turtle. edit: and skinning too

brndd commented 2 years ago

Yeah, that was my mistake. Looks like I forgot to patch skinning for that method.

Anyway, I poked around a bit and think I managed to fix the less reliable behaviour and make it work reliably. It seems there was some special logic that got run every time you opened a chest for the first time, and that branch needed to have the quickloot logic reversed as well.

I've made the more universal method the default again for v1.5.0. I tested around a bunch on a local server and couldn't find any edge cases for any type of looting I could think of (corpses, chests, mining, fishing, skinning, disenchanting, inventory containers at least). Didn't test insignias though, because those are kind of a pain to test on a local server alone. Please let me know if anything is still broken.