More inventory features #495
Conversation
WalkthroughRefactors container slot resolution to a SlotDifference value type and GetSlot API; adds a range indexer and changes Viewers initializer. Implements crafting result caching. Reworks inventory click handling (QuickMove, Clone, Throw, Pickup flows) and updates packet handling to use GetSlot/GetItem. Adds ItemStack equality operators and nullable annotation, test fakes, unit tests, and exposes internals to tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Server
participant Player
participant BaseContainer
participant Handler as MainEventHandler
participant CraftingTable
Client->>Server: ContainerClickPacket(ClickedSlot, Button, ClickType)
Server->>BaseContainer: GetSlot(ClickedSlot) => SlotDifference(slot, forPlayer)
Server->>Handler: OnInventoryClick(args)
rect rgba(220,235,255,0.35)
note right of Handler: Dispatch by ClickType
alt QuickMove
Handler->>Handler: HandleQuickMove(args)
Handler->>Player: TryMoveItem(from↔inventory)
else Clone (Creative)
Handler->>Handler: HandleClone(args)
Handler->>Player: Set CarriedItem to full stack
else Throw
Handler->>Handler: HandleThrow(args)
Handler->>Handler: ThrowItem(...)
opt not test
Handler->>Server: SpawnThrownItem(player, thrownItem)
end
else Pickup / PickupAll
Handler->>BaseContainer: GetItem(slot)
Handler->>Player: Merge with CarriedItem (+= / -=)
Handler->>BaseContainer: SetItem(slot, updated)
end
end
opt Crafting result update
Handler->>CraftingTable: SetResult(result)
end
Handler-->>Server: Apply inventory updates
Server-->>Client: Sync packets
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
api
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
Obsidian/Net/Packets/Play/Serverbound/ContainerClickPacket.cs (1)
71-79: Fix slot mapping and changed-slot validation (off-by-one and wrong container).
- Compute mapping against the base (opened) container, not the possibly re-assigned container.
- Use GetSlot for each ChangedSlots entry; the current “> container.Size” check is both fragile and wrong for boundary cases (clickedSlot == Size). This can cause out-of-range access and desync.
Apply:
- var container = player.OpenedContainer ?? player.Inventory; - - var (slot, forPlayer) = container.GetSlot(ClickedSlot); - - if (this.IsPlayerInventory || forPlayer) - container = player.Inventory; - - var clickedItem = slot != -999 ? container[slot] : null; + var baseContainer = player.OpenedContainer ?? player.Inventory; + var (slot, forPlayer) = baseContainer.GetSlot(ClickedSlot); + var container = (this.IsPlayerInventory || forPlayer) ? player.Inventory : baseContainer; + var clickedItem = slot != -999 ? container.GetItem(slot) : null;- foreach (var (changedSlot, hashedItem) in this.ChangedSlots) - { - var currentContainer = changedSlot > container.Size || forPlayer ? player.Inventory : player.OpenedContainer; - - var checkedItem = currentContainer[changedSlot]; - - if (hashedItem == null) - { - currentContainer.RemoveItem(changedSlot); - continue; - } - - if (checkedItem != null && !hashedItem.Compare(checkedItem)) - invalidItems.Add(changedSlot, hashedItem); - } + foreach (var (changedSlot, hashedItem) in this.ChangedSlots) + { + var (mappedSlot, isPlayerSlot) = baseContainer.GetSlot(changedSlot); + var currentContainer = isPlayerSlot ? player.Inventory : baseContainer; + + if (hashedItem == null) + { + currentContainer.RemoveItem(mappedSlot); + continue; + } + + var checkedItem = currentContainer.GetItem(mappedSlot); + if (checkedItem != null && !hashedItem.Compare(checkedItem)) + invalidItems.Add(changedSlot, hashedItem); + }Also applies to: 95-110
Obsidian/Net/Packets/Play/Serverbound/SetCreativeModeSlotPacket.cs (1)
25-38: Held-item equipment update never fires after GetSlot change.The comparison uses ClickedSlot (window slot), but you set the item at slot (inventory slot). Hotbar indices are 36–44; CurrentHeldItemSlot is 0–8.
Apply:
- var (slot, isForPlayer) = inventory.GetSlot(ClickedSlot); + var (slot, isForPlayer) = inventory.GetSlot(ClickedSlot); if (isForPlayer) inventory = player.Inventory; inventory.SetItem(slot, ClickedItem); - if (player.CurrentHeldItemSlot == ClickedSlot) + // If we updated the held hotbar slot, broadcast equipment. + if (isForPlayer && slot >= 36 && slot <= 44 && (slot - 36) == player.CurrentHeldItemSlot) { var heldItem = player.GetHeldItem();Obsidian/Events/MainEventHandler.Inventory.cs (2)
76-86: Clear crafting result via API, not by removing slot 9.Result is now cached via CraftingTable.SetResult. Ensure it’s cleared when no recipe is found to avoid stale outputs.
if (recipe is null) { - if (container[9] != null) - container.RemoveItem(9); + table.SetResult(null); await player.Client.QueuePacketAsync(new ContainerSetSlotPacket { Slot = 0, ContainerId = player.CurrentContainerId, SlotData = null });Also applies to: 60-71
438-446: Possible NRE: currentItem can be null.Use IsNullOrAir() to handle null safely.
- if (carriedItem != null) + if (carriedItem != null) { - if (currentItem.IsAir) + if (currentItem.IsNullOrAir()) container.RemoveItem(clickedSlot); else container.SetItem(clickedSlot, currentItem);
🧹 Nitpick comments (12)
Obsidian.API/Containers/BaseContainer.cs (4)
21-21: Range indexer returns a copy, not a live view. Document or expose an API that makes this clear.Callers might expect mutations to reflect back into the container. Consider renaming or documenting the copy semantics; alternatively expose a method to set a range.
120-129: Doc comment is outdated; return type is SlotDifference (slot + forPlayer).Update XML docs to reflect both values and consider validating boundary cases (e.g., clickedSlot == Size).
Example doc tweak:
- /// <returns>The actual slot number for the given clicked slot.</returns> + /// <returns>A SlotDifference containing the resolved slot and whether it maps to the player's inventory.</returns>
140-147: Manual Deconstruct likely redundant for record struct.Record structs synthesize Deconstruct for primary constructor members. You can remove this method unless you’re customizing names/behavior.
-public readonly record struct SlotDifference(short Slot, bool ForPlayer) -{ - public void Deconstruct(out short slot, out bool forPlayer) - { - slot = this.Slot; - forPlayer = this.ForPlayer; - } -} +public readonly record struct SlotDifference(short Slot, bool ForPlayer);
18-18: Add explicit C# language version to Obsidian.API project
To enable the C# 12 list-expression syntax ([]), add a<LangVersion>entry in Obsidian.API/Obsidian.API.csproj—e.g.:<PropertyGroup> … <LangVersion>latestMajor</LangVersion> </PropertyGroup>Alternatively, ensure your CI uses the .NET 9 SDK (which defaults to a C# version ≥ 12).
Obsidian.Tests/Player.cs (1)
1-149: Nice coverage; consider two more tests.
- QuickMove from player slot to container while a window is open (regresses the from==to bug).
- Pickup-left outside: verify entity spawn path (can run in “test mode” to assert non-null thrown result).
I can add these tests targeting the fixed QuickMove and Throw paths if you want.
Obsidian.Tests/Fakes/FakePlayer.cs (7)
29-29: Avoid C# 12 target-typed collection for broader compatibilityUse an explicit constructor to keep the test project flexible on LangVersion.
- public List<short> DraggedSlots { get; } = []; + public List<short> DraggedSlots { get; } = new List<short>();
13-13: Make CurrentContainerId settable for testsAllow tests to switch container contexts without redefining the fake.
- public byte CurrentContainerId => 0; + public byte CurrentContainerId { get; set; } = 0;
167-167: Implement SetGamemodeAsync as a simple setterKeeps behavior consistent and avoids unnecessary exceptions.
- public ValueTask SetGamemodeAsync(Gamemode gamemode) => throw new NotImplementedException(); + public ValueTask SetGamemodeAsync(Gamemode gamemode) + { + this.Gamemode = gamemode; + return ValueTask.CompletedTask; + }
137-139: Return sensible defaults for held/offhand itemsPrevents throws if tests read these.
- public ItemStack GetHeldItem() => throw new NotImplementedException(); + public ItemStack GetHeldItem() => this.Inventory.GetItem(this.CurrentHeldItemSlot) ?? ItemStack.Air; @@ - public ItemStack GetOffHandItem() => throw new NotImplementedException(); + public ItemStack GetOffHandItem() => ItemStack.Air;
84-84: Give Uuid a default valueUseful for server player registry tests; avoids default(Guid) collisions.
- public Guid Uuid { get; set; } + public Guid Uuid { get; set; } = Guid.NewGuid();
20-21: LoadedChunks should not throwIf any code enumerates it, throwing will surprise tests. Prefer an empty set.
- public ConcurrentHashSet<long> LoadedChunks => throw new NotImplementedException(); + public ConcurrentHashSet<long> LoadedChunks { get; } = new ConcurrentHashSet<long>();
158-166: Most messaging/networking methods can be no-opsIf not explicitly asserted in tests, return CompletedTask to keep fakes lightweight.
- public ValueTask SendActionBarAsync(string text) => throw new NotImplementedException(); + public ValueTask SendActionBarAsync(string text) => ValueTask.CompletedTask; @@ - public ValueTask SendMessageAsync(ChatMessage message) => throw new NotImplementedException(); + public ValueTask SendMessageAsync(ChatMessage message) => ValueTask.CompletedTask; @@ - public ValueTask SendMessageAsync(ChatMessage message, Guid sender, SecureMessageSignature messageSignature) => throw new NotImplementedException(); + public ValueTask SendMessageAsync(ChatMessage message, Guid sender, SecureMessageSignature messageSignature) => ValueTask.CompletedTask; @@ - public ValueTask SendPlayerInfoAsync() => throw new NotImplementedException(); + public ValueTask SendPlayerInfoAsync() => ValueTask.CompletedTask; @@ - public ValueTask SendSoundAsync(ISoundEffect soundEffect) => throw new NotImplementedException(); + public ValueTask SendSoundAsync(ISoundEffect soundEffect) => ValueTask.CompletedTask; @@ - public ValueTask SendSubtitleAsync(ChatMessage subtitle, int fadeIn, int stay, int fadeOut) => throw new NotImplementedException(); + public ValueTask SendSubtitleAsync(ChatMessage subtitle, int fadeIn, int stay, int fadeOut) => ValueTask.CompletedTask; @@ - public ValueTask SendTitleAsync(ChatMessage title, int fadeIn, int stay, int fadeOut) => throw new NotImplementedException(); + public ValueTask SendTitleAsync(ChatMessage title, int fadeIn, int stay, int fadeOut) => ValueTask.CompletedTask; @@ - public ValueTask SendTitleAsync(ChatMessage title, ChatMessage subtitle, int fadeIn, int stay, int fadeOut) => throw new NotImplementedException(); + public ValueTask SendTitleAsync(ChatMessage title, ChatMessage subtitle, int fadeIn, int stay, int fadeOut) => ValueTask.CompletedTask; @@ - public ValueTask SetActionBarTextAsync(ChatMessage message) => throw new NotImplementedException(); + public ValueTask SetActionBarTextAsync(ChatMessage message) => ValueTask.CompletedTask; @@ - public ValueTask UpdateDisplayNameAsync(string newDisplayName) => throw new NotImplementedException(); + public ValueTask UpdateDisplayNameAsync(string newDisplayName) => ValueTask.CompletedTask; @@ - public ValueTask UpdatePlayerInfoAsync() => throw new NotImplementedException(); + public ValueTask UpdatePlayerInfoAsync() => ValueTask.CompletedTask;Also applies to: 171-184
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
Obsidian.API/Containers/BaseContainer.cs(3 hunks)Obsidian.API/Containers/CraftingTable.cs(2 hunks)Obsidian.API/Events/ContainerClickEventArgs.cs(1 hunks)Obsidian.API/Inventory/ItemStack.cs(3 hunks)Obsidian.API/Utilities/Extensions.cs(2 hunks)Obsidian.Tests/Fakes/FakePlayer.cs(1 hunks)Obsidian.Tests/Fakes/FakeServer.cs(1 hunks)Obsidian.Tests/Player.cs(1 hunks)Obsidian/Events/MainEventHandler.Inventory.cs(9 hunks)Obsidian/InternalsVisibleTo.cs(1 hunks)Obsidian/Net/Packets/Play/Serverbound/ContainerClickPacket.cs(1 hunks)Obsidian/Net/Packets/Play/Serverbound/SetCreativeModeSlotPacket.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
Obsidian.API/Containers/CraftingTable.cs (1)
Obsidian.API/Containers/BaseContainer.cs (3)
ItemStack(64-64)ItemStack(157-157)SetResult(155-155)
Obsidian.Tests/Fakes/FakeServer.cs (2)
Obsidian.API/Configuration/ServerConfiguration.cs (1)
ServerConfiguration(5-108)Obsidian.Tests/Fakes/FakePlayer.cs (6)
Task(140-140)Task(151-151)Task(155-155)Task(156-156)Task(157-157)Task(181-181)
Obsidian.API/Utilities/Extensions.cs (4)
Obsidian.API/Containers/BaseContainer.cs (2)
ItemStack(64-64)ItemStack(157-157)Obsidian.API/Inventory/ItemStack.cs (3)
ItemStack(9-109)ItemStack(28-34)ItemStack(36-36)Obsidian.API/_Interfaces/IPlayer.cs (2)
ItemStack(131-131)ItemStack(132-132)Obsidian/Entities/Player.cs (2)
ItemStack(187-187)ItemStack(188-188)
Obsidian.API/Events/ContainerClickEventArgs.cs (2)
Obsidian.API/_Interfaces/IServer.cs (3)
IPlayer(58-58)IPlayer(59-59)IPlayer(60-60)Obsidian.API/Containers/BaseContainer.cs (3)
BaseContainer(6-138)BaseContainer(23-23)BaseContainer(25-30)
Obsidian/Events/MainEventHandler.Inventory.cs (5)
Obsidian.API/Containers/BaseContainer.cs (11)
SetResult(155-155)BaseContainer(6-138)BaseContainer(23-23)BaseContainer(25-30)RemoveItem(66-74)RemoveItem(76-88)RemoveItem(90-103)RemoveItem(105-118)SetItem(62-62)ItemStack(64-64)ItemStack(157-157)Obsidian.API/Containers/CraftingTable.cs (2)
SetResult(17-17)ItemStack(19-19)Obsidian.API/Events/ContainerClickEventArgs.cs (3)
ContainerClickEventArgs(5-65)ContainerClickEventArgs(39-39)ContainerClickEventArgs(46-64)Obsidian.API/Utilities/Extensions.cs (1)
IsNullOrAir(14-14)Obsidian.API/Inventory/ItemStack.cs (3)
ItemStack(9-109)ItemStack(28-34)ItemStack(36-36)
Obsidian.API/Inventory/ItemStack.cs (3)
Obsidian.API/Containers/BaseContainer.cs (2)
ItemStack(64-64)ItemStack(157-157)Obsidian.API/Registries/ItemsRegistry.cs (3)
ItemStack(16-16)ItemStack(18-18)ItemStack(20-20)Obsidian.API/Inventory/DataComponents/DataComponentsStorage.cs (1)
ContainsKey(42-42)
Obsidian.Tests/Player.cs (6)
Obsidian/Events/MainEventHandler.Inventory.cs (3)
MainEventHandler(10-484)ItemStack(289-303)OnInventoryClickTest(455-483)Obsidian.Tests/Fakes/FakeServer.cs (1)
FakeServer(12-62)Obsidian.Tests/Fakes/FakePlayer.cs (3)
FakePlayer(11-185)ItemStack(137-137)ItemStack(139-139)Obsidian.API/Containers/BaseContainer.cs (3)
ItemStack(64-64)ItemStack(157-157)SetItem(62-62)Obsidian.API/Inventory/ItemStack.cs (3)
ItemStack(9-109)ItemStack(28-34)ItemStack(36-36)Obsidian.API/Events/ContainerClickEventArgs.cs (3)
ContainerClickEventArgs(5-65)ContainerClickEventArgs(39-39)ContainerClickEventArgs(46-64)
Obsidian.Tests/Fakes/FakePlayer.cs (3)
Obsidian.API/Containers/BaseContainer.cs (5)
BaseContainer(6-138)BaseContainer(23-23)BaseContainer(25-30)ItemStack(64-64)ItemStack(157-157)Obsidian.API/Inventory/ItemStack.cs (3)
ItemStack(9-109)ItemStack(28-34)ItemStack(36-36)Obsidian/Events/MainEventHandler.Inventory.cs (2)
ItemStack(289-303)ValueTask(50-87)
🔇 Additional comments (5)
Obsidian/InternalsVisibleTo.cs (1)
3-3: No strong-naming detected — PublicKey not required
Searches across all *.csproj and *.props files found no<SignAssembly>,<AssemblyOriginatorKeyFile>,<PublicKey>or legacy signing attributes, so this assembly isn’t strong-named and no PublicKey is needed in InternalsVisibleTo.Obsidian.API/Containers/CraftingTable.cs (2)
7-8: Clarify result lifecycle: when is it invalidated/refreshed?Caching
resultdecouples it from slot 9. Ensure it’s cleared/updated whenever the grid changes to avoid stale outputs.Do you want a helper on the container to invalidate
resultwhen inputs mutate?
17-20: Verify removal of all legacy slot-9 calls
rgreturned no matches forSetItem(9, …)orGetItem(9)—manually inspect container/UI/serialization code to ensure no numeric slot-9 references remain.Obsidian.API/Events/ContainerClickEventArgs.cs (1)
46-64: Factory looks good.The Create(...) helper keeps tests clean and correctly sets required fields.
Obsidian.Tests/Fakes/FakeServer.cs (1)
38-43: No-op properties unused—no implementation needed
Tests don’t reference CommandHandler, ScoreboardManager, or EventDispatcher, so leaving them throwing NotImplementedException is acceptable.
Tides
added
epic
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Obsidian/Events/MainEventHandler.Inventory.cs (2)
221-250: Guard QuickCraft finalize with IsNullOrAir and fix merge into existing stacks.Current code subtracts from carried but does not increment existing items in target slots; items disappear.
Apply:
- if (!player.IsDragging && player.CarriedItem != null) + if (!player.IsDragging && !player.CarriedItem.IsNullOrAir()) @@ - for (var i = 0; i < player.DraggedSlots.Count; i++) + for (var i = 0; i < player.DraggedSlots.Count; i++) { var slotIndex = span[i]; var item = container.GetItem(slotIndex); var amount = Math.Min(perSlot, player.CarriedItem.MaxStackSize - (item?.Count ?? 0)); if (amount > 0) { - container.SetItem(slotIndex, item ?? new(player.CarriedItem, amount)); + if (item is null) + container.SetItem(slotIndex, new(player.CarriedItem, amount)); + else + item += amount; player.CarriedItem -= amount; } if (player.CarriedItem.Count <= 0) break; }
259-264: QuickCraft AddSlotRight/AddSlotMiddle misuse carried instance; fixes needed.
- AddSlotRight assigns the carried object to the slot, then decrements carried, aliasing the same instance.
- AddSlotMiddle fills a full stack but doesn’t adjust carried.
Apply:
- case DraggingState.AddSlotRight: - container.SetItem(clickedSlot, player.CarriedItem); - player.CarriedItem -= 1; + case DraggingState.AddSlotRight: + { + var existing = container.GetItem(clickedSlot); + if (existing.IsNullOrAir()) + { + container.SetItem(clickedSlot, new(player.CarriedItem, 1)); + player.CarriedItem -= 1; + } + else if (existing == player.CarriedItem && existing.Count < existing.MaxStackSize) + { + existing += 1; + player.CarriedItem -= 1; + } + } break; case DraggingState.AddSlotMiddle: - container.SetItem(clickedSlot, new(player.CarriedItem, player.CarriedItem.MaxStackSize)); + { + var existing = container.GetItem(clickedSlot); + var room = (existing?.MaxStackSize ?? player.CarriedItem.MaxStackSize) - (existing?.Count ?? 0); + if (room <= 0) break; + var take = Math.Min(room, player.CarriedItem.Count); + if (existing is null) + container.SetItem(clickedSlot, new(player.CarriedItem, take)); + else + existing += take; + player.CarriedItem -= take; + } break;
♻️ Duplicate comments (3)
Obsidian/Events/MainEventHandler.Inventory.cs (3)
353-360: Pickup outside now spawns a dropped item.Nice fix; this addresses the previously noted omission.
91-103: QuickMove can select the same container for source and target.When clicking a non-player slot, container == player.OpenedContainer, but to is also player.OpenedContainer, so TryMoveItem operates within the same container. Derive target from the actual source instead.
Apply:
- TryMoveItem(container, clickedSlot, player.OpenedContainer ?? player.Inventory); + var from = container; + var to = ReferenceEquals(from, player.Inventory) + ? (player.OpenedContainer ?? player.Inventory) + : player.Inventory; + TryMoveItem(from, clickedSlot, to);
288-302: ThrowItem returns leftover stack instead of thrown amount.This corrupts state and spawns wrong quantities. Return the actual thrown stack and perform bounded subtraction.
Apply:
- private static ItemStack? ThrowItem(IPlayer player, BaseContainer container, short clickedSlot, sbyte button, bool forPlayer = false) + private static ItemStack? ThrowItem(IPlayer player, BaseContainer container, short clickedSlot, sbyte button, bool forPlayer = false) { var amountToRemove = button == 0 ? 1 : 64; - ItemStack? removedItem; + ItemStack? thrown; if (forPlayer) { - player.CarriedItem -= amountToRemove; - removedItem = player.CarriedItem; + if (player.CarriedItem.IsNullOrAir()) + return null; + var take = Math.Min(amountToRemove, player.CarriedItem.Count); + thrown = new ItemStack(player.CarriedItem, take); + player.CarriedItem -= take; } else - container.RemoveItem(clickedSlot, amountToRemove, out removedItem); + { + var src = container.GetItem(clickedSlot); + if (src.IsNullOrAir()) + return null; + var take = Math.Min(amountToRemove, src.Count); + thrown = new ItemStack(src, take); + container.RemoveItem(clickedSlot, take); + } - return removedItem; + return thrown; }
🧹 Nitpick comments (3)
Obsidian/Events/MainEventHandler.Inventory.cs (2)
104-146: Minor cleanups in TryMoveItem; semantics mostly OK.
- No-op: from.SetItem(fromSlot, sourceItem) at the end is unnecessary; sourceItem is already a reference to the slot item.
Apply:
- from.SetItem(fromSlot, sourceItem); return false;
158-212: PickupAll uses value equality; confirm intended matching criteria.Using operator != will match by Holder+components, ignoring Count. If you want type-only matching (e.g., ignore custom names), add an explicit predicate.
I can provide a helper like MatchesForPickupAll(item, carried) if desired.
Obsidian.API/Containers/BaseContainer.cs (1)
21-21: Range indexer returns a copy; ensure callers don’t expect a live view.Array slicing creates a new array; mutations won’t reflect back into the container. Consider documenting this or exposing Span/ReadOnlySpan-based accessors for perf-sensitive paths.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
Obsidian.API/Containers/BaseContainer.cs(3 hunks)Obsidian.API/Inventory/ItemStack.cs(2 hunks)Obsidian.API/Utilities/Extensions.cs(2 hunks)Obsidian.Tests/Player.cs(1 hunks)Obsidian/Events/MainEventHandler.Inventory.cs(11 hunks)Obsidian/Net/Packets/Play/Serverbound/ContainerClickPacket.cs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Obsidian.API/Utilities/Extensions.cs
- Obsidian.Tests/Player.cs
- Obsidian.API/Inventory/ItemStack.cs
🧰 Additional context used
🧬 Code graph analysis (3)
Obsidian/Events/MainEventHandler.Inventory.cs (5)
Obsidian.API/Containers/BaseContainer.cs (11)
SetResult(150-150)BaseContainer(6-140)BaseContainer(23-23)BaseContainer(25-30)RemoveItem(66-74)RemoveItem(76-88)RemoveItem(90-103)RemoveItem(105-118)SetItem(62-62)ItemStack(64-64)ItemStack(152-152)Obsidian.API/Containers/CraftingTable.cs (2)
SetResult(17-17)ItemStack(19-19)Obsidian.API/Events/ContainerClickEventArgs.cs (3)
ContainerClickEventArgs(5-65)ContainerClickEventArgs(39-39)ContainerClickEventArgs(46-64)Obsidian.API/Utilities/Extensions.cs (1)
IsNullOrAir(14-14)Obsidian.API/Inventory/ItemStack.cs (3)
ItemStack(7-107)ItemStack(26-32)ItemStack(34-34)
Obsidian.API/Containers/BaseContainer.cs (1)
Obsidian.API/Inventory/ItemStack.cs (3)
ItemStack(7-107)ItemStack(26-32)ItemStack(34-34)
Obsidian/Net/Packets/Play/Serverbound/ContainerClickPacket.cs (1)
Obsidian.API/Containers/BaseContainer.cs (4)
RemoveItem(66-74)RemoveItem(76-88)RemoveItem(90-103)RemoveItem(105-118)
🔇 Additional comments (11)
Obsidian/Net/Packets/Play/Serverbound/ContainerClickPacket.cs (5)
2-2: Import aligns with API evolution.Adding the Obsidian.API import is consistent with the new GetSlot/SlotDifference usage.
72-79: Container/slot resolution looks correct.Deriving (slot, forPlayer) via baseContainer.GetSlot and then choosing the actual container avoids mutating baseContainer and keeps event source accurate. The -999 passthrough is preserved in clickedItem.
91-92: Confirm SetSlot StateId handling.StateId = 0 may be intentional to force a hard resync, but some clients expect the current StateId echo. Please confirm protocol expectations for your server/client mix.
Would you like a quick script to grep for other SetSlot usages to ensure consistent StateId semantics?
98-107: ChangedSlots mapping is consistent with GetSlot.Mapping each changedSlot through baseContainer.GetSlot and operating on the resolved container/slot prevents cross-inventory mismatches. Good use of GetItem/RemoveItem APIs.
126-133: Event dispatch passes resolved slot/container.Passing the resolved slot with the derived container keeps downstream handlers simple. Note that ContainerClickEventArgs.IsPlayerInventory reflects ContainerId==0 (window vs. player inv), not whether the clicked slot was in the player area—handlers should not rely on it for target derivation.
Obsidian/Events/MainEventHandler.Inventory.cs (3)
38-42: New handlers wired into switch.QuickMove and Clone are now reachable. Nice.
270-286: Throw outside inventory: behavior likely incomplete.Returning early on OutsideInventory skips throwing from the carried stack for ClickType.Throw. Confirm intended behavior; parity with Pickup’s outside drop may be expected here too.
If needed, I can draft the outside-throw branch to remove from carried and SpawnThrownItem.
455-483: Test entrypoint mirrors runtime switch.OnInventoryClickTest matches the dispatcher, good for unit coverage.
Obsidian.API/Containers/BaseContainer.cs (3)
18-19: Modernized empty collection initializer.Viewer list initializer with [] is fine and concise.
142-143: SlotDifference record is a good fit for deconstruction and clarity.Nice API ergonomics.
1-154: No stale GetDifference references remain. Rename to GetSlot is complete.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Obsidian.API/Containers/BaseContainer.cs (1)
130-131: Incorrect mapping for player hotbar (last 9 slots) — implement piecewise mapping.Current logic maps Size+27..Size+35 to 36..44, which is outside the 0..35 player inventory range and will misroute clicks. Use the standard two-range mapping (main 27 → 9..35, hotbar 9 → 0..8).
Apply:
- public virtual SlotDifference GetSlot(short clickedSlot) => - clickedSlot >= this.Size ? new((short)(clickedSlot - this.Size + 9), true) : new(clickedSlot, false); + public virtual SlotDifference GetSlot(short clickedSlot) + { + // Negative (e.g., -999 outside): do not map; caller must handle before indexing. + if (clickedSlot < 0) + return new(clickedSlot, false); + + // Container area + if (clickedSlot < this.Size) + return new(clickedSlot, false); + + // Player area appended after container + int rel = clickedSlot - this.Size; // 0..35 + // Main inventory (27): map to 9..35 + if (rel < 27) + return new((short)(rel + 9), true); + // Hotbar (9): map to 0..8 + return new((short)(rel - 27), true); + }Please add tests for:
- clickedSlot: Size-1 → (Slot=Size-1, ForPlayer=false)
- Size → (9, true), Size+26 → (35, true)
- Size+27 → (0, true), Size+35 → (8, true)
- -999 → (-999, false)
🧹 Nitpick comments (4)
Obsidian.API/Containers/BaseContainer.cs (4)
21-21: Range indexer returns a copied array; confirm this allocation and copy semantics are intentional.x[range] on arrays creates a new array. If you intended a non-allocating view, expose ReadOnlySpan<ItemStack?> or ArraySegment<ItemStack?> instead.
Example:
- public ItemStack?[] this[Range range] { get => this.items[range]; } + public ArraySegment<ItemStack?> this[Range range] + { + get + { + var (start, length) = range.GetOffsetAndLength(this.items.Length); + return new ArraySegment<ItemStack?>(this.items, start, length); + } + }
120-129: Clarify docs: describe player-main vs hotbar mapping and negative sentinel slots.Document that: (a) slots < Size map to the container; (b) Size..Size+26 map to player main inventory (9..35); (c) Size+27..Size+35 map to hotbar (0..8); (d) negative slots (e.g., -999) are not mapped and should be handled by the caller (throw/“outside” actions).
Proposed doc tweak:
-/// Gets the slot difference for the given clicked slot. +/// Maps a window (clicked) slot index to a concrete slot in either the container or the player's inventory. ... -/// A SlotDifference containing the resolved slot and whether it maps to the player's inventory. +/// A SlotDifference containing: +/// - Slot: the resolved slot index (container: 0..Size-1; player main: 9..35; hotbar: 0..8). +/// - ForPlayer: true if the slot resolves to the player's inventory (main/hotbar), false for the container. ... -/// This method is used to determine the actual slot number for a clicked slot in a container. +/// Mapping rules: +/// - clickedSlot < Size => container slot (Slot = clickedSlot, ForPlayer = false) +/// - Size .. Size+26 => player main 27 slots (Slot = clickedSlot - Size + 9, ForPlayer = true) +/// - Size+27 .. Size+35 => player hotbar 9 slots (Slot = clickedSlot - (Size + 27), ForPlayer = true) +/// - clickedSlot < 0 => special/“outside” clicks; returned as-is (caller must handle before indexing)
142-142: Type addition looks good; consider adding XML docs for fields.Record struct is appropriate. Optional: document Slot and ForPlayer, or rename ForPlayer → IsPlayerInventory for clarity.
18-18: Refactor Viewers to IReadOnlyList with controlled mutators and specify LangVersion
- Change
public List<IPlayer> Viewers { get; } = [];(BaseContainer.cs:18) to:private readonly List<IPlayer> _viewers = new(); public IReadOnlyList<IPlayer> Viewers => _viewers; public void AddViewer(IPlayer player) => _viewers.Add(player); public bool RemoveViewer(IPlayer player) => _viewers.Remove(player);- Obsidian.API.csproj targets net9.0 with
<EnablePreviewFeatures>true</EnablePreviewFeatures>but lacks<LangVersion>; add<LangVersion>12.0</LangVersion>(orpreview) to enable C# 12 collection expressions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
Obsidian.API/Containers/BaseContainer.cs(3 hunks)
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Obsidian/Events/MainEventHandler.Inventory.cs (1)
221-254: Guard QuickCraft paths and forbid Creative-only clone in Survival.
- Ensure QuickCraft right/left phases only run when
player.CarriedItemis non-null.- Middle-add currently clones a full stack without mode checks, enabling dupes in Survival.
Apply:
- if (!player.IsDragging && !player.CarriedItem.IsNullOrAir()) + if (!player.IsDragging && !player.CarriedItem.IsNullOrAir()) { ... } ... switch (state) { case DraggingState.AddSlotLeft: ... break; case DraggingState.AddSlotRight: - var existingItem = container.GetItem(clickedSlot); + if (player.CarriedItem.IsNullOrAir()) + break; + var existingItem = container.GetItem(clickedSlot); ... break; case DraggingState.AddSlotMiddle: - container.SetItem(clickedSlot, new(player.CarriedItem, player.CarriedItem.MaxStackSize)); + if (player.Gamemode == Gamemode.Creative && !player.CarriedItem.IsNullOrAir()) + container.SetItem(clickedSlot, new(player.CarriedItem, player.CarriedItem.MaxStackSize)); break; default: break; }Also applies to: 263-275, 277-279
♻️ Duplicate comments (2)
Obsidian/Events/MainEventHandler.Inventory.cs (2)
91-102: Fix QuickMove destination: from==to causes no-op or self-merge.When clicking inside a container (e.g., chest),
player.OpenedContainerequalscontainer, sotobecomes the same asfrom. Shift-click should move between the active container and the player inventory.Apply:
private static void HandleQuickMove(ContainerClickEventArgs args) { var container = args.Container; var player = args.Player; var clickedSlot = args.ClickedSlot; var clickedItem = args.Item; if (clickedItem.IsNullOrAir()) return; - TryMoveItem(container, clickedSlot, player.OpenedContainer ?? player.Inventory); + var from = container; + var to = ReferenceEquals(from, player.Inventory) + ? (player.OpenedContainer ?? player.Inventory) + : player.Inventory; + if (ReferenceEquals(from, to)) + return; + TryMoveItem(from, clickedSlot, to); }
303-317: ThrowItem returns the leftover stack; return the thrown amount instead and fix amount rules.Currently spawns wrong quantities and corrupts state. Also, outside-drop (forPlayer=true) should drop all on left-click and one on right-click; Q/ctrl+Q should drop one/all from the slot.
Apply:
- private static ItemStack? ThrowItem(IPlayer player, BaseContainer container, short clickedSlot, sbyte button, bool forPlayer = false) + private static ItemStack? ThrowItem(IPlayer player, BaseContainer container, short clickedSlot, sbyte button, bool forPlayer = false) { - var amountToRemove = button == 0 ? 1 : 64; - - ItemStack? removedItem; - if (forPlayer) - { - player.CarriedItem -= amountToRemove; - removedItem = player.CarriedItem; - } - else - container.RemoveItem(clickedSlot, amountToRemove, out removedItem); - - return removedItem; + if (forPlayer) + { + var src = player.CarriedItem; + if (src.IsNullOrAir()) + return null; + var take = button == 0 ? src.Count : 1; // left: drop all; right: drop one + take = Math.Min(take, src.Count); + var thrown = new ItemStack(src, take); + player.CarriedItem -= take; + return thrown; + } + else + { + var src = container.GetItem(clickedSlot); + if (src.IsNullOrAir()) + return null; + var take = button == 0 ? 1 : src.Count; // Q: one, Ctrl+Q: entire stack + take = Math.Min(take, src.Count); + var thrown = new ItemStack(src, take); + container.RemoveItem(clickedSlot, take); + return thrown; + } }
🧹 Nitpick comments (2)
Obsidian/Events/MainEventHandler.Inventory.cs (2)
158-212: PickupAll relies on IsPlayerInventory; prefer identity checks.
args.IsPlayerInventorycan be inconsistent across contexts. Derive intent viaReferenceEquals(args.Container, player.Inventory)to avoid edge cases when a window is open but the actual container is the player inventory.Proposed tweak:
- if (amountNeeded > 0 && !args.IsPlayerInventory) + if (amountNeeded > 0 && !ReferenceEquals(container, player.Inventory))
441-468: Minor: variable naming is misleading in HandleSwap.
carriedItemis actually the clicked-slot item (args.Item). Consider renaming toclickedItemfor clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
Obsidian/Events/MainEventHandler.Inventory.cs(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Obsidian/Events/MainEventHandler.Inventory.cs (5)
Obsidian.API/Containers/BaseContainer.cs (11)
SetResult(150-150)BaseContainer(6-140)BaseContainer(23-23)BaseContainer(25-30)RemoveItem(66-74)RemoveItem(76-88)RemoveItem(90-103)RemoveItem(105-118)SetItem(62-62)ItemStack(64-64)ItemStack(152-152)Obsidian.API/Containers/CraftingTable.cs (2)
SetResult(17-17)ItemStack(19-19)Obsidian.API/Events/ContainerClickEventArgs.cs (3)
ContainerClickEventArgs(5-65)ContainerClickEventArgs(39-39)ContainerClickEventArgs(46-64)Obsidian.API/Utilities/Extensions.cs (1)
IsNullOrAir(14-14)Obsidian.API/Inventory/ItemStack.cs (3)
ItemStack(7-107)ItemStack(26-32)ItemStack(34-34)
🔇 Additional comments (3)
Obsidian/Events/MainEventHandler.Inventory.cs (3)
65-66: Crafting result flow looks good.Switching to CraftingTable.SetResult(...) and mirroring via ContainerSetSlotPacket keeps server state and client view consistent.
Also applies to: 80-88
147-156: Clone behavior matches Creative semantics.Fills carried stack to max from the clicked item; looks correct.
285-301: Throw outside guard is fine; keep.Early-return on OutsideInventory in Throw context is reasonable, since Q/ctrl+Q target an actual slot.
Fully implements all inventory click events
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores