Conversation
github-actions
bot
added
api
WalkthroughThis update introduces a generic base class for numeric argument parsers, centralizing min/max bound logic and serialization. It adds hashed item stack support with new interfaces and classes, updates inventory event and packet handling to use hashed items and new click types, and revises inventory and player interfaces for improved input and disconnection handling. Inventory click handling is refactored into a dedicated partial event handler with detailed logic per click type. Changes
Sequence Diagram(s)Inventory Click Handling (New Flow)sequenceDiagram
participant Client
participant Server
participant MainEventHandler
participant Player
participant Container
Client->>Server: Send ContainerClickPacket (with ClickType, HashedItems)
Server->>MainEventHandler: OnInventoryClick(args)
MainEventHandler->>Player: Validate carried item vs container slot (hash compare)
alt mismatch
MainEventHandler->>Client: Send ContainerSetSlotPacket (resync)
else match
MainEventHandler->>Container: Apply changes (based on ClickType)
MainEventHandler->>Player: Update inventory/carried item
MainEventHandler->>Client: (If needed) Send ContainerSetContentPacket
end
Hashed Item Stack ComparisonsequenceDiagram
participant HashedItemStack
participant ItemStack
HashedItemStack->>ItemStack: For each component, serialize and hash
HashedItemStack->>HashedItemStack: Compare hashes
alt all hashes match
HashedItemStack-->>Caller: Return true
else any mismatch
HashedItemStack-->>Caller: Return false
end
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code Graph Analysis (1)Obsidian/Net/Packets/Play/Serverbound/ContainerClosePacket.cs (1)
🔇 Additional comments (2)
✨ Finishing Touches
🧪 Generate Unit Tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
♻️ Duplicate comments (40)
Obsidian/Assets/item_components.json (40)
2034-2037: Same concern as 1626-1629 – please confirm property support.
2131-2154: Same metadata gap as the black harness (1706-1729).
2729-2732: See earlier carpet shearing comment.
2826-2849: Same harness-metadata issue raised for black harness.
5158-5168: Same attribute-modifier duplication concern as 3297-3306.
5686-5689: See earlier carpet shearing comment.
5766-5789: Same harness-metadata issue raised for black harness.
7581-7591: Attribute-modifier ID duplication (see 3297-3306).
9548-9552: See horse-armour shearing comment (7140-7144).
9951-9954: See earlier carpet shearing comment.
10031-10054: Same harness-metadata issue raised for black harness.
10214-10217: See earlier carpet shearing comment.
10294-10317: Same harness-metadata issue raised for black harness.
11329-11333: See horse-armour shearing comment (7140-7144).
12325-12329: See horse-armour shearing comment (7140-7144).
12514-12517: See earlier carpet shearing comment.
12594-12617: Same harness-metadata issue raised for black harness.
12777-12780: See earlier carpet shearing comment.
12857-12880: Same harness-metadata issue raised for black harness.
13120-13123: See earlier carpet shearing comment.
13200-13223: Same harness-metadata issue raised for black harness.
13494-13497: See earlier carpet shearing comment.
13574-13597: Same harness-metadata issue raised for black harness.
16220-16223: See earlier carpet shearing comment.
16300-16323: Same harness-metadata issue raised for black harness.
17129-17139: Attribute-modifier ID duplication (see 3297-3306).
17267-17270: See earlier carpet shearing comment.
17347-17370: Same harness-metadata issue raised for black harness.
17535-17545: Attribute-modifier ID duplication (see 3297-3306).
18612-18615: See earlier carpet shearing comment.
18692-18715: Same harness-metadata issue raised for black harness.
19290-19293: See earlier carpet shearing comment.
19370-19393: Same harness-metadata issue raised for black harness.
20634-20644: Attribute-modifier ID duplication (see 3297-3306).
24836-24839: See earlier carpet shearing comment.
24916-24939: Same harness-metadata issue raised for black harness.
25123-25133: Attribute-modifier ID duplication (see 3297-3306).
25635-25638: See earlier carpet shearing comment.
25715-25738: Same harness-metadata issue raised for black harness.
25839-25849: Attribute-modifier ID duplication (see 3297-3306).
🧹 Nitpick comments (16)
Obsidian.API/Inventory/DataComponents/BlockPredicatesDataComponent.cs (1)
14-14: Consider documenting the incomplete implementation.The
WriteHashedmethod throwsNotImplementedException, which could cause runtime issues if called. Consider adding documentation or a TODO comment to indicate this is a placeholder during the hashed item stack refactoring.- public void WriteHashed(INetStreamWriter writer) => throw new NotImplementedException(); + // TODO: Implement hashed serialization for block predicates + public void WriteHashed(INetStreamWriter writer) => throw new NotImplementedException();Obsidian/Net/Packets/Play/Serverbound/PlayerCommandPacket.cs (1)
1-1: Remove unused using directive.The
Microsoft.Extensions.Loggingusing directive is added but no logging calls are present in the code.-using Microsoft.Extensions.Logging;Obsidian/Net/Packets/Play/Serverbound/PlayerInputPacket.cs (1)
15-31: Consider thread safety for concurrent input updates.The packet handling logic is correct, but consider potential race conditions if multiple input packets are processed concurrently for the same player.
The sneaking state change detection and broadcasting logic properly:
- Captures the previous sneaking state
- Updates player input
- Broadcasts
SetEntityDataPacketto other players when sneaking changes- Correctly excludes the player themselves using
player.EntityIdConsider adding synchronization if player input can be updated concurrently:
public override ValueTask HandleAsync(IServer server, IPlayer player) { + lock (player) // or use a more granular lock + { var wasSneaking = player.Sneaking; player.Input = this.Flags; if (player.Sneaking != wasSneaking) { player.World.PacketBroadcaster.QueuePacketToWorld(player.World, new SetEntityDataPacket { EntityId = player.EntityId, Entity = player }, player.EntityId); } + } return default; }Obsidian.API/Commands/ArgumentParsers/EntityArgumentParser.cs (1)
10-15: TODO implementation needed for production readiness.The
TryParseArgumentmethod is currently stubbed and always returns false. This means entity parsing for commands like@p,@a,@e,@rselectors and entity names will not work.Would you like me to help implement the entity parsing logic or create an issue to track this implementation?
Obsidian/Utilities/Extensions.Nbt.Chat.cs (1)
174-178: Appropriate use of NotImplementedException for incomplete features.The explicit exceptions for unimplemented hover content types are better than silent failures. These clearly indicate areas that need implementation.
Would you like me to help implement the missing HoverItemContent and HoverEntityComponent serialization logic?
Obsidian/HashedItemStack.cs (1)
18-43: Consider dynamic buffer sizing for large components.The implementation is solid with proper ArrayPool usage and CRC32 hashing. However, the fixed 256-byte buffer size might be insufficient for large data components.
Consider implementing dynamic buffer sizing:
- var sharedBuffer = ArrayPool<byte>.Shared.Rent(256); + var bufferSize = Math.Max(256, EstimateComponentSize(type)); + var sharedBuffer = ArrayPool<byte>.Shared.Rent(bufferSize);Or handle buffer overflow scenarios gracefully by catching exceptions and retrying with a larger buffer.
Obsidian.API/_Types/BaseArgumentParser.cs (1)
5-5: Track TODO for async parsing support.The TODO comment indicates that async parsing support may be needed in the future. This could be important for parsers that need to perform I/O operations (e.g., database lookups, external API calls).
Would you like me to create an issue to track the implementation of async
TryParseArgumentmethods?Obsidian.SourceGenerators/Registry/RegistryGenerator.cs (1)
17-18: Remove commented debugger code.Commented debugging code should be removed before merging to production.
public void Initialize(IncrementalGeneratorInitializationContext ctx) { - //if (!Debugger.IsAttached) - // Debugger.Launch(); - var jsonFiles = ctx.AdditionalTextsProviderObsidian.API/Events/ContainerClickEventArgs.cs (1)
26-26: Consider adding validation for the required properties.The required property pattern is implemented correctly. However, consider adding validation to ensure
ClickedSlotis within valid bounds for the container size.+private bool IsValidSlot => ClickedSlot >= 0 && ClickedSlot < Container.Size;Also applies to: 28-31, 33-33, 35-35
Obsidian/Assets/tags.json (4)
2348-2354: Minor nit: keep tag names & contents consistentConsider renaming either this tag or the previous one so that “dry vegetation” and “sand” are mutually exclusive, or refactor per the previous comment.
Otherwise LGTM.
3072-3081: Empty dialog tags ship as dead codeBoth
dialog/pause_screen_additionsanddialog/quick_actionsare empty arrays.
If that’s intentional, 👍. If they’re placeholders, consider omitting them to avoid bloating the tag file or add a_todocomment.
3464-3492: followable_friendly_mobs gets big – consider splittingThis tag is starting to become a kitchen-sink list (29 entries).
Long flat arrays are hard to maintain; think about factoring by behaviour (e.g.followable_friendly_mobs/land,/air,/rideable) and composing with#references.
4640-4674: Harness & tempt-item tags – good, but enforce colour orderingGreat to see the colour-complete harness list.
For future diffs, sticking to alphabetical (or dye-order) sorting reduces conflict churn, but that’s just a style hint.Obsidian/Assets/recipes.json (1)
15488-15502: Adding a craftable saddle may break vanilla balanceVanilla intentionally makes saddles non-craftable. Introducing this recipe alters progression and loot-table value.
If that’s intentional, gate it behind a datapack toggle or config flag; otherwise consider removing.Obsidian/Assets/item_components.json (2)
1706-1729: Harness definition is missing core item metadataThe newly-added harness items have no
minecraft:creative_category,minecraft:icon, orminecraft:render_offsets. They will not appear in creative inventory and may render incorrectly in hand.
Consider cloning the saddle definition and trimming unneeded fields instead of copying the minimal template.
3297-3306: Attribute-modifier ID re-use may stack unintentionally
"id": "minecraft:waypoint_transmit_range_hide"is re-used across multiple head items.
If two pieces are worn simultaneously (e.g., carved pumpkin + mob head via armour stand tricks) the engine may merge rather than treat them separately.
Generate per-item UUID-like IDs to be safe.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (71)
Obsidian.API/Commands/ArgumentParsers/BooleanArgumentParser.cs(1 hunks)Obsidian.API/Commands/ArgumentParsers/EntityArgumentParser.cs(1 hunks)Obsidian.API/Commands/ArgumentParsers/LocationArgumentParser.cs(1 hunks)Obsidian.API/Commands/ArgumentParsers/MinecraftTimeArgumentParser.cs(2 hunks)Obsidian.API/Commands/ArgumentParsers/NumericArgumentParsers.Helpers.cs(1 hunks)Obsidian.API/Commands/ArgumentParsers/NumericArgumentParsers.cs(1 hunks)Obsidian.API/Commands/ArgumentParsers/PlayerArgumentParser.cs(1 hunks)Obsidian.API/Commands/ArgumentParsers/StringArgumentParser.cs(1 hunks)Obsidian.API/Events/ContainerClickEventArgs.cs(1 hunks)Obsidian.API/Inventory/DataComponents/BlockPredicatesDataComponent.cs(1 hunks)Obsidian.API/Inventory/DataComponents/ComponentBuilder.cs(2 hunks)Obsidian.API/Inventory/DataComponents/DataComponentsStorage.cs(1 hunks)Obsidian.API/Inventory/ItemStack.cs(1 hunks)Obsidian.API/Obsidian.API.csproj(2 hunks)Obsidian.API/Plugins/IPluginRegistry.cs(1 hunks)Obsidian.API/_Attributes/ArgumentParserAttribute.cs(1 hunks)Obsidian.API/_Enums/ClickType.cs(1 hunks)Obsidian.API/_Enums/EntityType.cs(2 hunks)Obsidian.API/_Enums/PlayerInput.cs(1 hunks)Obsidian.API/_Enums/ProtocolVersion.cs(1 hunks)Obsidian.API/_Interfaces/IHashedItemStack.cs(1 hunks)Obsidian.API/_Interfaces/INetStreamReader.cs(1 hunks)Obsidian.API/_Interfaces/IPlayer.cs(2 hunks)Obsidian.API/_Types/BaseArgumentParser.cs(1 hunks)Obsidian.API/_Types/HexColor.cs(1 hunks)Obsidian.API/_Types/MinecraftTime.cs(2 hunks)Obsidian.Nbt/Obsidian.Nbt.csproj(1 hunks)Obsidian.Nbt/RawNbtWriter.Primitives.cs(2 hunks)Obsidian.Nbt/RawNbtWriter.cs(4 hunks)Obsidian.SourceGenerators/Obsidian.SourceGenerators.csproj(1 hunks)Obsidian.SourceGenerators/Registry/EntityGenerator.cs(0 hunks)Obsidian.SourceGenerators/Registry/Models/Assets.cs(1 hunks)Obsidian.SourceGenerators/Registry/RegistryGenerator.cs(1 hunks)Obsidian.Tests/Commands.cs(1 hunks)Obsidian.Tests/Nbt.cs(1 hunks)Obsidian/Assets/command_parsers.json(1 hunks)Obsidian/Assets/item_components.json(49 hunks)Obsidian/Assets/packets.json(7 hunks)Obsidian/Assets/recipes.json(67 hunks)Obsidian/Assets/tags.json(9 hunks)Obsidian/Commands/CommandNode.cs(3 hunks)Obsidian/Commands/CommandNodeType.cs(1 hunks)Obsidian/Commands/Framework/CommandHandler.cs(3 hunks)Obsidian/Commands/Modules/MainCommandModule.cs(2 hunks)Obsidian/Commands/Parsers/CommandParser.cs(0 hunks)Obsidian/Commands/Parsers/EntityCommandParser.cs(0 hunks)Obsidian/Commands/Parsers/MinecraftTimeParser.cs(0 hunks)Obsidian/Commands/Parsers/NumberCommandParsers.cs(0 hunks)Obsidian/Commands/Parsers/StringCommandParser.cs(0 hunks)Obsidian/Entities/Entity.cs(2 hunks)Obsidian/Entities/Player.Helpers.cs(0 hunks)Obsidian/Entities/Player.cs(3 hunks)Obsidian/Events/MainEventHandler.Inventory.cs(1 hunks)Obsidian/Events/MainEventHandler.cs(2 hunks)Obsidian/HashedItemStack.cs(1 hunks)Obsidian/Net/ClientHandlers/ConfigurationClientHandler.cs(0 hunks)Obsidian/Net/ClientHandlers/PlayClientHandler.cs(2 hunks)Obsidian/Net/NetworkBuffer.Reading.cs(6 hunks)Obsidian/Net/NetworkBuffer.Writing.cs(3 hunks)Obsidian/Net/NetworkBuffer.cs(1 hunks)Obsidian/Net/Packets/Play/Clientbound/ContainerSetContentPacket.cs(1 hunks)Obsidian/Net/Packets/Play/Serverbound/ContainerClickPacket.cs(4 hunks)Obsidian/Net/Packets/Play/Serverbound/PlayerCommandPacket.cs(4 hunks)Obsidian/Net/Packets/Play/Serverbound/PlayerInputPacket.cs(1 hunks)Obsidian/Obsidian.csproj(1 hunks)Obsidian/Plugins/PluginRegistry.cs(1 hunks)Obsidian/Registries/CommandsRegistry.cs(2 hunks)Obsidian/Server.cs(1 hunks)Obsidian/Utilities/Extensions.Nbt.Chat.cs(4 hunks)Obsidian/Utilities/Extensions.Nbt.cs(0 hunks)Obsidian/Utilities/Extensions.cs(1 hunks)
💤 Files with no reviewable changes (9)
- Obsidian/Net/ClientHandlers/ConfigurationClientHandler.cs
- Obsidian.SourceGenerators/Registry/EntityGenerator.cs
- Obsidian/Entities/Player.Helpers.cs
- Obsidian/Commands/Parsers/MinecraftTimeParser.cs
- Obsidian/Utilities/Extensions.Nbt.cs
- Obsidian/Commands/Parsers/CommandParser.cs
- Obsidian/Commands/Parsers/StringCommandParser.cs
- Obsidian/Commands/Parsers/NumberCommandParsers.cs
- Obsidian/Commands/Parsers/EntityCommandParser.cs
🧰 Additional context used
🧬 Code Graph Analysis (14)
Obsidian.Tests/Commands.cs (1)
Obsidian/Commands/Framework/CommandParser.cs (3)
CommandParser(5-88)IsCommandQualified(9-19)SplitQualifiedString(21-87)
Obsidian.API/Commands/ArgumentParsers/NumericArgumentParsers.Helpers.cs (1)
Obsidian.API/Commands/ArgumentParsers/NumericArgumentParsers.cs (2)
NumericArgumentParser(5-48)NumericArgumentParser(17-26)
Obsidian.API/_Interfaces/INetStreamReader.cs (1)
Obsidian/Net/NetworkBuffer.Reading.cs (1)
IHashedItemStack(121-149)
Obsidian/Net/NetworkBuffer.cs (3)
Obsidian.Nbt/RawNbtWriter.cs (1)
Span(275-275)Obsidian.API/_Interfaces/INetStream.cs (3)
Span(7-7)Span(9-9)Span(11-11)Obsidian/WorldData/RegionFile.cs (1)
offset(344-352)
Obsidian/Commands/Modules/MainCommandModule.cs (2)
Obsidian.API/_Types/HexColor.cs (4)
HexColor(11-14)HexColor(16-19)HexColor(21-23)HexColor(25-28)Obsidian/Utilities/Extensions.Nbt.Chat.cs (1)
ChatMessage(86-132)
Obsidian.API/_Interfaces/IPlayer.cs (2)
Obsidian/Entities/Player.cs (16)
ValueTask(175-207)ValueTask(209-219)ValueTask(221-245)ValueTask(247-262)ValueTask(264-290)ValueTask(292-293)ValueTask(295-296)ValueTask(298-299)ValueTask(301-327)ValueTask(329-329)ValueTask(330-330)ValueTask(381-396)ValueTask(428-435)ValueTask(437-444)ValueTask(446-462)ValueTask(464-474)Obsidian.API/_Types/ChatMessage.cs (16)
ChatMessage(60-60)ChatMessage(68-68)ChatMessage(76-79)ChatMessage(81-91)ChatMessage(93-103)ChatMessage(112-115)ChatMessage(125-129)ChatMessage(139-143)ChatMessage(171-177)ChatMessage(179-185)ChatMessage(188-194)ChatMessage(196-202)ChatMessage(204-212)ChatMessage(214-225)ChatMessage(227-239)ChatMessage(241-252)
Obsidian.API/Inventory/DataComponents/ComponentBuilder.cs (3)
Obsidian.API/_Interfaces/INetStreamWriter.cs (3)
WriteLengthPrefixedArray(54-54)WriteLengthPrefixedArray(56-56)WriteItemStack(35-35)Obsidian.API/_Interfaces/INetStreamReader.cs (2)
List(35-35)ItemStack(66-66)Obsidian/Net/NetworkBuffer.Reading.cs (1)
List(398-407)
Obsidian/Events/MainEventHandler.cs (2)
Obsidian/Events/MainEventHandler.Inventory.cs (1)
MainEventHandler(7-242)Obsidian.API/_Types/MinecraftEventHandler.cs (1)
MinecraftEventHandler(2-2)
Obsidian.API/Inventory/ItemStack.cs (4)
Obsidian.API/_Interfaces/IPlayer.cs (2)
ItemStack(124-124)ItemStack(125-125)Obsidian/Entities/Player.cs (2)
ItemStack(172-172)ItemStack(173-173)Obsidian.API/_Interfaces/INetStreamReader.cs (1)
ItemStack(66-66)Obsidian/Utilities/Extensions.Nbt.cs (1)
ItemStack(23-31)
Obsidian/Entities/Player.cs (3)
Obsidian/Entities/Player.Helpers.cs (1)
Player(12-416)Obsidian/Client.cs (8)
Player(411-414)ValueTask(163-185)ValueTask(228-243)ValueTask(245-261)ValueTask(301-313)ValueTask(386-398)Client(23-415)Client(139-161)Obsidian.API/_Interfaces/IPlayer.cs (10)
ValueTask(60-60)ValueTask(61-61)ValueTask(62-62)ValueTask(63-63)ValueTask(64-64)ValueTask(65-65)ValueTask(66-66)ValueTask(67-67)ValueTask(68-68)ValueTask(86-86)
Obsidian/Net/Packets/Play/Serverbound/PlayerInputPacket.cs (2)
Obsidian/Services/PacketBroadcaster.cs (1)
PacketBroadcaster(9-115)Obsidian/Entities/Entity.cs (1)
Entity(7-436)
Obsidian.API/_Interfaces/IHashedItemStack.cs (6)
Obsidian.API/_Interfaces/INetStreamReader.cs (3)
IHashedItemStack(67-67)List(35-35)ItemStack(66-66)Obsidian/Net/NetworkBuffer.Reading.cs (2)
IHashedItemStack(121-149)List(398-407)Obsidian/HashedItemStack.cs (1)
Compare(18-43)Obsidian.API/_Interfaces/IPlayer.cs (2)
ItemStack(124-124)ItemStack(125-125)Obsidian/Entities/Player.cs (2)
ItemStack(172-172)ItemStack(173-173)Obsidian.API/Inventory/ItemStack.cs (1)
ItemStack(25-31)
Obsidian/Entities/Entity.cs (2)
Obsidian/Entities/Player.cs (16)
ValueTask(175-207)ValueTask(209-219)ValueTask(221-245)ValueTask(247-262)ValueTask(264-290)ValueTask(292-293)ValueTask(295-296)ValueTask(298-299)ValueTask(301-327)ValueTask(329-329)ValueTask(330-330)ValueTask(381-396)ValueTask(428-435)ValueTask(437-444)ValueTask(446-462)ValueTask(464-474)Obsidian/WorldData/World.cs (2)
World(15-889)World(88-95)
Obsidian.API/Events/ContainerClickEventArgs.cs (2)
Obsidian.API/Containers/BaseContainer.cs (5)
ItemStack(64-64)ItemStack(140-140)BaseContainer(6-130)BaseContainer(22-22)BaseContainer(24-29)Obsidian.API/Containers/Container.cs (3)
Container(5-88)Container(22-22)Container(24-32)
🔇 Additional comments (112)
Obsidian.Nbt/Obsidian.Nbt.csproj (1)
24-26: LGTM! Good addition for performance optimization.The CommunityToolkit.HighPerformance dependency will enable efficient buffer management for NBT operations as mentioned in the summary.
Obsidian.API/_Attributes/ArgumentParserAttribute.cs (1)
1-7: Well-designed attribute implementation.The
ArgumentParserAttributeis correctly implemented with appropriateAttributeUsageconfiguration and follows C# attribute conventions.Obsidian.API/_Enums/ClickType.cs (1)
1-12: Comprehensive and well-structured enum.The
ClickTypeenum appropriately covers the various inventory click operations and follows good enum design practices with explicit int typing.Obsidian.API/_Enums/ProtocolVersion.cs (1)
76-79: Correct protocol version addition.The new
v1_21_6protocol version follows the established naming and numbering conventions properly.Obsidian.Tests/Commands.cs (1)
28-28: LGTM! Proper namespace qualification.The fully qualified
CommandParserusage correctly addresses potential namespace conflicts from the command parser refactoring. The test functionality remains unchanged while being more explicit about which parser class is being used.Also applies to: 31-31
Obsidian.API/Commands/ArgumentParsers/NumericArgumentParsers.Helpers.cs (1)
1-2: LGTM! Consistent with command parser refactoring.The namespace change to
Obsidian.API.Commands.ArgumentParsersand class rename toNumericArgumentParser<TNumber>aligns with the broader architectural refactoring. The helper methods correctly handle type-specific serialization of numeric bounds using appropriate conversion methods.Obsidian/Net/NetworkBuffer.cs (1)
59-59: LGTM! Unnecessary cast removal.Removing the explicit
intcasts foroffsetandsizeis correct since these fields are already declared asinttype. This improves readability without affecting functionality.Also applies to: 64-64
Obsidian/Utilities/Extensions.cs (1)
56-57: LGTM! Enhanced entity type specificity.Replacing
EntityType.Potionwith the more specificEntityType.SplashPotionandEntityType.LingeringPotionimproves classification granularity. Both potion types are correctly classified as non-living entities.Obsidian.API/_Interfaces/INetStreamReader.cs (1)
67-67: LGTM! Proper interface extension for hashed item stacks.The
ReadHashedItemStack()method declaration correctly extends the interface to support the new hashed item stack functionality. The nullable return type and naming convention are consistent with other optional reading methods in the interface.Obsidian.SourceGenerators/Registry/Models/Assets.cs (1)
17-18: LGTM! Clean formatting improvement.The parameter list split improves readability without affecting functionality.
Obsidian/Commands/CommandNodeType.cs (1)
15-15: LGTM! Proper flag enum addition.The new
IsRestrictedflag correctly follows the powers of 2 pattern and supports the enhanced command node state representation.Obsidian.SourceGenerators/Obsidian.SourceGenerators.csproj (1)
15-20: All package versions are current and secureThe updates in Obsidian.SourceGenerators/Obsidian.SourceGenerators.csproj (lines 15–20) use the latest stable releases and have no known security vulnerabilities:
- Microsoft.CodeAnalysis.CSharp 4.14.0 – latest
- Microsoft.CodeAnalysis.Analyzers 4.14.0 – latest
- Microsoft.Bcl.AsyncInterfaces 9.0.6 – latest
- System.Text.Encodings.Web 9.0.6 – latest
- System.Text.Json 9.0.6 – latest
- System.IO.Pipelines 9.0.6 – latest
No action required.
Obsidian/Obsidian.csproj (1)
53-56: All dependencies are up-to-date and secure.
- Microsoft.Extensions.DependencyInjection 9.0.6
- Microsoft.Extensions.Hosting.Abstractions 9.0.6
- Microsoft.Extensions.Logging 9.0.6
- Microsoft.Extensions.Logging.Abstractions 9.0.6
- System.IO.Hashing 9.0.5
These are the latest stable releases and no known security vulnerabilities have been reported.
Obsidian/Net/NetworkBuffer.Writing.cs (1)
414-414: Minor formatting adjustment.Added blank line for code readability.
Obsidian/Server.cs (1)
47-47: Protocol version update looks good.The update from
v1_21_5tov1_21_6aligns with the protocol enhancements mentioned in the PR summary, including new packets and features.Obsidian.API/Obsidian.API.csproj (2)
44-44: Good addition of command parser asset.The
command_parsers.jsonasset file supports the new command argument parsing system mentioned in the PR summary.
77-81: Dependency updates look appropriate.The addition of
CommunityToolkit.HighPerformanceand the minor version updates to Microsoft.Extensions packages support the performance optimizations and new features in this refactor.Obsidian.API/_Interfaces/IPlayer.cs (2)
12-12: Well-designed addition for player input tracking.The
PlayerInput Inputproperty provides a clean way to track player input state using the new flag-based enum. This enhances the interface capabilities for handling player interactions.
60-60: Good addition for enhanced disconnect messaging.The
DisconnectAsync(ChatMessage reason)method complements the existingKickAsyncmethods and provides richer disconnect messaging capabilities using the ChatMessage type.Obsidian.API/_Enums/PlayerInput.cs (1)
1-12: Excellent enum design for player input flags.The
PlayerInputenum is well-designed with:
- Correct
[Flags]attribute for bitwise operations- Efficient
byteunderlying type for network transmission- Proper power-of-2 values for bit manipulation
- Comprehensive coverage of player input actions
- Clear, consistent naming
This provides an efficient way to represent multiple simultaneous player inputs in a single byte.
Obsidian/Net/Packets/Play/Clientbound/ContainerSetContentPacket.cs (1)
6-9: LGTM! Type consistency improvement.The change from
byte windowIdtoint containerIdaligns the container identifier type with the integer-based container IDs used elsewhere in the codebase, such as inContainerClickEventArgs. The serialization logic usingWriteVarIntalready supports integer values, so this change is backward compatible.Obsidian.API/Inventory/ItemStack.cs (1)
6-6: LGTM! Appropriate use of record type.Converting
ItemStackto arecord classis an excellent choice for this value type. Records provide automatic value-based equality implementations, which aligns perfectly with the hash-based validation and synchronization improvements mentioned in the PR objectives.Obsidian/Assets/command_parsers.json (1)
1-59: command_parsers.json – validation passed
- JSON syntax is valid.
- All 57 parser mappings have unique IDs (57 total, 57 unique).
- Naming conventions are consistent.
Obsidian/Plugins/PluginRegistry.cs (1)
81-83: LGTM! Improved parser registration pattern.The changes align well with the new parser architecture:
- Using
BaseArgumentParser<T>provides better type safety than generic constraints- The
TryAddArgumentParsermethod suggests a defensive approach to prevent duplicate parser registrations, which is appropriate for a plugin registryObsidian/Commands/Modules/MainCommandModule.cs (3)
71-71: LGTM! Improved separation of content and styling.Removing inline color codes from the text and using structured
ChatMessageproperties improves maintainability and reduces the risk of formatting errors.
81-82: LGTM! Consistent use of explicit color properties.The change from inline color codes to the explicit
Color = HexColor.Goldproperty provides better structure and type safety for chat message formatting.
88-93: LGTM! Well-structured chat message composition.The refactored approach using
ChatMessage.Simple()and explicit color properties creates a cleaner, more maintainable structure for building complex chat messages. The separation of the description text with its own color styling is particularly well done.Obsidian.API/Plugins/IPluginRegistry.cs (1)
6-6: LGTM: Interface signature improvement enhances type safety.The change from a generic constraint to an explicit
BaseArgumentParser<T>parameter makes the interface more type-safe and aligns with the broader parser refactoring. This explicit typing provides clearer expectations for implementers.Obsidian/Events/MainEventHandler.cs (2)
7-7: LGTM: Partial class conversion enables modular organization.Converting to a partial class allows the
MainEventHandlerto be split across multiple files, which improves code organization. This aligns with the newMainEventHandler.Inventory.csfile mentioned in the relevant code snippets.
289-294: LGTM: Index-based iteration improves slot handling precision.The change from
foreachto aforloop with explicit slot indexing provides better control over item positioning in containers. This change supports the new inventory click handling system that requires precise slot management.Obsidian.API/Commands/ArgumentParsers/LocationArgumentParser.cs (1)
3-4: LGTM: Parser refactoring aligns with new attribute-driven architecture.The addition of the
[ArgumentParser("minecraft:vec3")]attribute and partial class declaration follows the new standardized parser pattern. This enables source generation of parser metadata while maintaining the existing parsing logic. The Minecraft identifier correctly matches the vec3 command parser type.Obsidian.API/Commands/ArgumentParsers/PlayerArgumentParser.cs (1)
3-4: LGTM: Consistent parser refactoring maintains functionality.The
[ArgumentParser("minecraft:game_profile")]attribute and partial class conversion follows the same pattern as other parsers in this refactoring. The identifier correctly matches Minecraft's game profile parser type, and the existing parsing logic is preserved.Obsidian/Entities/Player.cs (4)
43-43: LGTM: PlayerInput property enables comprehensive input tracking.The addition of the
PlayerInputproperty provides a centralized way to track player input states using flag-based enumeration, which is efficient and extensible.
45-55: LGTM: Sneaking property override integrates well with input system.The override correctly uses bitwise operations to manage the sneak flag within the
PlayerInputproperty. The implementation properly sets and clears thePlayerInput.Sneakflag while maintaining the existingSneakingproperty interface.
627-627: LGTM: DisconnectAsync method provides clean delegation.The method correctly forwards the disconnect call to the client's
DisconnectAsyncmethod, providing a convenient API on the Player class while maintaining proper separation of concerns.
145-145: Verify if constructor change is intentional.The constructor line is marked as changed but appears identical to what would be expected. Please confirm if this represents a formatting change or if there are non-visible modifications.
Obsidian.Nbt/RawNbtWriter.Primitives.cs (3)
1-4: LGTM! Proper dependencies added for high-performance buffer operations.The new using directives align with the buffer management enhancements using
ArrayPool<byte>andCommunityToolkit.HighPerformance.
171-171: Good defensive programming practice.Adding the
Reservecall before copying data prevents buffer overflows and ensures adequate capacity.
178-191: Excellent buffer management implementation.The
Reservemethod provides:
- Proper input validation with
Debug.Assert- Efficient resizing using
ArrayPool<byte>.Shared.Resize- Smart growth strategy (doubling capacity)
- Public access for preemptive buffer management
The implementation follows .NET performance best practices for buffer operations.
Obsidian.API/Inventory/DataComponents/ComponentBuilder.cs (3)
143-144: Good refactoring to explicit lambda expressions.Converting from method group references to explicit lambdas makes the delegate signatures clearer and aligns with the hashed item stack serialization improvements mentioned in the PR.
147-148: Consistent pattern applied.The same lambda expression pattern ensures consistency across all item stack components.
209-209: Pattern completion for Container component.Applying the same explicit lambda pattern maintains consistency with the other item stack components.
Obsidian/Commands/CommandNode.cs (5)
12-12: Good architectural improvement.Changing from
CommandParser?toBaseArgumentParser?provides a more flexible and generic parser system, aligning with the new argument parser framework.
18-18: Enhanced suggestion support.The new
SuggestionTypeproperty enables command suggestion metadata, improving the command system's usability.
24-24: Cleaner serialization using helper method.Using
WriteLengthPrefixedArrayis more readable and maintainable than manual loop serialization.
28-28: Critical bug fix in redirect serialization.Writing the node's own
Indexinstead of iterating over children is the correct behavior for redirect serialization according to the Command Data protocol.
41-44: Proper conditional suggestion serialization.The conditional serialization of
SuggestionTypewhen theHasSuggestionsflag is set follows the protocol specification correctly.Obsidian.API/_Types/HexColor.cs (1)
33-48: HexColor Code Verification CompleteAll hex values in Obsidian.API/_Types/HexColor.cs have been checked against the standard Minecraft color definitions and match expected RGB equivalents. The switch to hex-string constructors and the updated naming align with conventions and are correct.
• File verified: Obsidian.API/_Types/HexColor.cs
• Result: All color codes match expected values.Refactoring approved—no further action needed.
Obsidian/Net/Packets/Play/Serverbound/PlayerCommandPacket.cs (3)
14-14: Good refactoring to improve clarity.Renaming from
EActiontoPlayerCommandprovides better semantic meaning and aligns with the architectural separation where sneaking commands are moved toPlayerInputPacket.Also applies to: 22-22
67-79: Appropriate separation of concerns.The removal of
StartSneakingandStopSneakingfrom the enum aligns with the introduction ofPlayerInputPacketfor handling input state, creating a cleaner separation between command actions and input flags.
32-57: Switch statement correctly updated.All cases properly use the new
PlayerCommandenum values, maintaining the same functionality while improving code clarity.Obsidian.Nbt/RawNbtWriter.cs (3)
26-26: LGTM! Constructors correctly use the new buffer size.The constructors properly use the updated
InitialBufferSizeconstant.Also applies to: 36-36
76-82: Conditional root tag logic is correct and consistentThe early-return for List root types in RawNbtWriter.WriteCompoundStart matches the implementation in NbtWriterStream.WriteCompoundStart, ensuring that list elements do not write a tag header. No changes are required.
• Confirmed in Obsidian.Nbt/RawNbtWriter.cs WriteCompoundStart (lines 76–82)
• Behavior mirrors Obsidian.Nbt/NbtWriterStream.cs WriteCompoundStart
8-8: Buffer resizing via doubling strategy ensures amortized performanceThe
Reserve(int capacity)implementation inObsidian.Nbt/RawNbtWriter.Primitives.csusesArrayPool<byte>.Shared.Resize(ref data, newCapacity)with a doubling-growth strategy (and falls back to the exact required size if that’s larger). While dropping the initial buffer from 4096→256 bytes does mean a few more resizes for very large NBT payloads, the amortized cost remains constant and impact in real‐world usage will be negligible. No changes needed.Obsidian/Net/Packets/Play/Serverbound/PlayerInputPacket.cs (1)
10-13: LGTM! Proper packet deserialization.The
Populatemethod correctly reads the byte value and casts it to thePlayerInputenum.Obsidian.API/Commands/ArgumentParsers/BooleanArgumentParser.cs (1)
1-19: LGTM! Well-implemented boolean argument parser.The implementation demonstrates good practices:
- Proper use of
[ArgumentParser]attribute with brigadier identifier- Primary constructor with default value support via chained constructor
- Correct use of
bool.TryParsefor argument parsing- Proper serialization pattern calling
base.Write()before writing valueThe sealed partial class design aligns with the broader argument parser refactoring mentioned in the AI summary.
Obsidian/Registries/CommandsRegistry.cs (2)
2-2: LGTM! Updated import for new argument parser namespace.The import correctly references the new
Obsidian.API.Commands.ArgumentParsersnamespace.
78-87: Audit negative integer acceptance for ‘brigadier:integer’ and ‘brigadier:long’The switch in Obsidian/Registries/CommandsRegistry.cs (lines 78–87) now uses UnsignedIntArgumentParser and UnsignedLongArgumentParser, which will reject negative values. Please verify that none of your registered commands rely on negative integer or long inputs (e.g., block coordinates, relative offsets, time adjustments). If some commands do need to accept negatives, revert those cases to a signed parser or add explicit branches.
• File: Obsidian/Registries/CommandsRegistry.cs (lines 78–87) – “brigadier:integer” and “brigadier:long” mappings
Obsidian/Net/NetworkBuffer.Reading.cs (2)
30-30: LGTM! Minor formatting improvements.The formatting adjustments in various methods improve code consistency.
Also applies to: 231-231, 260-260, 420-420
105-107: LGTM! Code reorganization without functional changes.The repositioning of component reading variables doesn't change the functionality but improves code organization.
Obsidian.API/Commands/ArgumentParsers/MinecraftTimeArgumentParser.cs (4)
3-4: LGTM! Attribute-based parser identification implemented correctly.The
[ArgumentParser("minecraft:time")]attribute andsealed partial classdeclaration align well with the new argument parser framework using source generation.
6-6: LGTM! Min property with sensible default.The
Minproperty with default value 0 provides a configurable minimum tick value, which is reasonable for time validation.
40-45: LGTM! Proper minimum validation implemented.The minimum validation logic correctly rejects parsed time values below the threshold and resets the result appropriately before returning false.
49-54: LGTM! Serialization follows established pattern.The
Writemethod correctly calls the base implementation and serializes theMinproperty, maintaining consistency with other argument parsers.Obsidian.API/_Interfaces/IHashedItemStack.cs (1)
4-17: Well-designed interface for hashed item stack abstraction.The interface provides a clean abstraction for hashed item stacks with appropriate properties and comparison functionality. The property names and types are clear and consistent with the item stack domain.
Obsidian.API/Inventory/DataComponents/DataComponentsStorage.cs (5)
4-4: Good design choice using record class.Converting to
abstract record classis appropriate for value-type semantics and provides better immutability patterns for component storage.
7-7: LGTM! Clean separation of hashed and regular component storage.The separate
hashedStoragedictionary appropriately maintains the hashed components distinct from the regular component storage.
13-13: LGTM! Proper indexer implementation.The indexer provides clean access to components by type using the internal storage dictionary.
17-28: Excellent documentation and clear method purpose.The documentation clearly explains that hashes are client-provided and used only for server-side comparison, which is crucial for understanding the security and validation model.
30-31: LGTM! Efficient hash comparison method.The
CompareComponentHashmethod provides efficient hash-based comparison usingTryGetValuefor optimal performance.Obsidian/Net/ClientHandlers/PlayClientHandler.cs (3)
80-82: LGTM! New PlayerInputPacket handling added correctly.The new case for PlayerInputPacket (ID 42) follows the established pattern and integrates properly with the existing packet handling framework.
83-88: LGTM! Packet ID shifts handled correctly.The RecipeBookSeenRecipePacket and RenameItemPacket cases have been properly updated to their new IDs (46 and 47).
12-18: Packet ID consistency confirmedAll packet mappings in PlayClientHandler.cs align with the serverbound Play definitions in Obsidian/Assets/packets.json—no discrepancies found.
Obsidian/Entities/Entity.cs (4)
47-58: Good design choice making entity properties virtual.Making these boolean properties virtual enables proper polymorphism and allows derived classes like
Playerto override behavior (e.g.,Sneakingproperty). This supports the new player input system mentioned in the summary.
50-50: LGTM! MovementFlags property virtualization supports inheritance.Making
MovementFlagsvirtual aligns with the other state properties and enables derived classes to customize movement behavior.
227-227: LGTM! RemoveAsync method virtualization enables custom cleanup.Making
RemoveAsyncvirtual allows derived entities to implement custom removal logic while maintaining the default world entity destruction behavior.
229-229: LGTM! GenerateBitmask virtualization supports entity-specific flags.Making
GenerateBitmaskvirtual enables derived entities to customize their metadata bitmask generation, which is essential for entity-specific behaviors.Obsidian.API/Commands/ArgumentParsers/EntityArgumentParser.cs (3)
3-8: LGTM! Well-structured parser with proper defaults.The class structure, attribute decoration, and constructor design follow the new argument parser pattern correctly. The default constructor with
SingleEntityOrPlayerprovides sensible defaults.
17-23: LGTM! Correct network serialization.The
Writemethod properly calls the base implementation and correctly serializes theEntityFilterenum as a signed byte for network protocol compliance.
25-29: LGTM! Appropriate enum design.The
EntityFilterenum withsbytebacking type and meaningful values aligns well with Minecraft protocol requirements.Obsidian.Tests/Nbt.cs (2)
13-105: LGTM! Comprehensive NBT reading test activated.The
ReadBigTestmethod provides thorough validation of NBT deserialization across various tag types, nested compounds, lists, and arrays. The test assertions are well-structured and validate both data integrity and structure correctness.
108-181: LGTM! Well-designed round-trip test.The
WriteBigTestmethod creates a complex NBT structure and validates it by reading it back, ensuring write/read symmetry. The use ofMemoryStreamand callingReadBigTestfor validation is an excellent testing pattern.Obsidian.API/_Types/MinecraftTime.cs (3)
10-15: LGTM! Proper day-to-time conversions.The mathematical relationships are correct: 1 day = 24000 ticks = 1200 seconds. This ensures all time properties are synchronized when creating from a day value.
17-22: LGTM! Correct second-to-time conversions.The conversions properly calculate ticks (second * 20) and days (second / 1200), maintaining consistency across all time representations.
24-29: LGTM! Accurate tick-to-time conversions.The tick-based conversions correctly derive seconds (tick / 20) and days (tick / 24000), ensuring complete time synchronization.
Obsidian/Utilities/Extensions.Nbt.Chat.cs (3)
33-45: LGTM! Improved streaming list serialization.The change from creating intermediate list objects to streaming serialization improves performance and memory usage. The loop structure correctly handles compound writing for each extra item.
167-167: LGTM! Correct snake_case naming convention.The change from "hoverEvent" to "hover_event" aligns with Minecraft protocol standards for NBT tag naming.
182-182: LGTM! Consistent snake_case naming.The change from "clickEvent" to "click_event" maintains naming consistency with the protocol.
Obsidian/HashedItemStack.cs (2)
7-16: LGTM! Clean class structure and constructor.The class properly implements
IHashedItemStackwith clear property initialization. The primary constructor with default count parameter is well-designed.
24-40: LGTM! Proper resource management and hash comparison.The ArrayPool usage is correct with proper return, and the CRC32 hashing provides efficient component comparison. The early return on hash mismatch optimizes performance.
Obsidian/Commands/Framework/CommandHandler.cs (1)
30-36: Add null checks and validate generic arguments count.The parser initialization logic assumes
BaseTypeis not null and has at least one generic argument. This could throw exceptions if these assumptions don't hold.Apply this diff to add defensive checks:
var parsers = typeof(StringArgumentParser).Assembly.GetTypes() .Where(type => typeof(BaseArgumentParser).IsAssignableFrom(type) && !type.IsAbstract) - .Where(type => type.BaseType?.IsGenericType is true && type.BaseType.GetGenericArguments().Length != 0) + .Where(type => type.BaseType?.IsGenericType is true && type.BaseType.GetGenericArguments().Length > 0) .Select(x => (Activator.CreateInstance(x) as BaseArgumentParser)!); _argumentParsers = parsers.OrderBy(x => x.Id) - .ToDictionary(x => x.GetType().BaseType!.GetGenericArguments().First(), x => x); + .ToDictionary(x => x.GetType().BaseType!.GetGenericArguments()[0], x => x);Additionally, consider logging a warning if any parser types are skipped due to missing generic arguments.
Likely an incorrect or invalid review comment.
Obsidian/Events/MainEventHandler.Inventory.cs (3)
50-51: LGTM! Early return optimization.Good defensive check to avoid unnecessary processing when the carried item is null or already at max stack size.
162-162: Verify if thrown items should glow.All thrown items are set to glow. Is this intentional for gameplay mechanics or debugging?
If this is not intended, remove the glowing effect:
EntityId = Server.GetNextEntityId(), Item = removedItem, - Glowing = true, World = player.World, Position = loc
222-222: Hotbar slot offset is correctVerified that
PlayerHotbarStartin Obsidian.API/Containers/Container.cs is defined as 36, sobutton + 36correctly maps to the player’s hotbar slots.• Container.cs:
private const int PlayerHotbarStart = 36; private const int PlayerHotbarEnd = PlayerHotbarStart + 8;• MainEventHandler.Inventory.cs (line 222):
var localSlot = button + PlayerHotbarStart; // 36Consider replacing the magic number
36with thePlayerHotbarStartconstant for clarity.Obsidian.API/Commands/ArgumentParsers/StringArgumentParser.cs (1)
3-4: LGTM! Consistent attribute usage.The
[ArgumentParser]attributes correctly specify the brigadier/minecraft identifiers for both parser classes, aligning with the new source generation pattern.Also applies to: 24-25
Obsidian.API/Events/ContainerClickEventArgs.cs (2)
13-13: LGTM!The logic for determining player inventory based on
ContainerId == 0aligns with the documented behavior.
15-21: LGTM!Comprehensive documentation and proper implementation of the required property pattern.
Obsidian/Assets/tags.json (6)
1150-1161: Verify block list completeness for happy ghast avoidanceThe avoidance list looks sane, but you might also want to include self–igniting hazards the entity meets frequently in the Nether such as
lava,campfireorfire_pitblocks (if they exist in your dataset) to avoid edge-case damage loops.
Please double-check the intended design spec.
2355-2362: Naming is accurate, but double-check client sound hooksJust a reminder: make sure the client/engine side actually hooks the happy-ghast “dried_ghast” ambient sounds to
soul_sand/soul_soil, otherwise this new tag will be a no-op.
3380-3385: First‐class harness capability registeredTag correctly links
minecraft:happy_ghastto the new harness mechanic.
Nothing to change.
3428-3438: Happy ghast added to underwater-dismount groupGood catch – avoids rare soft-lock situations when riding entities underwater.
No issues spotted.
3454-3461: Fall-damage immunity tag updatedAppending
happy_ghastmakes sense and is consistent with vanillaghast.
No action required.
4716-4718: Horse food: adding plain carrot ✅
minecraft:carrotwas missing and is required for parity with Java breeding rules.
Change looks correct.Obsidian/Net/Packets/Play/Serverbound/ContainerClickPacket.cs (1)
52-67: LGTM!The deserialization logic is well-implemented with proper capacity initialization for the dictionary.
Obsidian/Assets/recipes.json (1)
9760-9768: Confirm that “~” is a legal key characterThe shaped recipe (lead?) swaps
S/#for~. While the spec only forbids space, JSON patchers occasionally mis-parse tildes in pattern strings. Double-check in-game loading or switch to a safer ASCII letter.Obsidian/Assets/packets.json (1)
170-177: Verify CustomClickAction packet in multiple statesThe
CustomClickActionpacket appears in both Configuration state (packet ID 8) and Play state (packet ID 65). While this is valid in the Minecraft protocol, please confirm this duplication is intentional.Also applies to: 1562-1569
Obsidian/Assets/item_components.json (6)
1626-1629: Verify engine support forcan_be_sheared/shearing_soundon carpetsCarpet items have never exposed these two properties in vanilla Bedrock. Double-check that the runtime actually consumes them; otherwise they’ll be silently ignored and create dead config.
7140-7144: Confirmcan_be_shearedis valid on horse-armour equippablesHorse armour has no precedent for being sheared. Ensure the property is recognised; otherwise strip it to avoid parser warnings.
10497-10511: Spawn-egg translation key differs from vanilla patternVanilla uses
"item.minecraft.xyz_spawn_egg.name"in language files.
Ensure localisation entry matches or the name will show as the raw key.
14814-14828: Validatejukebox_playablesound eventEnsure the sound event
"minecraft:tears"exists in the new sounds.json; otherwise the disc will play silence and log an error.
20017-20021: Saddle: verifyshearing_soundrouteSaddles are equippable on several entities; confirm that adding a shearing path does not collide with existing “unequip” logic or break dispenser behaviour.
25208-25210: Wolf armour: confirmcan_be_shearedvalidityThe wolf armour item historically uses
item.wolf_armor.unequip; ensure the engine triggers this when armour is removed, not via actual shearing.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
Obsidian.API/Commands/ArgumentParsers/NumericArgumentParsers.cs (4)
37-47: Critical: Incomplete type handling in WriteNumbers method remains unresolved.The
WriteNumbersmethod still only handles 4 of the 10+ numeric types supported by the parser implementations. This will cause serialization to fail silently forbyte,sbyte,short,ushort,uint,ulong, anddecimaltypes.Add support for all missing numeric types:
private void WriteNumbers(INetStreamWriter writer) { if (NumberType == typeof(int)) this.WriteAsInt(writer); + else if (NumberType == typeof(byte)) + this.WriteAsByte(writer); + else if (NumberType == typeof(sbyte)) + this.WriteAsSByte(writer); + else if (NumberType == typeof(short)) + this.WriteAsShort(writer); + else if (NumberType == typeof(ushort)) + this.WriteAsUShort(writer); + else if (NumberType == typeof(uint)) + this.WriteAsUInt(writer); else if (NumberType == typeof(double)) this.WriteAsDouble(writer); else if (NumberType == typeof(float)) this.WriteAsSingle(writer); else if (NumberType == typeof(long)) this.WriteAsLong(writer); + else if (NumberType == typeof(ulong)) + this.WriteAsULong(writer); + else if (NumberType == typeof(decimal)) + this.WriteAsDecimal(writer); }
50-108: Critical: Multiple parsers share the same brigadier identifier causing registration conflicts.Six different parser classes use
[ArgumentParser("brigadier:integer")], which will result in only one being registered while the others are silently ignored at runtime.Assign unique brigadier identifiers to each parser type:
-[ArgumentParser("brigadier:integer")] +[ArgumentParser("brigadier:int")] public sealed partial class SignedIntArgumentParser : NumericArgumentParser<int> -[ArgumentParser("brigadier:integer")] +[ArgumentParser("brigadier:byte")] public sealed partial class UnsignedByteArgumentParser : NumericArgumentParser<byte> -[ArgumentParser("brigadier:integer")] +[ArgumentParser("brigadier:sbyte")] public sealed partial class SignedByteArgumentParser : NumericArgumentParser<sbyte> -[ArgumentParser("brigadier:integer")] +[ArgumentParser("brigadier:short")] public sealed partial class SignedShortArgumentParser : NumericArgumentParser<short> -[ArgumentParser("brigadier:integer")] +[ArgumentParser("brigadier:ushort")] public sealed partial class UnsignedShortArgumentParser : NumericArgumentParser<ushort> -[ArgumentParser("brigadier:integer")] +[ArgumentParser("brigadier:uint")] public sealed partial class UnsignedIntArgumentParser : NumericArgumentParser<uint>
110-128: Critical: Duplicate brigadier identifier for long parsers causing registration conflict.Both signed and unsigned long parsers use the same
"brigadier:long"identifier, which will cause a registration collision.Use distinct identifiers for signed vs unsigned long:
[ArgumentParser("brigadier:long")] public sealed partial class SignedLongArgumentParser : NumericArgumentParser<long> -[ArgumentParser("brigadier:long")] +[ArgumentParser("brigadier:ulong")] public sealed partial class UnsignedLongArgumentParser : NumericArgumentParser<ulong>
150-151: Questionable: DecimalArgumentParser using "brigadier:double" identifier.While better than the previous
"brigadier:integer", using"brigadier:double"for a decimal parser may be semantically incorrect sincedecimalanddoublehave different precision and behavior characteristics.Consider using a more appropriate identifier:
-[ArgumentParser("brigadier:double")] +[ArgumentParser("brigadier:decimal")] public sealed partial class DecimalArgumentParser : NumericArgumentParser<decimal>Obsidian/Net/Packets/Play/Serverbound/ContainerClickPacket.cs (2)
81-93: Still using incorrect item for carried slot synchronization.The synchronization packet is still sending
clickedIteminstead ofthis.CarriedItemfor slot -1. This is the same issue identified in previous reviews and can cause client-server desynchronization.await player.Client.QueuePacketAsync(new ContainerSetSlotPacket { ContainerId = this.ContainerId, Slot = -1, - SlotData = clickedItem, + SlotData = this.CarriedItem, StateId = 0,//State id is ignored if slot is set to -1 });
114-119: Still using incorrect carried item in container sync packet.The
CarriedItemproperty in the sync packet should bethis.CarriedItem(the actual item the player is carrying), notclickedItem. This is the same issue from previous reviews.await player.Client.QueuePacketAsync(new ContainerSetContentPacket(this.ContainerId, container.ToList()) { StateId = this.StateId, - CarriedItem = clickedItem + CarriedItem = this.CarriedItem });
🧹 Nitpick comments (2)
Obsidian/Net/Packets/Play/Serverbound/ContainerClickPacket.cs (1)
95-108: Consider extracting inventory validation logic.The inventory validation logic is well-structured but could benefit from being extracted into a separate method for better maintainability and testability.
+ private Dictionary<short, IHashedItemStack> ValidateInventoryChanges(IContainer container) + { + var invalidItems = new Dictionary<short, IHashedItemStack>(); + foreach (var (changedSlot, hashedItem) in this.ChangedSlots) + { + var checkedItem = container[changedSlot]; + + if (hashedItem == null) + { + container.RemoveItem(changedSlot); + continue; + } + + if (!hashedItem.Compare(checkedItem)) + invalidItems.Add(changedSlot, hashedItem); + } + return invalidItems; + }Then use it in the main method:
- var invalidItems = new Dictionary<short, IHashedItemStack>(); - foreach (var (changedSlot, hashedItem) in this.ChangedSlots) - { - var checkedItem = container[changedSlot]; - - if(hashedItem == null) - { - container.RemoveItem(changedSlot); - continue; - } - - if (!hashedItem.Compare(checkedItem)) - invalidItems.Add(changedSlot, hashedItem); - } + var invalidItems = ValidateInventoryChanges(container);Obsidian/Events/MainEventHandler.Inventory.cs (1)
162-162: Remove glowing effect from thrown items.Thrown items don't typically glow in Minecraft. This appears to be unintentional.
- Glowing = true,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Obsidian.API/Commands/ArgumentParsers/NumericArgumentParsers.cs(1 hunks)Obsidian.API/Events/ContainerClickEventArgs.cs(1 hunks)Obsidian.API/Events/ContainerEventArgs.cs(1 hunks)Obsidian/Events/MainEventHandler.Inventory.cs(1 hunks)Obsidian/Net/NetworkBuffer.Writing.cs(2 hunks)Obsidian/Net/Packets/Play/Serverbound/ContainerClickPacket.cs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Obsidian/Net/NetworkBuffer.Writing.cs
- Obsidian.API/Events/ContainerClickEventArgs.cs
🔇 Additional comments (7)
Obsidian.API/Commands/ArgumentParsers/NumericArgumentParsers.cs (2)
5-26: Good: Well-designed generic base class with proper constraints and flag management.The generic base class effectively consolidates common numeric parser functionality with appropriate type constraints and flag-based min/max value tracking.
160-166: Fixed: NumberFlags enum now has explicit flag values.The explicit values for
HasMinValue = 1andHasMaxValue = 2ensure proper bitwise operations for the flags enum.Obsidian/Net/Packets/Play/Serverbound/ContainerClickPacket.cs (4)
39-39: Good refactor to use ClickType enum.The change from
ModetoClickTypealigns well with the overall refactor objectives and improves code clarity.
42-42: Good fix for the nullable reference type issue.The initialization to an empty dictionary properly addresses the previous concern about using
default!.
121-129: Good refactor of event dispatch.The event dispatch has been properly updated to use the new
ContainerClickEventArgsstructure with the required properties.
58-67: ✅ Deserialization methods verifiedBoth
ReadVarInt<TEnum>()andReadHashedItemStack()are declared inINetStreamReaderand implemented inNetworkBuffer.Reading. No further changes needed.Obsidian/Events/MainEventHandler.Inventory.cs (1)
60-60: No action needed: ItemStack uses value-based equalityThe
ItemStacktype is declared as a C# record class (public sealed record class ItemStack …, IEquatable<ItemStack>), which the compiler synthesizes with value-based==,!=, andEqualsimplementations. The existing checksif (item is null || item != carriedItem)correctly compare item equality by value, so no change is required.
1 participant
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores
Refactor
Removals