Fix several bugs in villager trading #3230
Conversation
These changes should fix issues with trades that have more than one item in the output stack, trades that cost more than one emerald, and trades with two input items, such as trades for enchanted books and tipped arrows.
| limitItem !== null && | ||
| targetItem.type === limitItem.type && | ||
| targetItem.count >= limitItem.count | ||
| ) |
There was a problem hiding this comment.
Sometimes, tradeMatch is called with a null targetItem. If there is nothing in the targetItem slot, then the trade requirements cannot be satisfied, so return false in that case.
| } | ||
|
|
||
| function expectTradeUpdate (window) { | ||
| const trade = window.selectedTrade | ||
| const hasItem = !!window.slots[2] | ||
|
|
||
| if (hasItem !== tradeMatch(trade.inputItem1, window.slots[0])) { | ||
| if (trade.hasItems2) { |
There was a problem hiding this comment.
This was a typo, the field is called hasItem2.
| @@ -418,7 +423,7 @@ function inject (bot, { hideErrors }) { | |||
| } | |||
| } else if (window.type === 'minecraft:merchant') { | |||
| const toUpdate = [] | |||
| if (slot <= 2 && !window.selectedTrade.tradeDisabled && expectTradeUpdate(window)) { | |||
There was a problem hiding this comment.
The updateSlot:2 event is already awaited by promisePutAway in putAway(), so if we await it here too, we'll never get it. I think waitForWindowUpdate should only wait for updates on slots other than the slot it was called on.
| // After the trade goes through, if the inputs are still satisfied, | ||
| // expect another update in slot 2 | ||
| await once(bot.currentWindow, 'updateSlot:2') | ||
| } |
There was a problem hiding this comment.
In the trade loop, if the requirements of a trade are still satisfied after putAway but the window doesn't have an item in slot 2, then putRequirements will not deposit anything, an update in slot 2 will not be awaited, and putAway will be immediately called again without anything in slot 2. So here, we should only consider the trading window "fully updated" if the state of slot 2 correctly reflects whether the trade inputs are satisfied.
| throw new Error('Not enough item 1 to trade') | ||
| } | ||
| if (!hasEnoughItem2) { | ||
| throw new Error('Not enough item 2 to trade') |
There was a problem hiding this comment.
Log a bit more information here. I'm happy to leave this out if it amounts to an unnecessary breaking change.
| } | ||
| } | ||
| villager.on('updateSlot', listener) | ||
| }) |
There was a problem hiding this comment.
The bot consistently gets hung up here, and I honestly can't figure out what the purpose of this is. It's possible that it was necessary before some of the other changes I made. @nickelpro do you remember by chance?
|
|
||
| const input1 = slot1 | ||
| ? Math.max(0, Trade.realPrice - slot1.count) | ||
| : Trade.realPrice |
There was a problem hiding this comment.
Only put the requirements for a single trade at a time. This seems to greatly increase reliability, especially for trades with multiple inputs.
| ? Math.max(0, Trade.realPrice - slot1.count) | ||
| : Trade.realPrice | ||
| if (input1) { | ||
| await deposit(window, type1, metadata1, input1, 0) |
There was a problem hiding this comment.
Only call deposit if we actually need to move items
|
Could someone with permissions perhaps re-run https://github.com/PrismarineJS/mineflayer/actions/runs/6843109062/job/18605343146?pr=3230? The tests pass locally for me, this seems like a transient error unrelated to the changes. |
|
can you add a test to make sure what you fixed will keep working in the future ? |
- Add a test trade taking 1 emerald and returning 4 glass - Add a test trade taking [36 emeralds, 1 book] and returning 1 wooden sword (any unstackable should work to accurately simulate an enchanted_book trade) - Increase maximumNbTradeUses to 12 for all test trades
evan-goode
force-pushed
the
evan-goode/fix-trading
branch
from
ce8702d to
b1abd24
Compare
Sure, I added a couple tests in |
These changes should fix issues with trades that have more than one item in the output stack, trades that cost more than one emerald, and trades with two input items, such as trades for enchanted books and tipped arrows.
Resolves #3209 and probably #2681.
I've been developing a bot to automate void trading, and while the current villager plugin works fine for trades of 1 emerald to 1 item, I encountered many issues with trades of 1 emerald to multiple items, multiple emeralds to one item, and trades with a second input item. After these changes, all trades I've tested work correctly.
I play on a vanilla 1.20.1 server.
I'll add comments on this PR detailing the reason for each change.