feat: Implement ItemStack request processing system and inventory improvements#704
Open
lt-name wants to merge 41 commits into
Open
feat: Implement ItemStack request processing system and inventory improvements#704lt-name wants to merge 41 commits into
lt-name wants to merge 41 commits into
Conversation
…rovements - Add ItemStack request action processors (Craft, Transfer, Consume, etc.) - Implement ItemStackNetManager for network ID allocation - Add BundleInventory and HorseInventory support - Include EnchantmentHelper and TradeRecipeBuildUtils - Add unit tests for BundleInventory and NetworkMapping
Add inventory transaction packet processing on the player path.\nImprove item stack request rollback, container mapping and transfer events.
Contributor
Codex Code ReviewBlockers
High
Medium
Reviewed by Hermes Codex (PR #704) |
tachibana-kanade-angel
added
[Type] New Feature | 新特性
…ion, and fix action types for 1.21.40+
# Conflicts: # src/main/java/cn/nukkit/inventory/BaseInventory.java # src/main/java/cn/nukkit/inventory/PlayerInventory.java # src/main/java/cn/nukkit/item/Item.java
…ons instead of failing
…rden commit atomicity
…unt mismatch CraftCreativeActionProcessor used the client's numberOfRequestedCrafts to size the CREATED_OUTPUT stack, but the client always requests a full stack (maxStackSize) in the follow-up PLACE/DROP action. When the two numbers diverged (e.g. server count=1 vs client request=64), doTransfer's source count check rejected the request, triggering the d1cf689 rollback path and clearing the cursor, appearing as the item briefly held then vanishing. Fix: always materialize the creative output at maxStackSize, matching the Allay/PNX reference implementation and the client's always-full-stack expectation. Also skip the source stackNetworkId validation on the creative CREATED_OUTPUT transfer path: the server-allocated id is never echoed back to the client, so the follow-up action's source id is the client's own prediction and would otherwise never match. Added regression tests covering the full handler path (hotbar, cursor, occupied-slot, client-predicted netId, and the numberOfRequestedCrafts mismatch that reproduced the reported symptom).
…y UI and block-entity holders
# Conflicts: # src/main/java/cn/nukkit/inventory/InventoryType.java
canBePutInOffhandSlot() used a hardcoded numeric-ID whitelist that rejected every custom item (id == 255), even when the mod author called CustomItemDefinition.allowOffHand(true). That setting was only written to the client-bound NBT and never read server-side, so placing a custom item in the off-hand was always rejected in SAI mode. Override canBePutInOffhandSlot() in ItemCustom to read the allow_off_hand property from its CustomItemDefinition, so custom items opt-in via allowOffHand(true) now work in the off-hand slot. Root cause: commit e893b6e introduced the whitelist without a hook for custom items.
…side Custom armor and tools were silently broken because their server-side properties came from Item base defaults (0 armor points, 1 attack damage, tier 0, no equipment slot/tool type) instead of the configured CustomItemDefinition. The root cause was a circular dependency: ArmorBuilder/ToolBuilder.build() called item.isHelmet()/isPickaxe() etc. to decide which NBT to write, but ItemCustomArmor/ItemCustomTool never overrode those methods (all returned false), so no NBT was written, and there was nothing to read back. Break the cycle by making it explicit: - ArmorBuilder.slot(ArmorSlot) and ToolBuilder.toolType(ToolType) chain methods let mod authors declare the slot/type; builders write the NBT, and ItemCustomArmor/ItemCustomTool read it back via overrides of isHelmet/isChestplate/isLeggings/isBoots and isPickaxe/isAxe/isShovel/isHoe/isSword/isShears. - New builder chain methods (attackDamage, maxDurability, armorPoints, toughness, tier) feed the server-side getAttackDamage/getArmorPoints/ getToughness/getTier overrides so custom gear uses real values. - Add CustomItem.getDefinitionNbt() default method that reads the registry snapshot (falling back to getDefinition() when unregistered) as the single NBT source for all ItemCustom* overrides. Also fixes a pre-existing bug in SimpleBuilder.tag(): getList(name, cls) returns a fresh empty ListTag (never null) when the key is missing, so tags were silently dropped on first use. Now checks contains() first. Enchantability is now derived from the configured tier in build() (tierToArmorEnchantAbility/tierToToolEnchantAbility) instead of calling item.getEnchantAbility() during construction, which caused infinite recursion once getTier() started reading the definition NBT. Custom armor can now be equipped and provides protection; custom tools deal configured damage, have durability, receive type-appropriate enchantments, and can be enchanted at an enchant table. Root cause: commit e893b6e introduced server-authoritative slot validation without a hook for custom items.
…lity The previous commit (c8d5a02) introduced server-side overrides on ItemCustomTool/ItemCustomArmor that read from getDefinitionNbt(), but the builder fallbacks still called item.getMaxDurability()/getAttackDamage()/ getTier()/getArmorPoints()/getToughness(). Since the item is unregistered during build(), those overrides fall back to getDefinition() -> build() -> infinite recursion -> StackOverflowError, whenever a mod author omitted any of the chain setters. Fixes: - ToolBuilder.build(): use safe defaults (DURABILITY_WOODEN, 1, 0) instead of item instance methods when chain setters are unset. - ArmorBuilder.build(): same treatment (0 for protection/toughness/tier/ durability), plus a new maxDurability(int) chain method (was missing, causing max_durability to be written as -1). - ItemCustomArmor: add getMaxDurability() override reading minecraft:durability.max_durability (was missing, returned -1 from base, excluding custom armor from anvil repair). - ToolBuilder.speed(int): no longer calls item.isPickaxe() etc. when toolType is unset — that path also recursed. Now unconditionally accepts the speed value. Regression tests added: minimal-config custom armor and tool (only slot/toolType set) verify build() completes without StackOverflowError and unset properties return safe defaults.
…vior The builder chain-method javadocs claimed that unset properties fall back to item instance methods (e.g. getAttackDamage/getArmorPoints), but the build() methods actually use hardcoded safe defaults to avoid the getDefinitionNbt() recursion. Correct the docs to describe the real defaults. Critically, slot(ArmorSlot) no longer claims a fallback exists — when unset, no slot is written and the custom armor is unequippable. This is now explicitly documented as a warning, since a mod author relying on the non-existent fallback would ship broken armor.
…ability Two related fixes to CustomItemDefinition tool/armor builders: 1. digger component write-back (CustomItemDefinition): Previously minecraft:digger was written back immediately inside the toolType branch, so SWORD/HOE (no blockTags) and tools that only call addExtraBlock never got digger written back. This made server-side getSpeed() return null (mining speed lost) and the client never received destroy speeds. Now destroy_speeds entries are collected first and digger is written back only when non-empty, so tagless tools and isolated addExtraBlock tools still emit digger. (putCompound stores by reference, so entries appended to the list before write-back take effect.) 2. Armor default durability: maxDurability defaulted to 0 when unset, which destroyed the armor on the first hit in EntityHumanType#damageArmor. Default it to DURABILITY_DEFAULT (56, the lowest vanilla value - leather helmet) as a safe lower bound. Items that should be unbreakable must set a large durability or use Item#setUnbreakable(). Added regression tests covering both fixes: - sword/hoe/addExtraBlock write digger with correct destroy_speeds - shears with no blocks writes no digger - armor unset durability resolves to DURABILITY_DEFAULT
getSpeed() returned destroy_speeds[0] regardless of the mined block, so every block used one speed. Replace it with getSpeedFor(Block) which looks up the matching entry per blockId, and extend correctTool0 so digger-listed blocks count as correct tools. Fixes the sword/cobweb regression (15 not 6) and makes addExtraBlock per-block speeds apply. - ItemCustomTool: add getSpeedFor(Block/int) with lazy blockId cache; deprecate getSpeed() - Block: correctTool0 extension + toolBreakTimeBonus0 uses getSpeedFor; drop redundant customToolBreakTimeBonus/customToolType - CustomItemDefinition: align speed switch with the tier table (gold/diamond/netherite were wrong); sword web=15; stop mutating the shared static toolBlocks DigProperty (pollution across builds)
canBePutInOffhandSlot() read allow_off_hand from
components.item_properties, but LegacyItemBuilder produces a flat NBT
(no components/item_properties path). CompoundTag.getCompound returns a
fresh empty tag for missing keys, so getBoolean("allow_off_hand") was
always false — silently blocking every legacy custom item from the
off-hand slot, including legitimate ones (e.g. custom shields defined
in a behavior pack).
Guard the NBT read with isComponentBased(). Legacy items return true
because the server holds no allow_off_hand information; the policy lives
in the client behavior pack, so the server defers to the client. This
drops server-side defense-in-depth for legacy items, but the server
fundamentally cannot decide correctly, and the previous false was a
definite false negative. Component-based items keep their exact behavior.
- CustomItem: extract resolveDefinition() returning CustomItemDefinition;
getDefinitionNbt() delegates to it (no behavior change)
- ItemCustom: branch on isComponentBased() in canBePutInOffhandSlot()
- ItemStackRequestProcessorTest: add offhandAcceptsLegacyCustomItem
using a LegacyItemBuilder-based TestLegacyCustomItem
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
# Conflicts: # src/main/java/cn/nukkit/Player.java
- getTopWindow(): track most recently opened non-permanent window explicitly so the result is deterministic regardless of HashBiMap iteration order - CraftLoom/Grindstone: validate the consume plan before firing the craft event, so a rejected request never surfaces to plugin handlers as success - Trade/EnchantInventory.onClose: capture the addItem() remainder and drop it instead of silently deleting the unplaced portion when the pack is partial - BundleInventory.setItemForce: keep shulker-box and bundle-cycle invariants intact on the force/rollback path - Item.getCustomItemDefinition(String): add a direct lookup that avoids cloning the registry on hot paths (custom armor/tool property reads)
canBePutInOffhandSlot() only existed on ItemCustom, but ItemCustomTool, ItemCustomArmor, ItemCustomEdible, ItemCustomProjectile and ItemCustomBookEnchanted do not extend it (they extend XxxBase implements CustomItem). They fell through to Item.canBePutInOffhandSlot(), which only admits vanilla shields/arrows/etc., so the SAI path rejected every custom item of these subclasses from the off-hand slot. A CustomItem interface default cannot override Item's method due to Java's "class wins" rule, so centralize the logic in CustomItem.isAllowedInOffHand(CustomItem) (static) and have each ItemCustom* subclass override canBePutInOffhandSlot() to delegate. - CustomItem: add static isAllowedInOffHand(CustomItem) - ItemCustom: delegate instead of duplicating the rule - ItemCustomTool/Armor/Edible/Projectile/BookEnchanted: override and delegate - ItemStackRequestProcessorTest: cover ItemCustomTool off-hand path (offhandAcceptsCustomToolThatAllowsOffHand / offhandRejectsCustomToolThatDisallowsOffHand)
# Conflicts: # src/main/java/cn/nukkit/Player.java
Contributor
📋 PR Review✅ SummaryThis PR adds a substantial server-authoritative inventory (SAI) ItemStack request processing system with extensive test coverage. However, there are two blocking correctness/security concerns that should be addressed before merging. 🔍 Issues FoundBlocking:
Minor:
💡 Recommendations
📝 Notes
🤖 AI Review |
tachibana-kanade-angel
added
the
[Status] AI Confirmed Pending | AI 确认待核
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR introduces a comprehensive ItemStack request processing system and various inventory improvements to support modern Bedrock Edition client interactions.
Key Changes
ItemStack Request Processing
ItemStackRequestHandlerto process client item stack requestsTransferItemActionProcessor– Move items between slots/containersCraftRecipeActionProcessor/CraftRecipeAutoProcessor/CraftRecipeOptionalProcessor– Crafting operationsConsumeActionProcessor– Item consumptionDropActionProcessor– Dropping itemsSwapActionProcessor– Swapping itemsCreateActionProcessor/DestroyActionProcessor– Item creation/destructionCraftGrindstoneActionProcessor/CraftLoomActionProcessor– Specialized crafting stationsCraftCreativeActionProcessor– Creative mode craftingBeaconPaymentActionProcessor– Beacon interactionsMineBlockActionProcessor– Block mining with itemsInventory System Enhancements
ItemStackNetManagerfor network ID allocation and trackingBundleInventorywith dedicated unit testsHorseInventoryfor equine entity interactionsInventoryObserverSyncfor synchronized inventory observationNetworkMappingto handle container slot mappings (with unit tests)Utility & Helper Classes
EnchantmentHelperfor enchantment-related calculationsTradeRecipeBuildUtilsfor villager trading supportItemStackRequestContextfor request execution contextEvents & API
ItemStackRequestActionEventfor plugin interactionPlayerTransferItemEventfor item transfer hooksFixes & Improvements
Testing
BundleInventoryTest– validates bundle inventory mechanicsNetworkMappingTest– validates slot mapping logic