feat: item-backed container menus (Phase 3) - #14
Draft
KP2048 wants to merge 3 commits into
Draft
Conversation
Add ComposeItemContainerMenu, an ItemStack-backed sibling of the existing BlockEntity-backed menu (renamed ComposeContainerMenu -> ComposeBlockContainerMenu to match) - e.g. for a backpack/bag with its own GUI. ## 3a: base-class extraction Extract ComposeContainerMenuBase<SELF> from ComposeContainerMenu, holding the ~95% of slot layout/registration/quickMoveStack machinery that's already fully holder-agnostic. ComposeBlockContainerMenu keeps tile/blockEntityState/ BlockEntityStateManager registration; new onMenuOpened()/onMenuClosed(player) hooks let each subclass register whatever state-tracking its holder needs. Add isPlayerSlotExcluded(index) to freeze one player-inventory slot in place (mayPlace/mayPickup false, special-cased in quickMoveStack too) - used by ComposeItemContainerMenu to stop a backpack's own inventory slot accepting placement or being shift-clicked into itself while its GUI is open. ComposeContainerScreen drops its second (BlockEntity) type parameter, now `<T : ComposeContainerMenuBase<T>>`; provides LocalBlockEntityState/LocalItemState conditionally based on which concrete menu type is actually open. Caught and fixed before it shipped: calling onMenuOpened() from ComposeContainerMenuBase's own init block dispatched into the subclass's override before the subclass's own constructor-parameter properties (tile/ itemAccess) were assigned - a "leaking this during construction" NPE trap. Each concrete subclass now calls it from its own init, after its own state is ready. ## 3b: @sync wiring for item-backed NBTHolder fields New SyncedItemHolder interface (registerSyncedProperty at declaration time + onSyncedPropertyChanged on every write). ItemStackNBTHolderImpl's field/ listField/mapField/itemField/fluidField/energyField now push through it the same way NBTHolderImpl already does for BlockEntity, and getSyncTag() is implemented for real (was a stub returning an empty tag). Also fixed a real, pre-existing bug this surfaced (not introduced by this change, but first actually exercised by it): ItemStackNBTHolderImpl's init { loadFromStack() } runs before any field is declared, so loadFromTag's itemStorage/fluidStorage/energyStorage re-hydration loop always iterated an empty map - itemField/fluidField/energyField never actually loaded a previously-saved stack's data into the freshly-constructed storage. Fixed by hydrating each storage directly from already-loaded raw data at declaration time instead of relying on that loop. ## 3c: the menu itself ItemContainerAccess (+ PlayerInventoryItemAccess) locates the backing stack, re-resolved fresh every call since the underlying reference can be swapped out from under the menu. ComposeItemState/ItemStatePacket/ItemUpdatePacket mirror the BlockEntity state-sync path, reusing BlockEntityStatePacket.SerializedValue directly; routing needs no position-keyed lookup on either side since an item-backed menu is inherently 1:1 with one player's session - context.player.containerMenu is always "the current menu." ItemStateManager drives per-tick dirty-property sync and force-closes a menu whose itemAccess.stillValid(player) goes false. ## Verification New ComposeItemContainerMenuTests (server, direct construction against makeMockServerPlayerInLevel - no network round trip) covers slot-content persistence, stillValid, @sync serializer registration at declaration time, and ItemStateManager's real tick-driven force-close. New ComposeItemContainerMenuClientTests (client, real MenuRegistry.openExtendedMenu + real TestItemContainerScreen layout pass) covers the excluded-slot mayPlace/ mayPickup/quickMoveStack behavior, which needs updateSlotData to have actually run - unreachable from a server-only GameTest since packet registration itself is skipped there (Archie.kt's own gating, unrelated to this feature). All 4 server tests and both client tests (the new one plus the existing TestScreenGameTest smoke test) pass under real fabric-test:runGametest / runGametestClient runs. Not covered - manual-only, matching precedent (full Compose rendering + real player interaction isn't GameTest-covered anywhere else in this codebase either): the actual on-screen backpack open/close/shift-click flow beyond what the client test above exercises, and copyOnDeath-style cross-mod capability-lookup interop combining Phase 1's exposeItemStorage with a Phase-3-backed backpack. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ADeferredRegistryHolder.initClient() scheduled on Architectury's LifecycleEvent.SETUP (Common Setup on NeoForge), which fires after RegisterMenuScreensEvent - the event MenuRegistry.registerScreenFactory itself listens for internally. Screens silently never registered on NeoForge as a result. Adds scheduleEarlyClientRegistration, an expect/actual that runs immediately on Fabric (no staged registry events to race) and hooks RegisterMenuScreensEvent directly on NeoForge, and removes the manual per-mod workaround this previously required in Archie-Test. Also fixes two build issues surfaced while verifying this end-to-end: actualizer's stubUnfulfilledExpects() stub was leaking into common's published jar/sourcesJar and neoforge's dev jar, which could win Kotlin's actual-resolution over the real actual and (for any non-trivial actual body) crash at runtime with "never actualized".
- ItemStackNBTHolderImpl.updateProperty(): storage-backed fields (item/fluid/energy) wrote into `data` but never the live storage object, so a client-sent edit was immediately discarded by saveToTag()'s unconditional re-derivation from the untouched live storage. Now writes through via readSnapshot(). - field()/listField()/mapField()/itemField()/fluidField()/energyField(): a @sync property whose value pre-existed on the stack never announced its initial value, so a menu opened against pre-existing data started out unsynced until some unrelated future write touched it. - ComposeItemState.updateProperty(): a property packet arriving before any composable observed it stuck a bare mutableStateOf() in place of the send-forwarding PropertyState wrapper, silently breaking that property's client-to-server sync for the rest of the session. - ItemStateManager: isolate exceptions per-menu in the tick sync loop so one broken menu can't starve every other open menu of syncing. - TestItemMenu.kt: fix stale KDoc link to a test class that was never created. - docs/gametest.md: fix stale ComposeContainerMenu/ComposeContainerScreen signatures after the ComposeContainerMenuBase extraction.
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
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.
Phase 3 of the pre-release storage/GUI plan: item-backed container menus
Adds
ComposeItemContainerMenu, anItemStack-backed sibling of the existing BlockEntity-backedmenu (renamed
ComposeContainerMenu→ComposeBlockContainerMenuto match) — e.g. for abackpack/bag with its own GUI.
3a — base-class extraction
Extracted
ComposeContainerMenuBase<SELF>fromComposeContainerMenu, holding the ~95% of slotlayout/registration/
quickMoveStackmachinery that's already fully holder-agnostic.ComposeBlockContainerMenukeepstile/blockEntityState/BlockEntityStateManagerregistration; new
onMenuOpened()/onMenuClosed(player)hooks let each subclass registerwhatever state-tracking its holder needs. Added
isPlayerSlotExcluded(index)to freeze oneplayer-inventory slot in place (
mayPlace/mayPickupfalse, special-cased inquickMoveStacktoo) — used by
ComposeItemContainerMenuto stop a backpack's own inventory slot acceptingplacement or being shift-clicked into itself while its GUI is open.
ComposeContainerScreendropsits second (BlockEntity) type parameter, now
<T : ComposeContainerMenuBase<T>>.Caught and fixed before it shipped: calling
onMenuOpened()fromComposeContainerMenuBase's owninitblock dispatched into the subclass's override beforethe subclass's own constructor-parameter properties (
tile/itemAccess) were assigned — a"leaking
thisduring construction" NPE trap. Each concrete subclass now calls it from its owninit, after its own state is ready.3b —
@Syncwiring for item-backedNBTHolderfieldsNew
SyncedItemHolderinterface.ItemStackNBTHolderImpl's field declarations now push throughit the same way
NBTHolderImplalready does forBlockEntity, andgetSyncTag()is implementedfor real (was a stub returning an empty tag).
Also fixed a real, pre-existing bug this surfaced (not introduced by this change, but first
actually exercised by it):
ItemStackNBTHolderImpl'sinit { loadFromStack() }runs before anyfield is declared, so
loadFromTag'sitemStorage/fluidStorage/energyStoragere-hydrationloop always iterated an empty map —
itemField/fluidField/energyFieldnever actually loaded apreviously-saved stack's data into the freshly-constructed storage. Fixed by hydrating each
storage directly from already-loaded raw data at declaration time instead.
3c — the menu itself
ItemContainerAccess(+PlayerInventoryItemAccess) locates the backing stack, re-resolved freshevery call since the underlying reference can be swapped out from under the menu.
ComposeItemState/ItemStatePacket/ItemUpdatePacketmirror the BlockEntity state-sync path,reusing
BlockEntityStatePacket.SerializedValuedirectly; routing needs no position-keyed lookupon either side since an item-backed menu is inherently 1:1 with one player's session —
context.player.containerMenuis always "the current menu."ItemStateManagerdrives per-tickdirty-property sync and force-closes a menu whose
itemAccess.stillValid(player)goes false.Verification
New
ComposeItemContainerMenuTests(server, direct construction againstmakeMockServerPlayerInLevel— no network round trip) covers slot-content persistence,stillValid,@Syncserializer registration at declaration time, andItemStateManager's realtick-driven force-close. New
ComposeItemContainerMenuClientTests(client, realMenuRegistry.openExtendedMenu+ a realTestItemContainerScreenlayout pass) covers theexcluded-slot
mayPlace/mayPickup/quickMoveStackbehavior, which needsupdateSlotDatatohave actually run — unreachable from a server-only GameTest since packet registration itself is
skipped there (
Archie.kt's own gating, unrelated to this feature).Not covered — manual-only, matching precedent (full Compose rendering + real player interaction
isn't GameTest-covered anywhere else in this codebase either): the actual on-screen backpack
open/close/shift-click flow beyond what the client test above exercises, and
copyOnDeath-stylecross-mod capability-lookup interop combining Phase 1's
exposeItemStoragewith aPhase-3-backed backpack.
Docs:
docs/gui.mdgets a newComposeBlockContainerMenu/ComposeItemContainerMenusection plusan expanded state-sync paragraph covering
observeItemProperty;docs/transfer.md's "Wiringstorage into a menu" section now shows both menu kinds.
Final piece of the three-feature pre-release plan (capability lookup / data attachments /
item-backed container menus) — #12 and #13 cover the first two.
Rebased onto latest
1.21.x(picking up #10/#11) before opening; both GameTest suites re-verifiedgreen afterward.
🤖 Generated with Claude Code