helpy4/Minecraft-Console-Client
1
0
Fork 0
mirror of https://github.com/MCCTeam/Minecraft-Console-Client synced 2026-08-15 13:04:36 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 1

Remove local inventory report from branch

Browse source
This commit is contained in:
Anon 2026-06-06 10:27:20 +02:00
parent 6bfcd9e9ca
commit b1d02edd99
1 changed files with 0 additions and 377 deletions
Download patch file Download diff file Expand all files Collapse all files

377
inventory-sync-issue-report.md
View file

@ -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 <containerSlot> 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 <containerSlot> 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 <containerSlot> 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 <containerSlot> 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.