From b1d02edd9922e9929899d9ada4bc0eaf5ba7cfad Mon Sep 17 00:00:00 2001 From: Anon Date: Sat, 6 Jun 2026 10:27:20 +0200 Subject: [PATCH] Remove local inventory report from branch --- inventory-sync-issue-report.md | 377 --------------------------------- 1 file changed, 377 deletions(-) delete mode 100644 inventory-sync-issue-report.md diff --git a/inventory-sync-issue-report.md b/inventory-sync-issue-report.md deleted file mode 100644 index 2d743820..00000000 --- a/inventory-sync-issue-report.md +++ /dev/null @@ -1,377 +0,0 @@ -# MCC inventory sync issues report - -## Sources read - -- GitHub issue #3112: https://github.com/MCCTeam/Minecraft-Console-Client/issues/3112 -- GitHub issue #3126: https://github.com/MCCTeam/Minecraft-Console-Client/issues/3126 -- GitHub issue #3124: https://github.com/MCCTeam/Minecraft-Console-Client/issues/3124 -- Related issue referenced by #3124: https://github.com/MCCTeam/Minecraft-Console-Client/issues/3119 -- Commit inspected: `3c59bfe13abc18291506bec59118b5bd7509f4eb` (`Experimental inventory sync`) - -## Short version - -There are three separate but related problems in the reports: - -1. Before commit `3c59bfe`, `/inventory container list` could be correct while `/inventory player list` stayed stale. The script used `GetPlayerInventory()`, so it believed moved or trashed items were still in the player inventory and repeated the same actions. -2. Commit `3c59bfe` tried to fix that by mirroring the player-inventory part of an open container back into inventory id `0`. That addressed the stale player list in the first test, but it introduced or exposed `x0` ghost items because the mirrored player inventory can share the same `Item` object reference as the open container, and later local shift-click prediction can mutate that object to `Count = 0`. -3. Issue #3124 is mainly a Script Scheduler/reconnect duplication issue. It matters here because the posted `WarpAFK.cs` scripts show the user process and the workaround they wrote for the stale inventory problem, but the scheduler bug itself is not fixed by `3c59bfe`. - -## What the issue is exactly - -MCC keeps inventories in a dictionary keyed by server window id. Inventory id `0` is the player inventory. Open containers use separate ids, for example the `/trash` or `/çöp` GUI and the `/is chest N` GUI. - -In Minecraft protocol, an open container window contains both: - -- the container's own top slots -- a mirrored copy of the player's normal 36 inventory slots - -For example: - -- A 27-slot chest (`Generic_9x3`) has 63 total slots in MCC: 27 chest slots plus 36 mirrored player slots. -- A 36-slot trash GUI (`Generic_9x4`) has 72 total slots in MCC: 36 trash slots plus 36 mirrored player slots. -- The mirrored player slots begin after the container's own top slots. - -The user's old script scans `GetPlayerInventory()` to decide what to do, then it clicks the mirrored player slot in the open container: - -- For `/çöp`, the GUI has 36 top slots, so `containerSlot = playerSlot + 27`. Example: player slot `36` maps to container slot `63`. -- For `/is chest N`, the GUI has 27 top slots, so `containerSlot = playerSlot + 18`. Example: player slot `36` maps to container slot `54`. - -Before `3c59bfe`, `DoWindowAction()` updated only the clicked container object. If the script clicked the open trash/chest container, MCC's current container snapshot was updated, but inventory id `0` was not updated from that container's mirrored player slots. Therefore: - -- `/inventory container list` showed the item removed. -- `/inventory player list` still showed the old item. -- `GetPlayerInventory()` returned the stale id `0` state. -- The bot returned to trash/storage mode and clicked the same logical item slots again. - -This is the core bug from #3112. - -## How to reproduce #3112 manually by hand - -These steps avoid the automation script and use only MCC commands. They are based on the user's server and process from #3112. - -Prerequisites: - -- Connect MCC to `play.ronemacraft.com`. -- Use Minecraft `1.20.4`. -- Use a build before the experimental inventory sync fix, for example the reported MCC version 445 or another build before `3c59bfe`. -- Have inventory handling enabled. -- Have at least one item that the server can move into the trash GUI or an island chest. - -Trash GUI reproduction: - -1. Stand still in the Opskyblock/AFK area where the user observed the problem. -2. Put an ore item in the normal player inventory, for example `EmeraldOre`, `IronOre`, `GoldOre`, or `DiamondOre`. -3. Run `/inventory player list`. -4. Note the player slot that contains the ore. For a hotbar item this is usually in slots `36` through `44`. -5. Send `/çöp` or the server's `/trash` alias. -6. Wait until MCC logs that a virtual trash inventory opened. -7. Run `/inventory container list`. -8. Find the same item in the container's mirrored player section. For the 36-slot trash GUI, use `containerSlot = playerSlot + 27`. -9. Run `/inventory container click ShiftClick`. -10. Run `/inventory container list` again. The container view should show that the item was removed or moved. -11. Run `/inventory player list`. -12. On the broken builds, the player list can still show the item in the old player slot. -13. Close the container with `/inventory container close`. -14. Open `/çöp` again and repeat the same click. The server-side item is already gone, but MCC still thinks it exists because id `0` is stale. - -Island chest reproduction: - -1. Put any non-ore item in the player inventory. -2. Run `/inventory player list` and note the player slot. -3. Send `/opskyblock` if needed by the server flow. -4. Send `/is chest 21`. -5. Wait until the chest opens. -6. For the 27-slot chest GUI, compute `containerSlot = playerSlot + 18`. -7. Run `/inventory container click ShiftClick`. -8. Run `/inventory container list`. The open container view should reflect the move. -9. Run `/inventory player list`. On affected builds, the item can still be listed in the player inventory. -10. The user's script then sees `hasOther = true`, tries the next chest, and repeats until its own workaround stops it. - -Why moving or warping seems to fix it: - -- The user said `/spawn` to `/warp afk5` refreshes `/inventory player list`. -- That is consistent with the server resending player inventory or slot state during a world/lobby/teleport transition. -- It is not a useful workaround for the user because leaving the AFK location resets the 30-minute reward counter. - -## How issue #3124 fits in - -Issue #3124 is not the same inventory defect. It is a continuation of #3119, where Script Scheduler can re-run login-triggered scripts during `/reco` or reconnect spam while old script instances keep running. - -The relevant Script Scheduler config from #3124 is: - -- `Trigger_On_Login = true` -- One task sends `/login password`. -- Another task runs `script WarpAFK.cs`. - -Manual scheduler reproduction: - -1. Enable `ChatBot.ScriptScheduler`. -2. Add a login-triggered task that starts a script, like the `WarpAFK.cs` shown in #3124. -3. Connect to the server. -4. Repeatedly run `/reco`, or force several disconnect/reconnect cycles. -5. Watch the log for multiple script instances continuing at once. -6. Expected behavior would be that old script instances are stopped before a new login-triggered instance starts. -7. Actual behavior from #3119/#3124 was repeated commands such as `/opskyblock`, `/pay`, `/is chest 21`, and `/warp afk5`, sometimes before MCC considers itself fully connected. - -Why this matters for the inventory report: - -- The first `WarpAFK.cs` in #3124 contains slot-recorder logic (`doneSlotTypes`, `confirmedPSlots`, `failedPSlots`) specifically because `/inventory player list` was unreliable. -- The later `WarpAFK.cs` posted in #3124 is simpler because the user believed the new player inventory sync worked. It goes back to scanning `GetPlayerInventory()` for `PlayerItems()`. -- That means the experimental inventory sync directly changed how the script could be written, even though the reconnect duplication is a separate bug. - -## What changed in commit 3c59bfe - -Commit `3c59bfe13abc18291506bec59118b5bd7509f4eb` is titled `Experimental inventory sync`. - -Files changed: - -- `MinecraftClient/McClient.cs` -- `MinecraftClient/Resources/Translations/Translations.resx` -- `MinecraftClient/Resources/Translations/Translations.Designer.cs` - -Main inventory changes: - -1. Fixed a merge-order bug in `TryMergeSlot()`. - - Before the commit, the full-merge branch set `item.Count = 0` before adding it to the destination stack. That meant the destination received `0`. - - After the commit, it adds `item.Count` to `curItem.Count` first, then sets `item.Count = 0`. - - Current code: `MinecraftClient/McClient.cs`, around lines 1998-2014. - -2. Added mirrored player-inventory range detection. - - `TryGetMirroredPlayerInventoryRange()` assumes non-player containers end with 36 mirrored player inventory slots. - - `TryGetMirroredPlayerInventorySlot()` maps a container window slot back to a player inventory slot using `playerInventorySlot = windowSlot - firstWindowSlot + 9`. - - Current code: `MinecraftClient/McClient.cs`, around lines 2065-2095. - -3. Added `SetPlayerInventorySlot()`. - - This updates inventory id `0` for a mapped player slot. - - It removes the slot when the incoming item is null or empty. - - It skips work when `AreSameInventorySlot()` says the old and new slot are equivalent. - - Current code: `MinecraftClient/McClient.cs`, around lines 2097-2125. - -4. Added sync from open containers to player inventory id `0`. - - `SyncPlayerInventorySlotFromWindow()` syncs one mapped mirrored slot. - - `SyncPlayerInventorySlotsFromWindow()` syncs all mirrored slots. - - `DoWindowAction()` now calls `SyncPlayerInventorySlotsFromWindow(inventory)` before sending the click packet. - - `OnWindowItems()` now syncs the whole mirrored range and dispatches `OnInventoryUpdate(0)` if changed. - - `OnSetSlot()` now syncs a mapped mirrored slot and dispatches `OnInventoryUpdate(0)` if changed. - - Current code: `MinecraftClient/McClient.cs`, around lines 2128-2152, 2896, and 3931-3989. - -5. Added empty-item filtering to `OnWindowItems()`. - - The commit removes items from incoming full window contents when `Item.IsEmpty` is true. - - Current code: `MinecraftClient/McClient.cs`, around lines 3935-3937. - -6. Added cursor cleanup for left/right click predictions. - - If cursor slot `-1` reaches zero count, it is removed. - - Current code: `MinecraftClient/McClient.cs`, around lines 2219-2221 and 2280-2282. - -7. Added shift-click cleanup. - - If a shift-clicked source item reaches zero count and is still present in the clicked container dictionary, it is removed. - - Current code: `MinecraftClient/McClient.cs`, around lines 2868-2872. - -Translation changes: - -- `cmd.inventory.shiftclick` changed from `Shift clicking slot {0} in window #{1}` to `Shift`. -- `cmd.inventory.shiftrightclick` changed from `Shift right-clicking slot {0} in window #{1}` to `Shift right`. -- This fixes the nested format-string problem from #3126. The outer message is `{0} clicking slot {1} in window #{2}`, so the action label must not contain its own `{0}` and `{1}` placeholders. -- Current command formatting is in `MinecraftClient/Commands/Inventory.cs`, around lines 355-365. - -## Why the commit helped #3112 - -The main stale-list failure was that id `0` did not follow the mirrored player section inside nonzero container windows. - -The new sync code makes this happen: - -1. Open `/çöp` or `/is chest N`. -2. The open container's full slot list includes the player's mirrored 36 slots. -3. `OnWindowItems()` copies that mirrored section into player inventory id `0`. -4. After a click, `DoWindowAction()` locally predicts the container change and syncs mirrored slots into id `0`. -5. Bot scripts reading `GetPlayerInventory()` now see the result of the container click. - -This is why the test build mentioned in #3112 appeared to work for the user's old script. - -## Why the commit did not fully work - -The commit can leave `x0` items in `/inventory player list`. - -The most likely concrete cause is object aliasing plus the equality check in `SetPlayerInventorySlot()`. - -Detailed flow: - -1. `OnWindowItems()` receives an open container, for example `/is chest 21`. -2. `SyncPlayerInventorySlotsFromWindow()` maps the mirrored player slots from that container into inventory id `0`. -3. `SetPlayerInventorySlot()` stores the same `Item` object reference from the container dictionary into the player inventory dictionary. It does not clone the item. -4. The user shift-clicks a mirrored player slot in the open container. -5. If the destination already has the same stackable item, `TryMergeSlot()` fully merges the source stack. -6. In that full-merge branch, the code sets the source `item.Count = 0`. -7. Because player inventory id `0` may hold the same `Item` object reference, the player inventory entry now also has `Count = 0`. -8. `DoWindowAction()` removes the source slot from the open container and calls `SyncPlayerInventorySlotsFromWindow()`. -9. The sync sees the window slot is now empty and tries to set the mapped player slot to `null`. -10. `SetPlayerInventorySlot()` calls `AreSameInventorySlot(previousItem, null)`. -11. `AreSameInventorySlot()` returns true when the previous item is empty and the new item is null. -12. Because it returns true, `SetPlayerInventorySlot()` exits early and does not remove the existing dictionary entry. -13. `/inventory player list` iterates the dictionary directly and prints the item, so it appears as `x0`. - -This matches #3126 very closely: - -- The user bought one grassy soil from `/market` or `/shop`. -- The script stored it in `/is chest`. -- The first item may have moved into an empty chest slot, so no `x0` appeared. -- The user bought the same item again. -- The second item could merge into the existing chest stack, causing `TryMergeSlot()` to set the source count to zero. -- The player inventory dictionary retained the zero-count item, so `/inventory player list` showed `x0`. - -It also explains why the issue is intermittent: - -- Moving into an empty destination slot uses `StoreInNewSlot()`, which removes the source slot but does not set the source item count to zero. -- Fully merging into an existing stack uses `TryMergeSlot()`, which does set the source item count to zero. -- Therefore the visible result depends on whether the item moved into an empty slot or merged into an existing compatible stack. - -## How to reproduce #3126 manually by hand - -Use the experimental build containing `3c59bfe`. - -Server-specific reproduction from the issue: - -1. Connect to `play.ronemacraft.com` on Minecraft `1.20.4`. -2. Go to the Opskyblock area where `/market` or `/shop` is available. -3. Buy exactly one item from `/market` or `/shop`. The user specifically mentioned grassy soil. -4. Run `/inventory player list` and note the player slot containing the item. -5. Open an island chest with `/is chest 21`. -6. Use `/inventory container list` to confirm the item appears in the mirrored player section. -7. Compute the container slot as `playerSlot + 18` for a 27-slot chest. -8. Run `/inventory container click ShiftClick`. -9. Close the container with `/inventory container close`. -10. Run `/inventory player list`. The item may disappear correctly. -11. Buy the same item again, exactly one item. -12. Open the same `/is chest 21` again. -13. Shift-click the second item into the chest using the same slot mapping. -14. Run `/inventory player list`. -15. If the second item merged into the existing stack in the chest, the player list can show that item as `x0`. - -Generic reproduction that should not require this specific server: - -1. Use a build containing `3c59bfe`. -2. Connect to any server where inventory actions work. -3. Put a partial stack of a stackable item in a chest, for example 1 dirt in a chest slot. -4. Put 1 dirt in the player's inventory. -5. Open the chest. -6. Run `/inventory player list` and note the player slot of the dirt. -7. For a normal 27-slot chest, compute `containerSlot = playerSlot + 18`. -8. Run `/inventory container click ShiftClick`. -9. Run `/inventory player list` quickly after the click. -10. The expected correct result is no dirt in the old player slot. -11. The suspected broken result is a retained entry with `x0 Dirt`. - -If this generic reproduction does not show `x0`, add logging or inspect the dictionaries immediately after `DoWindowAction()`. Server correction packets can sometimes mask the display timing, but the code path itself can retain a zero-count item. - -## Is this just an issue on this server? - -The stale `/inventory player list` issue is not purely a RonemaCraft server bug. It is a real MCC state-model gap: - -- MCC had separate snapshots for player inventory id `0` and open container ids. -- Open containers include mirrored player inventory slots. -- Before the commit, MCC did not sync those mirrored slots back into id `0`. -- Any server or plugin GUI that updates only the open container window can expose this mismatch. - -RonemaCraft makes the issue easy to notice because: - -- It uses plugin GUIs for `/çöp`, `/is chest`, `/market`, and daily rewards. -- The user's bot depends on `GetPlayerInventory()` while staying stationary in an AFK area. -- Warping away refreshes inventory but resets the AFK reward counter, so the stale state persists long enough to hurt the workflow. - -The `x0` issue from `3c59bfe` is also not inherently server-only. It follows from MCC's own local prediction and reference sharing. The server's workflow makes it reproducible because buying the same item twice and storing it into the same chest naturally creates a merge-into-existing-stack case. - -The server can still affect how often the bug appears: - -- Some servers send extra full inventory refreshes that mask stale id `0` state. -- Some plugin GUIs send unusual item stacks, custom names, or zero-count placeholders. -- Some server flows delay or omit player-window updates while only updating the open plugin window. - -So the right classification is: - -- #3112: general MCC design gap, exposed strongly by this server's plugin GUI workflow. -- #3126: general bug in the experimental fix, easiest to trigger with this server's repeated market/chest process. -- #3124/#3119: Script Scheduler/reconnect lifecycle bug, separate from inventory sync. - -## How the inventory system worked before this commit - -Before `3c59bfe`, the relevant flow was: - -1. MCC initializes the inventory dictionary with player inventory id `0`. - - `ClearInventories()` creates `inventories[0] = new Container(0, ContainerType.PlayerInventory, "Player Inventory")`. - - Current code is around `MinecraftClient/McClient.cs` lines 2960-2970. - -2. When the server opens a container, MCC adds another `Container` under that server window id. - - The container type comes from the protocol menu type id. - - The title comes from the packet title. - - For 1.20.4, this happens through `OpenWindow` handling in `Protocol18.cs`. - -3. Each `Container` has a flat `Items` dictionary. - - Key: slot id. - - Value: `Item`. - - Empty slots are normally absent from the dictionary. - -4. Container slot counts include the player's mirrored inventory section. - - `Generic_9x3` is 63 total slots: 27 top slots plus 36 player slots. - - `Generic_9x4` is 72 total slots: 36 top slots plus 36 player slots. - - `Generic_9x6` is 90 total slots: 54 top slots plus 36 player slots. - - `PlayerInventory` is 46 total slots. - - Current values are in `MinecraftClient/Inventory/ContainerTypeExtensions.cs`, around lines 10-40. - -5. `/inventory player list` reads only inventory id `0`. - - It iterates `inventory.Items` directly. - - It prints every dictionary entry, including an item whose count is zero. - - Current command code is around `MinecraftClient/Commands/Inventory.cs` lines 320-327. - -6. `/inventory container list` reads the highest open inventory id when no id is supplied. - - This is usually the foreground open server GUI. - - It does not read inventory id `0`. - -7. `OnWindowItems()` replaced only the addressed inventory's item dictionary. - - Before this commit, it did not filter zero-count items. - - Before this commit, it did not sync mirrored player slots into inventory id `0`. - - Before this commit, it dispatched only `OnInventoryUpdate(inventoryID)`. - -8. `OnSetSlot()` updated only the addressed inventory id, except special protocol cases. - - `inventoryID == 254` maps to player inventory id `0`. - - `inventoryID == 255` and slot `-1` maps to the cursor item stored in player inventory slot `-1`. - - Newer 1.21.2+ `SetPlayerInventory` packets map directly to id `0`, but the reported server uses 1.20.4, so that packet path is not available. - -9. `DoWindowAction()` locally predicted the result of clicks before sending the click packet. - - For a nonzero window id, it modified that open container's `Items`. - - It did not also update player inventory id `0`. - - It sent `ClickWindow` with the calculated changed slots and state id. - -10. Bot APIs reflect this model. - - `GetPlayerInventory()` returns inventory id `0`. - - `GetInventories()` returns the full dictionary of player plus open containers. - - `OnInventoryUpdate(int inventoryId)` tells bots which inventory id changed. - - Before the commit, a container mirror change did not imply an `OnInventoryUpdate(0)` event. - -The practical consequence before the commit: - -- If a script needed live item state while a container was open, `/inventory container list` or `GetInventories()[openContainerId]` was more reliable than `GetPlayerInventory()`. -- Scripts had to manually translate mirrored container slots back to player slots, exactly as the user's workaround did. -- `GetPlayerInventory()` was safe only after the server actually refreshed inventory id `0`. - -## Suggested direction for a real fix - -This report is not a patch, but the code evidence points to a safer direction: - -1. Do not store the same mutable `Item` instance in both the open container dictionary and player inventory id `0`. - - Clone items when syncing from a container mirror into player inventory. - -2. Make `SetPlayerInventorySlot()` remove an existing dictionary entry when the new item is null or empty, even if the old entry is already empty. - - The dictionary state matters. An empty item object in the dictionary is not equivalent to no dictionary entry for display and script behavior. - -3. Consider filtering empty items when listing inventories too. - - The data model should not keep `x0` entries, but display code can defensively skip `item.IsEmpty`. - -4. Keep the mirror sync idea, but add tests around these cases: - - Shift-click mirrored player slot into an empty container slot. - - Shift-click mirrored player slot into an existing compatible stack. - - Shift-click one item twice into the same chest stack. - - Open container `WindowItems` sync followed by local `DoWindowAction()`. - - Server `OnSetSlot(windowId, sourceSlot, null, stateId)` after local prediction already changed the source item to zero. - -5. Keep #3124 separate. - - Fixing inventory sync should not be expected to fix Script Scheduler duplicate script instances after reconnect.