Fix several bugs in villager trading (#3230)

* Fix several bugs in villager trading

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.

* Add more tests for villager trading

- 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

---------

Co-authored-by: Romain Beaumont <romain.rom1@gmail.com>

Author: evan-goode

lib/plugins/inventory.js

@@ -407,15 +407,20 @@ function inject (bot, { hideErrors }) {
   }
 
   function tradeMatch (limitItem, targetItem) {
-    return targetItem.type === limitItem.type && targetItem.count >= limitItem.count
+    return (
+      targetItem !== null &&
+      limitItem !== null &&
+      targetItem.type === limitItem.type &&
+      targetItem.count >= limitItem.count
+    )
   }
 
   function expectTradeUpdate (window) {
     const trade = window.selectedTrade
     const hasItem = !!window.slots[2]
 
     if (hasItem !== tradeMatch(trade.inputItem1, window.slots[0])) {
-      if (trade.hasItems2) {
+      if (trade.hasItem2) {
         return hasItem !== tradeMatch(trade.inputItem2, window.slots[1])
       }
       return true
@@ -434,7 +439,7 @@ function inject (bot, { hideErrors }) {
       }
     } else if (window.type === 'minecraft:merchant') {
       const toUpdate = []
-      if (slot <= 2 && !window.selectedTrade.tradeDisabled && expectTradeUpdate(window)) {
+      if (slot <= 1 && !window.selectedTrade.tradeDisabled && expectTradeUpdate(window)) {
         toUpdate.push(once(bot.currentWindow, 'updateSlot:2'))
       }
       if (slot === 2) {
@@ -443,6 +448,12 @@ function inject (bot, { hideErrors }) {
         }
       }
       await Promise.all(toUpdate)
+
+      if (slot === 2 && !window.selectedTrade.tradeDisabled && expectTradeUpdate(window)) {
+        // After the trade goes through, if the inputs are still satisfied,
+        // expect another update in slot 2
+        await once(bot.currentWindow, 'updateSlot:2')
+      }
     }
   }
 

lib/plugins/villager.js

@@ -141,8 +141,11 @@ function inject (bot, { version }) {
       itemCount2 = villager.count(Trade.inputItem2.type, Trade.inputItem2.metadata)
       hasEnoughItem2 = itemCount2 >= Trade.inputItem2.count * count
     }
-    if (!hasEnoughItem1 || !hasEnoughItem2) {
-      throw new Error('Not enough items to trade')
+    if (!hasEnoughItem1) {
+      throw new Error('Not enough item 1 to trade')
+    }
+    if (!hasEnoughItem2) {
+      throw new Error('Not enough item 2 to trade')
     }
 
     selectTrade(choice)
@@ -152,31 +155,12 @@ function inject (bot, { version }) {
       if (Trade.hasItem2) proms.push(once(villager, 'updateSlot:1'))
       if (bot.supportFeature('setSlotAsTransaction')) {
         proms.push(once(villager, 'updateSlot:2'))
-        await new Promise((resolve, reject) => {
-          let countOfItemOneLeftToTake = itemCount1 > 64 ? 64 : itemCount1
-          let countOfItemTwoLeftToTake = 0
-          if (Trade.hasItem2) {
-            countOfItemTwoLeftToTake = itemCount2 > 64 ? 64 : itemCount2
-          }
-          const listener = (slot, oldItem, newItem) => {
-            if (!(slot >= villager.inventoryStart && slot <= villager.inventoryEnd)) return
-            if (newItem === null) {
-              if (oldItem.type === Trade.inputItem1.type) countOfItemOneLeftToTake -= oldItem.count
-              else if (Trade.hasItem2 && oldItem.type === Trade.inputItem2.type) countOfItemTwoLeftToTake -= oldItem.count
-            }
-            if (countOfItemOneLeftToTake === 0 && countOfItemTwoLeftToTake === 0) {
-              villager.off('updateSlot', listener)
-              resolve()
-            }
-          }
-          villager.on('updateSlot', listener)
-        })
       }
       await Promise.all(proms)
     }
 
     for (let i = 0; i < count; i++) {
-      await putRequirements(villager, Trade, count)
+      await putRequirements(villager, Trade)
       // ToDo: See if this does anything kappa
       Trade.nbTradeUses++
       if (Trade.maximumNbTradeUses - Trade.nbTradeUses === 0) {
@@ -215,24 +199,25 @@ function inject (bot, { version }) {
     }
   }
 
-  async function putRequirements (window, Trade, count) {
+  async function putRequirements (window, Trade) {
     const [slot1, slot2] = window.slots
-    const { stackSize: stackSize1, type: type1, metadata: metadata1 } = Trade.inputItem1
-    const tradeCount1 = Trade.realPrice
-    const neededCount1 = Math.min(stackSize1, tradeCount1 * count)
-
-    const input1 = !slot1
-      ? neededCount1
-      : (slot1.count < tradeCount1 ? neededCount1 - slot1.count : 0)
-    await deposit(window, type1, metadata1, input1, 0)
+    const { type: type1, metadata: metadata1 } = Trade.inputItem1
+
+    const input1 = slot1
+      ? Math.max(0, Trade.realPrice - slot1.count)
+      : Trade.realPrice
+    if (input1) {
+      await deposit(window, type1, metadata1, input1, 0)
+    }
     if (Trade.hasItem2) {
-      const { count: tradeCount2, stackSize: stackSize2, type: type2, metadata: metadata2 } = Trade.inputItem2
-      const needCount2 = Math.min(stackSize2, tradeCount2 * count)
+      const { count: tradeCount2, type: type2, metadata: metadata2 } = Trade.inputItem2
 
-      const input2 = !slot2
-        ? needCount2
-        : (slot2.count < tradeCount2 ? needCount2 - slot2.count : 0)
-      await deposit(window, type2, metadata2, input2, 1)
+      const input2 = slot2
+        ? Math.max(0, tradeCount2 - slot2.count)
+        : tradeCount2
+      if (input2) {
+        await deposit(window, type2, metadata2, input2, 1)
+      }
     }
   }
 

test/externalTests/trade.js

@@ -6,14 +6,20 @@ module.exports = () => async (bot) => {
 
   const villagerType = bot.registry.entitiesByName.villager ? 'villager' : 'Villager'
   const testFluctuations = bot.supportFeature('selectingTradeMovesItems')
+
   const summonCommand = bot.supportFeature('indexesVillagerRecipes')
-    ? `/summon ${villagerType} ~ ~1 ~ {NoAI:1, Offers:{Recipes:[0:{maxUses:7,buy:{id:"minecraft:emerald",Count:2},sell:{id:"minecraft:pumpkin_pie",Count:2},uses: 1},1:{maxUses:7,buy:{id:"minecraft:emerald",Count:2},buyB:{id:"minecraft:pumpkin_pie",Count:2},sell:{id:"minecraft:wheat",Count:2}, uses:1}]}}`
-    : `/summon ${villagerType} ~ ~1 ~ {NoAI:1, Offers:{Recipes:[{maxUses:7,buy:{id:"minecraft:emerald",Count:2},sell:{id:"minecraft:pumpkin_pie",Count:2},${testFluctuations ? 'demand:60,priceMultiplier:0.05f,specialPrice:-4,' : ''}uses: 1},{maxUses:7,buy:{id:"minecraft:emerald",Count:2},buyB:{id:"minecraft:pumpkin_pie",Count:2},sell:{id:"minecraft:wheat",Count:2}, uses:1}]}}`
+    ? `/summon ${villagerType} ~ ~1 ~ {NoAI:1, Offers:{Recipes:[0:{maxUses:12,buy:{id:"minecraft:emerald",Count:2},sell:{id:"minecraft:pumpkin_pie",Count:2},uses: 1},1:{maxUses:12,buy:{id:"minecraft:emerald",Count:2},buyB:{id:"minecraft:pumpkin_pie",Count:2},sell:{id:"minecraft:wheat",Count:2}, uses:1},2:{maxUses:12,buy:{id:"minecraft:emerald",Count:1},sell:{id:"minecraft:glass",Count:4},uses: 1},3:{maxUses:12,buy:{id:"minecraft:emerald",Count:36},buyB:{id:"minecraft:book",Count:1},sell:{id:"minecraft:wooden_sword",Count:1},uses: 1}]}}`
+    : `/summon ${villagerType} ~ ~1 ~ {NoAI:1, Offers:{Recipes:[{maxUses:12,buy:{id:"minecraft:emerald",Count:2},sell:{id:"minecraft:pumpkin_pie",Count:2},${testFluctuations ? 'demand:60,priceMultiplier:0.05f,specialPrice:-4,' : ''}uses: 1},{maxUses:12,buy:{id:"minecraft:emerald",Count:2},buyB:{id:"minecraft:pumpkin_pie",Count:2},sell:{id:"minecraft:wheat",Count:2}, uses:1},{maxUses:12,buy:{id:"minecraft:emerald",Count:1},sell:{id:"minecraft:glass",Count:4},uses: 1},{maxUses:12,buy:{id:"minecraft:emerald",Count:36},buyB:{id:"minecraft:book",Count:1},sell:{id:"minecraft:wooden_sword",Count:1},uses: 1}]}}`
 
   const commandBlockPos = bot.entity.position.offset(0.5, 0, 0.5)
   const redstoneBlockPos = commandBlockPos.offset(1, 0, 0)
 
-  await bot.test.setInventorySlot(36, new Item(bot.registry.itemsByName.emerald.id, 64, 0))
+  let shouldHaveEmeralds = 0
+  for (let slot = 9; slot <= 17; slot += 1) {
+    await bot.test.setInventorySlot(slot, new Item(bot.registry.itemsByName.emerald.id, 64, 0))
+    shouldHaveEmeralds += 64
+  }
+  await bot.test.setInventorySlot(18, new Item(bot.registry.itemsByName.book.id, 11, 0))
 
   // A command block is needed to spawn the villager due to the chat's character limit in some versions
   bot.test.sayEverywhere(`/setblock ${commandBlockPos.toArray().join(' ')} command_block`)
@@ -40,9 +46,10 @@ module.exports = () => async (bot) => {
     assert.strictEqual(output.name, 'pumpkin_pie')
     assert.strictEqual(output.count, 2)
 
-    await bot.trade(villager, 0, 6)
-    assert.strictEqual(bot.currentWindow.count(bot.registry.itemsByName.emerald.id), testFluctuations ? 64 - 24 : 64 - 12)
-    assert.strictEqual(bot.currentWindow.count(bot.registry.itemsByName.pumpkin_pie.id), 12)
+    await bot.trade(villager, 0, 11)
+    shouldHaveEmeralds -= testFluctuations ? (2 * 2 * 11) : (2 * 11)
+    assert.strictEqual(bot.currentWindow.count(bot.registry.itemsByName.emerald.id), shouldHaveEmeralds)
+    assert.strictEqual(bot.currentWindow.count(bot.registry.itemsByName.pumpkin_pie.id), 22)
   }
 
   // Handle trade #2 -- takes [2x emerald, 2x pumpkin_pie] and returns 2x wheat
@@ -61,15 +68,59 @@ module.exports = () => async (bot) => {
     assert.strictEqual(output.name, 'wheat')
     assert.strictEqual(output.count, 2)
 
-    await bot.trade(villager, 1, 6)
-    assert.strictEqual(bot.currentWindow.count(bot.registry.itemsByName.emerald.id), testFluctuations ? 64 - 36 : 64 - 24)
+    await bot.trade(villager, 1, 11)
+    shouldHaveEmeralds -= 11 * 2
+    assert.strictEqual(bot.currentWindow.count(bot.registry.itemsByName.emerald.id), shouldHaveEmeralds)
     assert.strictEqual(bot.currentWindow.count(bot.registry.itemsByName.pumpkin_pie.id), 0)
-    assert.strictEqual(bot.currentWindow.count(bot.registry.itemsByName.wheat.id), 12)
+    assert.strictEqual(bot.currentWindow.count(bot.registry.itemsByName.wheat.id), 22)
+  }
+
+  // Handle trade #3 -- takes 1x emerald and returns 4x glass
+  {
+    const trade = villager.trades[2]
+    assert.strictEqual(trade.inputs.length, 1, 'Expected single input from villager on first trade')
+    verifyTrade(trade)
+
+    const [input] = trade.inputs
+    assert.strictEqual(input.name, 'emerald')
+    assert.strictEqual(input.count, 1)
+
+    const [output] = trade.outputs
+    assert.strictEqual(output.name, 'glass')
+    assert.strictEqual(output.count, 4)
+
+    await bot.trade(villager, 2, 11)
+    shouldHaveEmeralds -= 11
+    assert.strictEqual(bot.currentWindow.count(bot.registry.itemsByName.emerald.id), shouldHaveEmeralds)
+    assert.strictEqual(bot.currentWindow.count(bot.registry.itemsByName.glass.id), 44)
+  }
+
+  // Handle trade #4 -- takes [36x emerald, 1x book] and returns 1x wooden sword
+  {
+    const trade = villager.trades[3]
+    assert.strictEqual(trade.inputs.length, 2, 'Expected two inputs from villager on second trade')
+    verifyTrade(trade)
+
+    const [input1, input2] = trade.inputs
+    assert.strictEqual(input1.name, 'emerald')
+    assert.strictEqual(input1.count, 36)
+    assert.strictEqual(input2.name, 'book')
+    assert.strictEqual(input2.count, 1)
+
+    const [output] = trade.outputs
+    assert.strictEqual(output.name, 'wooden_sword')
+    assert.strictEqual(output.count, 1)
+
+    await bot.trade(villager, 3, 11)
+    shouldHaveEmeralds -= 11 * 36
+    assert.strictEqual(bot.currentWindow.count(bot.registry.itemsByName.emerald.id), shouldHaveEmeralds)
+    assert.strictEqual(bot.currentWindow.count(bot.registry.itemsByName.book.id), 0)
+    assert.strictEqual(bot.currentWindow.count(bot.registry.itemsByName.wooden_sword.id), 11)
   }
 
   function verifyTrade (trade) {
     assert.strictEqual(trade.nbTradeUses, 1)
-    assert.strictEqual(trade.maximumNbTradeUses, 7)
+    assert.strictEqual(trade.maximumNbTradeUses, 12)
     assert.strictEqual(trade.tradeDisabled, false)
 
     const printCountInv = function (item) {