Outdated findBlock usage in Farmer.js example

TheDudeFromCI commented 4 years ago

[x] The FAQ doesn't contain a resolution to my issue

Versions

mineflayer: 2.28.0
server: vanilla/spigot/paper N/A
node: N/A

Detailed description of a problem

The current farmer.js example uses the older implementation of findBlock to locate a suitable farm block to edit. This requires checking each block individually for specific conditions to be met, such as seeds above farmland. Since findBlock has been optimized to use block pallets in chunks, this function sometimes fails to find the required blocks to interact with. This should probably be tweaked by swapping out findBlock for a standard block iterator function, instead.

Your current code

N/A

Expected behavior

Available farmland and seeds within range can be found 100% of the time, regardless of block layout within the chunk.

Additional context

Another approach could be to add an optimizeSearch flag to the findBlock options. This would default to true. If set to false, it would iterate over blocks directly instead of block pallets.

Karang commented 4 years ago

1) Find nearest farmland blocks 2) Iterate over the list to check block above

TheDudeFromCI commented 4 years ago

Yeah, that's actually a much better idea xD I'll make a PR tomorrow.

Honestly, the more I think about it, the matcher being a function doesn't make much sense, though. In all cases where the optimization works as intended, the user is looking for a specific block type. Which is already handled by passing in the block id. Why use a matcher at all? Backwards compatibility?

Karang commented 4 years ago

Someone reported a similar issue on discord. I proposed this other solution:

matching: (block) => {
  // First check the type
  if (block && block.type === mcData.blocksByName.water.id) {
    // If position is defined, you can refine the search
    if (block.position) {
      const blockAbove = bot.blockAt(block.position.offset(0, 1, 0))
      return blockAbove.type === mcData.blocksByName.air.id
    }
    return true // otherwise return always true (there is water in the section so it should be checked)
  }
  return false
}

short explanation: findBlocks use matching internally for 2 reasons:

check if a given block should be return (in this case position is defined)
check if the block is in the palette (position is not defined)

For the 2nd case, you cannot use position, but you should return true if it match the other criteria, otherwise the section gets ignored

TheDudeFromCI commented 4 years ago

While you could use it that way, it seems a bit more confusing. It's not very intuitive to most new users how that work would. I propose two solutions:

1) We could separate the matcher function from the matcher IDs. The IDs are tested against the palette first. If they pass, the function is used for blocks within the chunk directly.

2) Check the matcher type. If using IDs, continue to use the optimizations and everything like we do now. If the matcher is a function, don't use block palettes. Just use a standard block iterator.

PrismarineJS / mineflayer