From 0a514d13511908205d32df213ce12eb1dd0fc0dd Mon Sep 17 00:00:00 2001 From: Anon Date: Sat, 6 Jun 2026 09:02:02 +0200 Subject: [PATCH 1/4] Revert inventory sync changes --- MinecraftClient/McClient.cs | 61 ------------------------------------- 1 file changed, 61 deletions(-) diff --git a/MinecraftClient/McClient.cs b/MinecraftClient/McClient.cs index da7cbc92..7e7f0435 100644 --- a/MinecraftClient/McClient.cs +++ b/MinecraftClient/McClient.cs @@ -2062,38 +2062,6 @@ namespace MinecraftClient }; } - private static bool TryGetMirroredPlayerInventoryRange(Container inventory, out int firstWindowSlot, out int lastWindowSlot) - { - firstWindowSlot = -1; - lastWindowSlot = -1; - - if (inventory.Type == ContainerType.PlayerInventory) - return false; - - const int mirroredPlayerInventorySlotCount = 36; - int slotCount = inventory.Type.SlotCount(); - if (slotCount < mirroredPlayerInventorySlotCount) - return false; - - firstWindowSlot = slotCount - mirroredPlayerInventorySlotCount; - lastWindowSlot = slotCount - 1; - return true; - } - - private static bool TryGetMirroredPlayerInventorySlot(Container inventory, int windowSlot, out int playerInventorySlot) - { - playerInventorySlot = -1; - - if (!TryGetMirroredPlayerInventoryRange(inventory, out int firstWindowSlot, out int lastWindowSlot)) - return false; - - if (windowSlot < firstWindowSlot || windowSlot > lastWindowSlot) - return false; - - playerInventorySlot = windowSlot - firstWindowSlot + 9; - return true; - } - private static bool AreSameInventorySlot(Item? left, Item? right) { if (left is null || left.IsEmpty) @@ -2125,27 +2093,6 @@ namespace MinecraftClient return true; } - private bool SyncPlayerInventorySlotFromWindow(Container inventory, int windowSlot) - { - if (!TryGetMirroredPlayerInventorySlot(inventory, windowSlot, out int playerInventorySlot)) - return false; - - inventory.Items.TryGetValue(windowSlot, out Item? item); - return SetPlayerInventorySlot(playerInventorySlot, item); - } - - private bool SyncPlayerInventorySlotsFromWindow(Container inventory) - { - if (!TryGetMirroredPlayerInventoryRange(inventory, out int firstWindowSlot, out int lastWindowSlot)) - return false; - - bool changed = false; - for (int windowSlot = firstWindowSlot; windowSlot <= lastWindowSlot; windowSlot++) - changed |= SyncPlayerInventorySlotFromWindow(inventory, windowSlot); - - return changed; - } - /// /// Click a slot in the specified window /// @@ -2872,8 +2819,6 @@ namespace MinecraftClient changedSlots.Add(new Tuple((short)slotId, null)); break; } - - SyncPlayerInventorySlotsFromWindow(inventory); } return handler.SendWindowAction(windowId, slotId, action, item, changedSlots, inventories[windowId].StateID); @@ -3915,9 +3860,6 @@ namespace MinecraftClient { inventories[inventoryID].Items = itemList; inventories[inventoryID].StateID = stateId; - bool playerInventoryChanged = SyncPlayerInventorySlotsFromWindow(inventories[inventoryID]); - if (playerInventoryChanged) - DispatchBotEvent(bot => bot.OnInventoryUpdate(0)); DispatchBotEvent(bot => bot.OnInventoryUpdate(inventoryID)); } } @@ -3958,9 +3900,6 @@ namespace MinecraftClient inventories[inventoryID].Items.Remove(slotID); } else inventories[inventoryID].Items[slotID] = item; - - if (SyncPlayerInventorySlotFromWindow(inventories[inventoryID], slotID)) - DispatchBotEvent(bot => bot.OnInventoryUpdate(0)); } } DispatchBotEvent(bot => bot.OnInventoryUpdate(inventoryID)); From 3c59bfe13abc18291506bec59118b5bd7509f4eb Mon Sep 17 00:00:00 2001 From: Anon Date: Sat, 6 Jun 2026 09:13:19 +0200 Subject: [PATCH 2/4] Experimental inventory sync --- MinecraftClient/McClient.cs | 86 ++++++++++++++++++- .../Translations/Translations.Designer.cs | 4 +- .../Resources/Translations/Translations.resx | 4 +- 3 files changed, 89 insertions(+), 5 deletions(-) diff --git a/MinecraftClient/McClient.cs b/MinecraftClient/McClient.cs index 7e7f0435..198d15d0 100644 --- a/MinecraftClient/McClient.cs +++ b/MinecraftClient/McClient.cs @@ -2004,8 +2004,8 @@ namespace MinecraftClient if (item.Count <= spaceLeft) { // Can fit into the stack - item.Count = 0; curItem.Count += item.Count; + item.Count = 0; changedSlots.Add(new Tuple((short)curId, curItem)); changedSlots.Add(new Tuple((short)slotId, null)); @@ -2062,6 +2062,38 @@ namespace MinecraftClient }; } + private static bool TryGetMirroredPlayerInventoryRange(Container inventory, out int firstWindowSlot, out int lastWindowSlot) + { + firstWindowSlot = -1; + lastWindowSlot = -1; + + if (inventory.Type == ContainerType.PlayerInventory) + return false; + + const int mirroredPlayerInventorySlotCount = 36; + int slotCount = inventory.Type.SlotCount(); + if (slotCount < mirroredPlayerInventorySlotCount) + return false; + + firstWindowSlot = slotCount - mirroredPlayerInventorySlotCount; + lastWindowSlot = slotCount - 1; + return true; + } + + private static bool TryGetMirroredPlayerInventorySlot(Container inventory, int windowSlot, out int playerInventorySlot) + { + playerInventorySlot = -1; + + if (!TryGetMirroredPlayerInventoryRange(inventory, out int firstWindowSlot, out int lastWindowSlot)) + return false; + + if (windowSlot < firstWindowSlot || windowSlot > lastWindowSlot) + return false; + + playerInventorySlot = windowSlot - firstWindowSlot + 9; + return true; + } + private static bool AreSameInventorySlot(Item? left, Item? right) { if (left is null || left.IsEmpty) @@ -2093,6 +2125,33 @@ namespace MinecraftClient return true; } + private bool SyncPlayerInventorySlotFromWindow(Container? inventory, int windowSlot) + { + if (inventory is null) + return false; + + if (!TryGetMirroredPlayerInventorySlot(inventory, windowSlot, out int playerInventorySlot)) + return false; + + inventory.Items.TryGetValue(windowSlot, out Item? item); + return SetPlayerInventorySlot(playerInventorySlot, item); + } + + private bool SyncPlayerInventorySlotsFromWindow(Container? inventory) + { + if (inventory is null) + return false; + + if (!TryGetMirroredPlayerInventoryRange(inventory, out int firstWindowSlot, out int lastWindowSlot)) + return false; + + bool changed = false; + for (int windowSlot = firstWindowSlot; windowSlot <= lastWindowSlot; windowSlot++) + changed |= SyncPlayerInventorySlotFromWindow(inventory, windowSlot); + + return changed; + } + /// /// Click a slot in the specified window /// @@ -2157,6 +2216,10 @@ namespace MinecraftClient playerInventory.Items.Remove(-1); } + // Clean up cursor item if count reached zero + if (playerInventory.Items.TryGetValue(-1, out Item? cursorAfterLeft) && cursorAfterLeft.IsEmpty) + playerInventory.Items.Remove(-1); + if (inventory.Items.ContainsKey(slotId)) changedSlots.Add(new Tuple((short)slotId, inventory.Items[slotId])); else @@ -2213,6 +2276,10 @@ namespace MinecraftClient inventory.Items[slotId] = itemClone; playerInventory.Items[-1].Count--; } + + // Clean up cursor item if count reached zero + if (playerInventory.Items.TryGetValue(-1, out Item? cursorItem) && cursorItem.IsEmpty) + playerInventory.Items.Remove(-1); } else { @@ -2798,6 +2865,11 @@ namespace MinecraftClient changedSlots.Add(new Tuple((short)slotId, inventory.Items[slotId])); } } + if (item!.Count <= 0 && inventory.Items.ContainsKey(slotId)) + { + inventory.Items.Remove(slotId); + changedSlots.Add(new Tuple((short)slotId, null)); + } } break; case WindowActionType.DropItem: @@ -2821,6 +2893,8 @@ namespace MinecraftClient } } + SyncPlayerInventorySlotsFromWindow(inventory); + return handler.SendWindowAction(windowId, slotId, action, item, changedSlots, inventories[windowId].StateID); } @@ -3858,8 +3932,15 @@ namespace MinecraftClient { if (inventories.ContainsKey(inventoryID)) { + // Filter out empty items (Count=0 or Air) that some servers may send + foreach (int key in itemList.Where(slot => slot.Value.IsEmpty).Select(slot => slot.Key).ToList()) + itemList.Remove(key); + inventories[inventoryID].Items = itemList; inventories[inventoryID].StateID = stateId; + bool playerInventoryChanged = SyncPlayerInventorySlotsFromWindow(inventories[inventoryID]); + if (playerInventoryChanged) + DispatchBotEvent(bot => bot.OnInventoryUpdate(0)); DispatchBotEvent(bot => bot.OnInventoryUpdate(inventoryID)); } } @@ -3900,6 +3981,9 @@ namespace MinecraftClient inventories[inventoryID].Items.Remove(slotID); } else inventories[inventoryID].Items[slotID] = item; + + if (SyncPlayerInventorySlotFromWindow(inventories[inventoryID], slotID)) + DispatchBotEvent(bot => bot.OnInventoryUpdate(0)); } } DispatchBotEvent(bot => bot.OnInventoryUpdate(inventoryID)); diff --git a/MinecraftClient/Resources/Translations/Translations.Designer.cs b/MinecraftClient/Resources/Translations/Translations.Designer.cs index 52ec91ae..2330a891 100644 --- a/MinecraftClient/Resources/Translations/Translations.Designer.cs +++ b/MinecraftClient/Resources/Translations/Translations.Designer.cs @@ -4135,7 +4135,7 @@ namespace MinecraftClient { } /// - /// Looks up a localized string similar to Shift clicking slot {0} in window #{1}. + /// Looks up a localized string similar to Shift. /// internal static string cmd_inventory_shiftclick { get { @@ -4153,7 +4153,7 @@ namespace MinecraftClient { } /// - /// Looks up a localized string similar to Shift right-clicking slot {0} in window #{1}. + /// Looks up a localized string similar to Shift right. /// internal static string cmd_inventory_shiftrightclick { get { diff --git a/MinecraftClient/Resources/Translations/Translations.resx b/MinecraftClient/Resources/Translations/Translations.resx index 6052e661..cb2dae94 100644 --- a/MinecraftClient/Resources/Translations/Translations.resx +++ b/MinecraftClient/Resources/Translations/Translations.resx @@ -1457,7 +1457,7 @@ Note that parameters in '[]' are optional. Right - Shift clicking slot {0} in window #{1} + Shift Shift click failed, this may be because this container type is not supported @@ -2329,7 +2329,7 @@ Logging in... Minimum number that you have provided is bigger than the maximum, swapping them around! - Shift right-clicking slot {0} in window #{1} + Shift right Avaliable profiles: From 6bfcd9e9cac3877a5f65c2d94f60744c36fd6e68 Mon Sep 17 00:00:00 2001 From: Anon Date: Sat, 6 Jun 2026 10:22:45 +0200 Subject: [PATCH 3/4] Fix mirrored player inventory sync --- MinecraftClient/McClient.cs | 99 +++++---- inventory-sync-issue-report.md | 377 +++++++++++++++++++++++++++++++++ 2 files changed, 439 insertions(+), 37 deletions(-) create mode 100644 inventory-sync-issue-report.md diff --git a/MinecraftClient/McClient.cs b/MinecraftClient/McClient.cs index 198d15d0..e19a0260 100644 --- a/MinecraftClient/McClient.cs +++ b/MinecraftClient/McClient.cs @@ -54,6 +54,7 @@ namespace MinecraftClient private readonly List bots = new(); private static readonly List botsOnHold = new(); private static readonly Dictionary inventories = new(); + private static readonly HashSet inventoriesWithFullContents = new(); private readonly Dictionary unlockedRecipes = new(StringComparer.Ordinal); private readonly Dictionary achievements = new(StringComparer.Ordinal); private string? activeAdvancementTab; @@ -2067,12 +2068,12 @@ namespace MinecraftClient firstWindowSlot = -1; lastWindowSlot = -1; - if (inventory.Type == ContainerType.PlayerInventory) + if (inventory.Type is ContainerType.PlayerInventory or ContainerType.Unknown) return false; const int mirroredPlayerInventorySlotCount = 36; int slotCount = inventory.Type.SlotCount(); - if (slotCount < mirroredPlayerInventorySlotCount) + if (slotCount <= mirroredPlayerInventorySlotCount) return false; firstWindowSlot = slotCount - mirroredPlayerInventorySlotCount; @@ -2080,20 +2081,6 @@ namespace MinecraftClient return true; } - private static bool TryGetMirroredPlayerInventorySlot(Container inventory, int windowSlot, out int playerInventorySlot) - { - playerInventorySlot = -1; - - if (!TryGetMirroredPlayerInventoryRange(inventory, out int firstWindowSlot, out int lastWindowSlot)) - return false; - - if (windowSlot < firstWindowSlot || windowSlot > lastWindowSlot) - return false; - - playerInventorySlot = windowSlot - firstWindowSlot + 9; - return true; - } - private static bool AreSameInventorySlot(Item? left, Item? right) { if (left is null || left.IsEmpty) @@ -2113,41 +2100,67 @@ namespace MinecraftClient if (!inventories.TryGetValue(0, out Container? playerInventory)) return false; + if (item is null || item.IsEmpty) + return playerInventory.Items.Remove(playerInventorySlot); + + Item itemClone = item.CloneWithCount(item.Count); + playerInventory.Items.TryGetValue(playerInventorySlot, out Item? previousItem); - if (AreSameInventorySlot(previousItem, item)) + if (AreSameInventorySlot(previousItem, itemClone)) return false; - if (item is null || item.IsEmpty) - playerInventory.Items.Remove(playerInventorySlot); - else - playerInventory.Items[playerInventorySlot] = item; + playerInventory.Items[playerInventorySlot] = itemClone; return true; } - private bool SyncPlayerInventorySlotFromWindow(Container? inventory, int windowSlot) - { - if (inventory is null) - return false; - - if (!TryGetMirroredPlayerInventorySlot(inventory, windowSlot, out int playerInventorySlot)) - return false; - - inventory.Items.TryGetValue(windowSlot, out Item? item); - return SetPlayerInventorySlot(playerInventorySlot, item); - } - private bool SyncPlayerInventorySlotsFromWindow(Container? inventory) { if (inventory is null) return false; + if (!inventoriesWithFullContents.Contains(inventory.ID)) + return false; + if (!TryGetMirroredPlayerInventoryRange(inventory, out int firstWindowSlot, out int lastWindowSlot)) return false; - bool changed = false; + if (!inventories.TryGetValue(0, out Container? playerInventory)) + return false; + + const int firstPlayerInventorySlot = 9; + const int lastPlayerInventorySlot = firstPlayerInventorySlot + 36 - 1; + Dictionary mirroredItems = new(); + for (int windowSlot = firstWindowSlot; windowSlot <= lastWindowSlot; windowSlot++) - changed |= SyncPlayerInventorySlotFromWindow(inventory, windowSlot); + { + if (!inventory.Items.TryGetValue(windowSlot, out Item? item) || item.IsEmpty) + continue; + + int playerInventorySlot = windowSlot - firstWindowSlot + firstPlayerInventorySlot; + mirroredItems[playerInventorySlot] = item.CloneWithCount(item.Count); + } + + bool changed = false; + for (int playerInventorySlot = firstPlayerInventorySlot; playerInventorySlot <= lastPlayerInventorySlot; playerInventorySlot++) + { + playerInventory.Items.TryGetValue(playerInventorySlot, out Item? previousItem); + mirroredItems.TryGetValue(playerInventorySlot, out Item? mirroredItem); + if (AreSameInventorySlot(previousItem, mirroredItem)) + continue; + + changed = true; + break; + } + + if (!changed) + return false; + + for (int playerInventorySlot = firstPlayerInventorySlot; playerInventorySlot <= lastPlayerInventorySlot; playerInventorySlot++) + playerInventory.Items.Remove(playerInventorySlot); + + foreach ((int playerInventorySlot, Item item) in mirroredItems) + playerInventory.Items[playerInventorySlot] = item; return changed; } @@ -2945,7 +2958,10 @@ namespace MinecraftClient if (inventories.ContainsKey(windowId)) { if (windowId != 0) + { inventories.Remove(windowId); + inventoriesWithFullContents.Remove(windowId); + } bool result = handler.SendCloseWindow(windowId); DispatchBotEvent(bot => bot.OnInventoryClose(windowId)); return result; @@ -2966,6 +2982,7 @@ namespace MinecraftClient return InvokeOnMainThread(ClearInventories); inventories.Clear(); + inventoriesWithFullContents.Clear(); inventories[0] = new Container(0, ContainerType.PlayerInventory, "Player Inventory"); ClearUnlockedRecipes(); return true; @@ -3778,6 +3795,7 @@ namespace MinecraftClient /// Inventory ID public void OnInventoryOpen(int inventoryID, Container inventory) { + inventoriesWithFullContents.Remove(inventoryID); inventories[inventoryID] = inventory; if (inventoryID != 0) @@ -3804,9 +3822,15 @@ namespace MinecraftClient if (inventories.ContainsKey(inventoryID)) { if (inventoryID == 0) + { inventories[0].Items.Clear(); // Don't delete player inventory + inventoriesWithFullContents.Clear(); + } else + { inventories.Remove(inventoryID); + inventoriesWithFullContents.Remove(inventoryID); + } } if (inventoryID != 0) @@ -3938,6 +3962,7 @@ namespace MinecraftClient inventories[inventoryID].Items = itemList; inventories[inventoryID].StateID = stateId; + inventoriesWithFullContents.Add(inventoryID); bool playerInventoryChanged = SyncPlayerInventorySlotsFromWindow(inventories[inventoryID]); if (playerInventoryChanged) DispatchBotEvent(bot => bot.OnInventoryUpdate(0)); @@ -3965,7 +3990,7 @@ namespace MinecraftClient inventoryID = 0; // Prevent key not found for some bots relied to this event if (inventories.ContainsKey(0)) { - if (item is not null) + if (item is not null && !item.IsEmpty) inventories[0].Items[-1] = item; else inventories[0].Items.Remove(-1); @@ -3982,7 +4007,7 @@ namespace MinecraftClient } else inventories[inventoryID].Items[slotID] = item; - if (SyncPlayerInventorySlotFromWindow(inventories[inventoryID], slotID)) + if (SyncPlayerInventorySlotsFromWindow(inventories[inventoryID])) DispatchBotEvent(bot => bot.OnInventoryUpdate(0)); } } diff --git a/inventory-sync-issue-report.md b/inventory-sync-issue-report.md new file mode 100644 index 00000000..2d743820 --- /dev/null +++ b/inventory-sync-issue-report.md @@ -0,0 +1,377 @@ +# 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. From b1d02edd9922e9929899d9ada4bc0eaf5ba7cfad Mon Sep 17 00:00:00 2001 From: Anon Date: Sat, 6 Jun 2026 10:27:20 +0200 Subject: [PATCH 4/4] 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.