Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix several bugs in villager trading #3230

Merged
merged 3 commits into from
Dec 30, 2023
Merged

Fix several bugs in villager trading #3230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
de819c2
Fix several bugs in villager trading
evan-goode Nov 11, 2023
b1abd24
Add more tests for villager trading
evan-goode Dec 21, 2023
77b4fe9
Merge branch 'master' into evan-goode/fix-trading
rom1504 Dec 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions lib/plugins/inventory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,15 +391,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
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a typo, the field is called hasItem2.

if (trade.hasItem2) {
return hasItem !== tradeMatch(trade.inputItem2, window.slots[1])
}
return true
Expand All @@ -418,7 +423,7 @@ function inject (bot, { hideErrors }) {
}
} else if (window.type === 'minecraft:merchant') {
const toUpdate = []
if (slot <= 2 && !window.selectedTrade.tradeDisabled && expectTradeUpdate(window)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (slot <= 1 && !window.selectedTrade.tradeDisabled && expectTradeUpdate(window)) {
toUpdate.push(once(bot.currentWindow, 'updateSlot:2'))
}
if (slot === 2) {
Expand All @@ -427,6 +432,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')
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

Expand Down
59 changes: 22 additions & 37 deletions lib/plugins/villager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Log a bit more information here. I'm happy to leave this out if it amounts to an unnecessary breaking change.

}

selectTrade(choice)
Expand All @@ -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)
})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}
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) {
Expand Down Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only put the requirements for a single trade at a time. This seems to greatly increase reliability, especially for trades with multiple inputs.

if (input1) {
await deposit(window, type1, metadata1, input1, 0)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only call deposit if we actually need to move items

}
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)
}
}
}

Expand Down
71 changes: 61 additions & 10 deletions test/externalTests/trade.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`)
Expand All @@ -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
Expand All @@ -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) {
Expand Down
Loading