Repository navigation
Conversation
rom1504
left a comment
There was a problem hiding this comment.
Astra agent review — AI-generated, not manually written by the maintainer.
Rechecked 0ff458e and the existing discussion.
The fallback is not ready to merge: a wrong tool now receives its generic mining multiplier on hand-harvestable blocks. The inline oak-log case was reproduced with the actual model and registry earlier today, including hand and correct-tool controls. Prefer consolidating with #127's block/tool applicability path instead of maintaining another inference from tool names.
Skills used: prismarine-geometry-movement-review checked actual block/tool and coordinate behavior; prismarine-architecture-review checked ownership and public compatibility; prismarine-review checked current scope and existing feedback to avoid duplicate threads.
| const itemNameParts = item?.name.split('_') | ||
| const toolType = itemNameParts?.[itemNameParts.length - 1] | ||
| const mineableToolMultipliers = registry.materials[`mineable/${toolType}`] | ||
| if (mineableToolMultipliers?.[heldItemType]) { |
There was a problem hiding this comment.
Astra agent review — AI-generated, not manually written by the maintainer.
canHarvest does not establish that this tool is effective on this block. For hand-harvestable oak logs it returns true for a diamond pickaxe, so this branch adopts the pickaxe's generic speed. With the actual 1.21.4 registry and this candidate, oak_log.digTime is 3000 ms by hand, 400 ms with a diamond pickaxe, and 400 ms with a diamond axe. The pickaxe should not receive the axe's effective-tool speed. Check the block's applicable tool/material/tag, rather than selecting solely from the held tool name; #127 already implements the relevant tag-based ownership.
Skills used: prismarine-geometry-movement-review checked actual block/tool and coordinate behavior; prismarine-protocol-data-review checked the actual representation and version/dependency boundary;
There was a problem hiding this comment.
Addressed in b677494. The fallback now requires the block's explicit harvestTools to identify a matching mineable/* tool family; it no longer infers applicability from canHarvest or the tool name.
I reran npm test today: Standard lint and all 8,233 tests pass. With minecraft-data 3.117.0 / prismarine-registry 1.12.0 and the actual 1.21.4 registry, oak logs take 3,000 ms by hand, 3,000 ms with a diamond pickaxe, and 400 ms with a diamond axe. The regression suite also covers ineffective enchanted tools, insufficient tiers, and missing applicability data across 1.21.1, 1.21.4, and 1.21.11.
Regarding #127: its tag-based path is broader and leaves the existing material path unchanged when tags are unavailable. The released registry used above does not expose registry.tags, so this PR fixes that fallback case. I'm happy for this fallback and its tests to be folded into #127 if that is the preferred merge path. Could you re-review the updated patch with that distinction in mind?
Fixes #69
Blocks whose material is a tier restriction (such as
incorrect_for_wooden_tool) can lose the mining multipliers for effective tools. Resolve missing or hand-speed entries through the existingmineable/*material tables only when the block’s explicitharvestToolsidentifies the same tool family. No tool-name parsing is needed.This keeps a diamond pickaxe on oak logs at 3,000 ms and a diamond axe at 400 ms. Copper ore and gold ore receive their pickaxe speeds, including insufficient-tier pickaxes;
canHarvest()still applies the harvest penalty. Existing effective material multipliers remain authoritative, and blocks without explicit tool applicability keep their material behavior.The fallback works with released minecraft-data 3.117.0, which does not expose block tags. #127’s broader tag-based path remains complementary and depends on tag data becoming available.
Regression coverage includes oak logs, dirt, copper ore, smooth basalt, gold ore, ineffective enchanted tools, insufficient tiers, and missing applicability data across 1.21.1, 1.21.4, and 1.21.11.
Validation:
npm test(Standard lint and full Mocha suite) on Node 24.4.1; 8,233 tests pass after merging current master. No live Minecraft server validation was performed.