Conversation
github-actions
bot
added
api
WalkthroughThis update introduces a comprehensive refactor and expansion of the command argument parser infrastructure, moving from legacy parser classes to a new, attribute-driven, partial class system. It adds new parser types, updates serialization and registration logic, and integrates a source generator for automatic parser ID assignment based on JSON configuration. The update also brings new items, recipes, tags, and protocol/packet changes, including support for new entity types and player input handling. Changes
Sequence Diagram(s)sequenceDiagram
participant CommandRegistry
participant ArgumentParser (partial class)
participant RegistryGenerator
participant command_parsers.json
CommandRegistry->>ArgumentParser: Registers parser (with [ArgumentParser] attribute)
RegistryGenerator->>command_parsers.json: Reads parser identifier mappings
RegistryGenerator->>ArgumentParser: Generates partial class with Id/Identifier from JSON
ArgumentParser->>CommandRegistry: Provides Id/Identifier via generated code
CommandRegistry->>ArgumentParser: Looks up parser by argument type for command node
sequenceDiagram
participant Client
participant Server
participant Player
participant PlayerInputPacket
Client->>Server: Sends PlayerInputPacket (with input flags)
Server->>Player: Updates Player.Input property
Server->>Player: Checks Sneaking state
alt Sneaking state changed
Server->>All Players (except Player): Broadcast SetEntityDataPacket
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (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: 16
🔭 Outside diff range comments (1)
Obsidian.API/Commands/ArgumentParsers/PlayerArgumentParser.cs (1)
7-21: Fix return logic - should return false when player is not found.The method currently always returns
trueeven when the player is not found (line 20), which could cause downstream issues when commands expect a valid player object.result = player; - return true; + return player is not null;This ensures that the parsing fails appropriately when no matching player is found, allowing the command system to handle the error correctly.
♻️ Duplicate comments (16)
Obsidian/Assets/recipes.json (16)
5560-5606: Duplicate comment – same tag-refactor suggestion applies
5695-5723: Duplicate comment – same tag-refactor suggestion applies
5811-5839: Duplicate comment – same tag-refactor suggestion applies
5927-5955: Duplicate comment – same tag-refactor suggestion applies
6043-6071: Duplicate comment – same tag-refactor suggestion applies
6159-6187: Duplicate comment – same tag-refactor suggestion applies
6275-6303: Duplicate comment – same tag-refactor suggestion applies
6391-6419: Duplicate comment – same tag-refactor suggestion applies
6507-6535: Duplicate comment – same tag-refactor suggestion applies
6623-6651: Duplicate comment – same tag-refactor suggestion applies
6739-6767: Duplicate comment – same tag-refactor suggestion applies
6855-6883: Duplicate comment – same tag-refactor suggestion applies
6971-6999: Duplicate comment – same tag-refactor suggestion applies
7087-7115: Duplicate comment – same tag-refactor suggestion applies
7203-7231: Duplicate comment – same tag-refactor suggestion applies
10055-10072: Duplicate comment – shaped harness recipe
🧹 Nitpick comments (17)
Obsidian.API/Commands/ArgumentParsers/LocationArgumentParser.cs (1)
30-53: Consider edge case handling for malformed relative coordinates.The parsing logic correctly handles relative coordinates (
~), but consider what happens when a relative coordinate has additional text after~(e.g.,~5.5). Currently, this would fall through and not be parsed as relative.Consider enhancing the relative coordinate parsing:
- else if (text.StartsWith('~')) + else if (text.StartsWith('~')) { + var offsetText = text.Substring(1); + float offset = 0f; + if (!string.IsNullOrEmpty(offsetText) && !float.TryParse(offsetText, out offset)) + { + result = default; + return false; + } + if (player is null) { result = default; return false; } switch (i) { case 0: - location.X = player.Position.X; + location.X = player.Position.X + offset; break; case 1: - location.Y = player.Position.Y; + location.Y = player.Position.Y + offset; break; case 2: - location.Z = player.Position.Z; + location.Z = player.Position.Z + offset; break;Obsidian.API/Commands/ArgumentParsers/PlayerArgumentParser.cs (1)
6-6: Consider implementing selector support.The TODO comment indicates missing selector support (e.g.,
@p,@a,@e). This is important functionality for Minecraft commands.Would you like me to help implement selector parsing or create an issue to track this feature?
Obsidian/Net/Packets/Play/Serverbound/PlayerCommandPacket.cs (1)
1-1: Unused import detected.The
Microsoft.Extensions.Loggingnamespace is imported but not used in this file. Consider removing it unless it's used for logging in other parts of the partial class.Obsidian.SourceGenerators/Registry/RegistryGenerator.cs (2)
20-22: Consider making the JSON filename configurable.The hardcoded filename check for "command_parsers" could be made more flexible.
Consider using a constant or configuration:
+private const string ParserConfigFileName = "command_parsers"; + var jsonFiles = ctx.AdditionalTextsProvider - .Where(file => Path.GetFileNameWithoutExtension(file.Path) == "command_parsers") + .Where(file => Path.GetFileNameWithoutExtension(file.Path) == ParserConfigFileName) .Select(static (file, ct) => file.GetText(ct)!.ToString());
59-61: Make assembly name check configurable.The hardcoded assembly name check reduces reusability.
Consider making it configurable:
+private const string TargetAssemblyName = "Obsidian.API"; + var asm = compilation.AssemblyName; -if (asm != "Obsidian.API") +if (asm != TargetAssemblyName) return;Obsidian/Assets/packets.json (4)
114-121: AddedShowDialog(id 18) – remember to bump the upper-bound checksSame note as above; if any range checks (
id <= 17etc.) existed they now need +1.
Otherwise LGTM.
434-441: Out-of-order insertion hurts maintainability
ClearDialog(Play, id 132) is injected in the middle of the ≤20 range (just afterChunksBiomes, id 13).
JSON order does not affect runtime, but alphabetical / numerical ordering is the only way humans can grep this 2 k-line file.Suggested move:
- { … "packet_id": 132 … }, + // keep low-id section untouched … (move block) … + { … "packet_id": 132 … },
1226-1233: Same ordering issue forShowDialog(id 133)Placed after id 108 but before 110.
Consider moving to the 128-133 cluster (whereCustomReportDetailsandProjectilePowerlive) so ids stay monotonically increasing.
1386-1393:Waypoint(id 131) appears after 133; ordering now 128,129,130,132,133,131Low risk, but reinforces the previous two comments – please sort before merge.
Obsidian/Assets/recipes.json (1)
15488-15502: Craftable saddle may break existing loot-only balanceIntroducing a saddle recipe is excellent for survival QoL but affects progression/balance (previously dungeon loot only).
Consider gating it behind a smithing template or higher resource cost (e.g., iron blocks) to keep rarity.
Obsidian/Assets/tags.json (5)
1150-1161: Consider including soul-fire in the hazard setThe new
block/happy_ghast_avoidslist contains plainminecraft:firebut omitsminecraft:soul_fire, which is another common fire block the entity can encounter. If the intention is “avoid any fire”, addminecraft:soul_fire(or simply reference#minecraft:fire) to cover both variants and avoid behavioural gaps.
3072-3081: Empty dialog tags – deliberate placeholder?
dialog/pause_screen_additionsanddialog/quick_actionsare declared but currently empty.
If these are just forward-compat scaffolds, 👍. Otherwise populate them or drop them to keep the asset list lean.
3454-3455: Fall-immunity side effectsIncluding Happy Ghast in
fall_damage_immunewill also exempt it from stalagmite damage and other fall-based triggers that key off this tag. Confirm this is intended gameplay.
3463-3493:followable_friendly_mobslist is becoming largeAt 30+ entries this tag is edging into “kitchen-sink” territory and may impact AI look-ups each tick. Consider splitting into themed subsets (farm_animals, rideables, etc.) if perf profiles ever show hotspots.
4714-4718: Duplicates inhorse_food
minecraft:carrotis added, buthorse_foodalready containedcarrotin earlier versions for pigs. No functional harm, yet duplicates bloat the tag list. Consider de-duplicating to keep assets tidy.Obsidian/Assets/item_components.json (1)
1706-1729: Harness colour variants are massive copy-paste blocks – generate them instead.All 16 coloured harness definitions are byte-for-byte identical except for the colour token.
Consider emitting them from a small templating script (JSON5 +forloop or a C#SourceGeneratorlike you did for parsers). Gains:• Zero risk of accidental divergence between colours.
• One-line maintenance when you tweak a common value (equip_sound, rarity, etc.).
• Smaller diff noise in future PRs.Also applies to: 2131-2154, 2826-2850, 5766-5790, 10031-10055, 10294-10318, 12594-12618, 12857-12881, 13200-13224, 13574-13598, 16300-16324, 18692-18716, 19370-19394, 25715-25739
Obsidian.API/Commands/ArgumentParsers/NumericArgumentParsers.cs (1)
157-163: Consider explicit flag values for clarity.While the automatic enum value assignment works correctly, explicit bit flag values improve readability and prevent accidental reordering issues.
[Flags] public enum NumberFlags : byte { - None, - HasMinValue = 1, - HasMaxValue + None = 0, + HasMinValue = 1, + HasMaxValue = 2 }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (43)
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/Obsidian.API.csproj(1 hunks)Obsidian.API/_Attributes/ArgumentParserAttribute.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/IPlayer.cs(1 hunks)Obsidian.API/_Types/BaseArgumentParser.cs(1 hunks)Obsidian.Nbt/RawNbtWriter.cs(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/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/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.cs(2 hunks)Obsidian/Net/ClientHandlers/ConfigurationClientHandler.cs(0 hunks)Obsidian/Net/ClientHandlers/PlayClientHandler.cs(2 hunks)Obsidian/Net/Packets/Play/Serverbound/PlayerCommandPacket.cs(4 hunks)Obsidian/Net/Packets/Play/Serverbound/PlayerInputPacket.cs(1 hunks)Obsidian/Registries/CommandsRegistry.cs(2 hunks)Obsidian/Server.cs(1 hunks)Obsidian/Utilities/Extensions.Nbt.Chat.cs(1 hunks)Obsidian/Utilities/Extensions.cs(1 hunks)
💤 Files with no reviewable changes (7)
- Obsidian/Net/ClientHandlers/ConfigurationClientHandler.cs
- Obsidian.SourceGenerators/Registry/EntityGenerator.cs
- Obsidian/Commands/Parsers/MinecraftTimeParser.cs
- Obsidian/Commands/Parsers/CommandParser.cs
- Obsidian/Commands/Parsers/StringCommandParser.cs
- Obsidian/Commands/Parsers/EntityCommandParser.cs
- Obsidian/Commands/Parsers/NumberCommandParsers.cs
🔇 Additional comments (69)
Obsidian.Nbt/RawNbtWriter.cs (1)
8-8: Performance improvement: Buffer size increase looks good.Doubling the buffer size from 4KB to 8KB aligns well with the enhanced serialization requirements mentioned in the ArgumentParser refactor. This should reduce buffer reallocations during NBT writing operations.
Obsidian.API/Obsidian.API.csproj (1)
44-44: Good addition: JSON asset supports the new ArgumentParser source generator.Adding
command_parsers.jsonas an additional file enables the source generator to automatically assign parser IDs based on the JSON configuration. This addresses the PR objective of preventing command loading failures due to outdated IDs after protocol updates.Obsidian.Tests/Commands.cs (2)
28-28: Necessary update: Full qualification ensures test compatibility.The full qualification of
CommandParserwithObsidian.Commands.Frameworkis necessary due to the ArgumentParser refactor that restructured the command parsing infrastructure.
31-31: Consistent update: Maintaining full qualification for static method call.Consistently using the fully qualified namespace for the static
SplitQualifiedStringmethod call aligns with the changes made to the constructor call above.Obsidian.API/_Attributes/ArgumentParserAttribute.cs (1)
1-7: Well-designed attribute: Clean implementation for ArgumentParser refactor.The
ArgumentParserAttributeis well-designed with appropriate constraints:
AttributeTargets.Classlimits usage to classes onlyAllowMultiple = falseprevents duplicate attributes- Primary constructor syntax is clean and concise
- The string identifier enables the source generator to map parsers to their configuration
This attribute plays a crucial role in the new declarative approach to argument parser registration.
Obsidian/Utilities/Extensions.Nbt.Chat.cs (1)
1-1: Good cleanup: Removed unused using directive.Removing the unused
using Obsidian.API.Utilities;directive is good housekeeping that aligns with the broader refactoring efforts in this PR.Obsidian.API/_Interfaces/IPlayer.cs (1)
12-13: LGTM! Clean interface extension.The
PlayerInput Inputproperty addition follows proper C# interface conventions and integrates well with the existing property structure. The positioning after client-related properties and before chunk management is logical.Obsidian.SourceGenerators/Registry/Models/Assets.cs (1)
17-18: Good formatting improvement.The constructor parameter list formatting enhances readability without affecting functionality.
Obsidian/Commands/CommandNodeType.cs (1)
14-16: LGTM! Proper enum flag addition.The
IsRestricted = 0x20flag follows the correct binary flag pattern and integrates well with the existing command node type system. The value maintains the proper power-of-2 sequence for bitwise operations.Obsidian.API/_Enums/ProtocolVersion.cs (1)
76-80: LGTM! Proper protocol version addition.The
v1_21_6 = 771addition follows the established pattern with correct value sequence, proper Description attribute, and good formatting with the trailing comma for maintainability.Obsidian.API/Commands/ArgumentParsers/NumericArgumentParsers.Helpers.cs (2)
1-2: LGTM! Good architectural refactoring.The namespace move to
Obsidian.API.Commands.ArgumentParsersand rename toNumericArgumentParser<TNumber>improves the architectural separation and naming consistency. The partial class design supports the source generation approach mentioned in the PR objectives.
4-39: Well-structured serialization helpers.The four helper methods (
WriteAsInt,WriteAsDouble,WriteAsSingle,WriteAsLong) follow a consistent pattern and properly handle numeric bounds serialization using theIConvertibleinterface methods. The flag checking ensures only specified bounds are written, which is correct for the command parser protocol.Obsidian/Server.cs (1)
47-47: Protocol version update looks correct.The protocol version bump from
v1_21_5tov1_21_6aligns with the PR objectives and follows the expected pattern for protocol updates.Obsidian.API/_Enums/EntityType.cs (2)
61-61: New entity type addition looks good.The
HappyGhastentity type is properly positioned in alphabetical order afterGhast.
105-106: Verify breaking change impact of Potion replacement.The replacement of the single
Potionentity type withSplashPotionandLingeringPotionis a breaking change. Ensure all references toEntityType.Potionthroughout the codebase have been updated to use the appropriate specific potion type.#!/bin/bash # Description: Search for any remaining references to EntityType.Potion to ensure complete migration # Expected: No results or only references in comments/documentation echo "Searching for EntityType.Potion references..." rg "EntityType\.Potion\b" --type cs echo "Searching for just 'Potion' in EntityType contexts..." rg "EntityType\s*\.\s*Potion" --type csObsidian/Utilities/Extensions.cs (1)
56-57: Consistent update with EntityType enum changes.The replacement of
EntityType.PotionwithEntityType.SplashPotionandEntityType.LingeringPotionin thenonLivingarray correctly maintains consistency with the enum changes inObsidian.API/_Enums/EntityType.cs. Both potion types are appropriately classified as non-living entities.Obsidian/Entities/Player.cs (3)
43-43: PlayerInput property addition looks good.The new
Inputproperty integrates thePlayerInputenum into the player entity model appropriately.
45-55: Sneaking property override implementation is correct.The bit flag operations for the
Sneakingproperty are properly implemented:
- Uses
HasFlag()for reading the sneak state- Uses bitwise OR (
|=) to set the flag- Uses bitwise AND with complement (
&= ~) to clear the flagThis maintains backward compatibility while integrating with the new input system.
145-145: Minor formatting improvement.The trailing space removal in the constructor declaration is a good cleanup.
Obsidian.API/_Enums/PlayerInput.cs (1)
1-12: Well-designed flags enum for player input.The
PlayerInputenum is properly implemented with:
- Correct
[Flags]attribute for bitwise operations- Appropriate
byteunderlying type for 7 flags- Proper power-of-two values (0x01, 0x02, 0x04, 0x08, 0x10, 0x20, 0x40)
- Clear, descriptive flag names
This design allows for efficient bitwise combinations of multiple input states.
Obsidian/Assets/command_parsers.json (1)
1-59: Verify completeness and consistency of the parser registry.The JSON registry looks comprehensive, but please ensure:
- All parser IDs mentioned in the existing codebase are covered
- The numeric IDs align with the Minecraft protocol specification for version 1.21.6
- No duplicate IDs exist (though they appear unique in this file)
#!/bin/bash # Verify all ArgumentParser attributes in the codebase match entries in this JSON echo "=== Checking ArgumentParser attribute usage ===" rg -A 1 '\[ArgumentParser\(' --type cs | grep -o '"[^"]*"' | sort -u echo -e "\n=== Checking JSON parser identifiers ===" jq -r 'keys[]' Obsidian/Assets/command_parsers.json | sort echo -e "\n=== Checking for duplicate IDs in JSON ===" jq -r 'values[]' Obsidian/Assets/command_parsers.json | sort -n | uniq -dObsidian.API/Commands/ArgumentParsers/LocationArgumentParser.cs (1)
3-4: LGTM - Clean refactor to attribute-based parser registration.The transition from explicit constructor to
[ArgumentParser("minecraft:vec3")]attribute aligns well with the new architecture. The identifier correctly maps to ID 10 in the JSON registry.Obsidian.API/Commands/ArgumentParsers/PlayerArgumentParser.cs (1)
3-4: LGTM - Consistent refactor pattern.The attribute-based approach is properly implemented, and the identifier correctly maps to ID 7 in the JSON registry.
Obsidian.API/Commands/ArgumentParsers/MinecraftTimeArgumentParser.cs (2)
3-4: LGTM - Proper integration with new parser architecture.The attribute-based approach is correctly implemented, and the identifier maps to ID 43 in the JSON registry.
43-48: LGTM - Proper serialization implementation.The
Writemethod correctly calls the base implementation and serializes theMinproperty, following the established pattern for argument parser serialization.Obsidian/Commands/CommandNode.cs (5)
12-12: LGTM - Proper integration with new parser hierarchy.Changing the
Parserproperty type from the removedCommandParsertoBaseArgumentParsercorrectly integrates with the new argument parser architecture.
18-18: Good addition for command suggestion support.The
SuggestionTypeproperty adds valuable functionality for supporting command suggestions, which is important for user experience.
24-24: Improved serialization with length-prefixed array.Using
WriteLengthPrefixedArrayfor children indices is more robust than the previous approach and follows established serialization patterns.
41-44: LGTM - Proper suggestion type serialization.The conditional serialization of
SuggestionTypewhen theHasSuggestionsflag is present follows the correct protocol pattern.
26-29: ```shell
#!/bin/bashDisplay the full contents of CommandNode.cs for context
sed -n '1,200p' Obsidian/Commands/CommandNode.cs
</details> <details> <summary>Obsidian/Registries/CommandsRegistry.cs (2)</summary> `2-2`: **LGTM: Import statement updated for new namespace.** The import correctly reflects the new ArgumentParsers namespace structure. --- `80-86`: **Verify the switch from signed to unsigned integer parsers.** The migration changes `IntCommandParser` to `UnsignedIntArgumentParser` and `LongCommandParser` to `UnsignedLongArgumentParser`. This could be a breaking change if existing commands expect signed integer values. Additionally, ensure that the new `EmptyArgumentParser(id, mctype)` constructor properly handles the fallback case with the correct parameters. ```shell #!/bin/bash # Description: Search for existing command definitions that might use signed integer parameters # Expected: Find command methods that accept int/long parameters to verify compatibility echo "Searching for command method signatures with signed integer parameters..." ast-grep --pattern $'[Command($$$)] $$$ $METHOD($$$int $PARAM$$$) { $$$ }' echo -e "\nSearching for command method signatures with long parameters..." ast-grep --pattern $'[Command($$$)] $$$ $METHOD($$$long $PARAM$$$) { $$$ }'Obsidian/Net/Packets/Play/Serverbound/PlayerCommandPacket.cs (2)
14-14: LGTM: Enum refactoring is consistent.The property type change from
EActiontoPlayerCommandand corresponding switch case updates are implemented correctly. The enum renaming provides better clarity.Also applies to: 22-22, 32-56
67-79: Verify sneaking functionality is handled in PlayerInputPacket.The
StartSneakingandStopSneakingactions were removed from this enum. Based on the AI summary, this functionality has been moved to the newPlayerInputPacket. Ensure the sneaking state is properly synchronized between the two packet handlers.#!/bin/bash # Description: Verify that PlayerInputPacket properly handles sneaking functionality # Expected: Find PlayerInputPacket implementation and Player.Sneaking property usage echo "Searching for PlayerInputPacket implementation..." fd PlayerInputPacket.cs --exec cat {} echo -e "\nSearching for Player.Sneaking property usage..." rg -A 3 -B 3 "\.Sneaking" --type csObsidian/Net/Packets/Play/Serverbound/PlayerInputPacket.cs (3)
7-8: LGTM: Property declaration follows serialization pattern.The
Flagsproperty correctly uses the[Field(0), ActualType(typeof(byte))]attributes for proper packet serialization.
10-13: LGTM: Deserialization logic is correct.The
Populatemethod correctly reads a byte and casts it to thePlayerInputenum.
15-31: LGTM: State change detection and broadcasting implemented correctly.The
HandleAsyncmethod properly:
- Captures the previous sneaking state
- Updates the player's input flags
- Detects sneaking state changes
- Broadcasts entity data updates when necessary
The logic ensures efficient network updates by only broadcasting when the sneaking state actually changes.
Obsidian.API/Commands/ArgumentParsers/BooleanArgumentParser.cs (4)
3-4: LGTM: Class declaration follows new parser pattern.The class is properly decorated with
[ArgumentParser("brigadier:bool")]and follows the sealed partial class pattern with correct inheritance.
6-8: LGTM: Constructor pattern is well-designed.The primary constructor initializes the
Valueproperty, and the parameterless constructor provides a sensible default offalse.
10-11: LGTM: Parsing implementation is correct.Using
bool.TryParseis the appropriate approach for parsing boolean values from string input.
13-18: LGTM: Serialization logic is consistent.The
Writemethod correctly calls the base implementation and then writes the boolean value. This follows the established pattern for argument parser serialization.Obsidian.API/Commands/ArgumentParsers/EntityArgumentParser.cs (3)
3-4: LGTM: Class structure follows established pattern.The class declaration with
[ArgumentParser("minecraft:entity")]and the constructor/property pattern are consistent with the new argument parser framework.Also applies to: 6-8
12-17: LGTM: Serialization implementation is correct.The
Writemethod properly calls the base implementation and serializes theMaskas a signed byte, which matches theEntityFilterenum definition.
20-24: LGTM: EntityFilter enum is well-defined.The enum provides appropriate filter options for entity selection with correct byte values.
Obsidian/Net/ClientHandlers/PlayClientHandler.cs (1)
12-100: Protocol update changes look correct.The packet ID updates are consistent between the dictionary mappings and switch cases, and the new
PlayerInputPackethandling follows the established pattern.Obsidian/Entities/Entity.cs (2)
47-58: Virtual properties enable proper extensibility.Making these properties virtual is appropriate for supporting the new player input system and allows derived classes to customize movement and state behavior.
227-227: Modifier ordering follows C# conventions.The reordering to
public async virtualfollows the recommended modifier order in C# whereasyncprecedesvirtual.Obsidian.API/_Types/BaseArgumentParser.cs (1)
7-15: Well-designed abstract base class pattern.The abstract properties work perfectly with the source generator approach, and the virtual
Writemethod provides good extensibility for serialization.Obsidian/Assets/packets.json (3)
2-9: Packet ID 17 is free, but double-check for accidental duplication across namespaces
Clientbound / Configurationnow contains a new entryClearDialogwithpacket_id: 17.
A quick scan shows no other Clientbound-Configuration packet with id 17, so functionally this is fine.
Please verify the generators / handler enums have been regenerated so hard-coded constants don’t still expect the previous top-id (16).
170-177:CustomClickActionadded to Serverbound-Configuration: make sure it is mirrored on the Play sideThere is a matching Play-state packet further down (id 65).
Keep the implementations for both states in sync; they often differ only by handler wiring, so easy to let one stagnate.
1426-1480: ```shell
#!/bin/bashCheck for missing packet_id gaps per (namespace,state)
python3 - <<'EOF'
import json
from itertools import groupbydata = json.load(open('Obsidian/Assets/packets.json'))
Sort by namespace, state, then packet_id
data_sorted = sorted(data, key=lambda x: (x['namespace'], x['state'], x['packet_id']))
for (ns, st), items in groupby(data_sorted, key=lambda x: (x['namespace'], x['state'])):
ids = [pkt['packet_id'] for pkt in items]
if not ids:
continue
missing = [i for i in range(min(ids), max(ids) + 1) if i not in ids]
if missing:
print(f"{ns}|{st} missing packet_ids: {missing}")
EOF</details> <details> <summary>Obsidian/Assets/recipes.json (2)</summary> `5405-5409`: **Verify `"misc"` is an accepted recipe category** Vanilla 1.21 recognises `building`, `redstone`, `equipment`, `misc`. If you target exactly 1.21.6 this is fine, otherwise confirm runtime doesn’t reject unknown categories (older ≤1.19 snapshots will). --- `9766-9768`: **Lead recipe change alters output-shape – re-test unlocking behaviour** Removing the slime-ball shifts the pattern to a 2×2 area. Ensure the advancement/unlock JSON still references the correct pattern so the recipe appears in the book automatically. </details> <details> <summary>Obsidian.API/Commands/ArgumentParsers/StringArgumentParser.cs (6)</summary> `3-4`: **LGTM! Modern C# primary constructor pattern with proper attribute annotation.** The `[ArgumentParser]` attribute with "brigadier:string" identifier follows standard brigadier naming conventions, and the partial class with primary constructor is a clean, modern C# approach that reduces boilerplate code. --- `6-8`: **Good choice of default value and property design.** The read-only `Type` property initialized from the primary constructor parameter is well-designed. `StringType.QuotablePhrase` is a sensible default as it's the most commonly used string parsing type in command frameworks. --- `16-21`: **Proper serialization pattern implemented.** The `Write` method correctly follows the serialization pattern by calling `base.Write(writer)` first, then writing the specific parser data. Writing the enum as VarInt matches standard Minecraft protocol serialization practices. --- `24-29`: **Consistent implementation pattern for GUID parser.** The `GuidArgumentParser` follows the same solid pattern as `StringArgumentParser` with appropriate "minecraft:uuid" identifier. Using `Guid.Empty` as the default is safe, though consider if a different default might be more meaningful in your use case. --- `36-41`: **Correct UUID serialization implementation.** The `Write` method properly uses `WriteUuid` for GUID serialization, which is appropriate for Minecraft protocol UUID handling. --- `44-60`: **Well-designed enum with clear documentation.** The `StringType` enum is excellently documented with clear descriptions of each parsing behavior. The explicit int values (0, 1, 2) align with brigadier string parser conventions, and the documentation helps developers understand the differences between `SingleWord`, `QuotablePhrase`, and `GreedyPhrase`. </details> <details> <summary>Obsidian/Assets/tags.json (5)</summary> `2338-2362`: **Verify consumer code for renamed ambient-sound tags** Three new tags (`triggers_ambient_desert_*`) replace the single, older `plays_ambient_desert_block_sounds`. Make sure every place that previously queried the old tag has been updated, otherwise ambient SFX will silently break. <!-- review_comment_end --> --- `3379-3385`: **Ensure harness-capable entity is fully wired** `entity_type.can_equip_harness` references the new `minecraft:happy_ghast`. Double-check: 1. The enum/registry entry for `HappyGhast` exists. 2. Its `EquipmentComponent` (or equivalent) recognises the *harness* slot; otherwise equipping will NOP. <!-- review_comment_end --> --- `3428-3428`: **Edge-case: underwater dismount logic** Adding Happy Ghast to `dismounts_underwater` means riders will auto-dismount when the entity submerges. Verify that the mob is actually able to become submerged (design-wise it might stay airborne). <!-- review_comment_end --> --- `4639-4674`: **Validate harness item IDs** All harness colour variants are referenced here. Make sure corresponding item JSONs are present (item_components / model / texture). Missing ones will crash the game on tag load. <!-- review_comment_end --> --- `4646-4653`: **Circular tag reference looks good** `happy_ghast_tempt_items` re-uses `#minecraft:harnesses` – nice reuse, no issues spotted. <!-- review_comment_end --> </details> <details> <summary>Obsidian/Assets/item_components.json (4)</summary> `1623-1630`: **Verify custom `can_be_sheared` flag is implemented engine-side for carpets.** Multiple carpet variants now expose `can_be_sheared` + `shearing_sound`, but the original equippable-carpet component never had this field. If the runtime doesn’t consume the flag, the value is dead weight; if it *does* consume it, confirm that: 1. The shearing action produces the expected drops or state transition. 2. Llamas wearing carpets follow the same cool-down rules as vanilla shearing. 3. Sound IDs (`item.llama_carpet.unequip`) exist in the current resource pack. Also applies to: 2031-2038, 2726-2733, 5683-5690, 9950-9955 --- `3297-3308`: **Double-check the negative waypoint modifier maths.** Using `amount: -1.0` with `operation: add_multiplied_total` effectively multiplies the base value by –1 before stacking. If the base transmit range is `N`, result becomes `N + (N × –1) = 0`, not “hidden”. Ensure this is the intended behaviour; if you merely wanted to *subtract* the full base, use `add` with `amount: -N` or clamp in code. Also applies to: 5158-5169, 7581-7592, 17129-17140, 17535-17546, 20634-20645, 25123-25134, 25839-25850 --- `20014-20021`: **Horse/saddle gear now shearable – UX & loot table impact?** `can_be_sheared: true` on saddle and horse-armour implies players can instantly strip gear with shears. Confirm that: • The server emits the correct item back to the player (not duplicated / deleted). • Wolves & horses honour existing cooldowns. • There’s no conflict with right-click unequip logic already present. Also applies to: 7137-7144, 9545-9552, 11326-11333, 12322-12329 --- `7610-7625`: **New item registrations: make sure supporting assets & localisation strings ship with the same build.** Items `dried_ghast`, `music_disc_tears`, and `happy_ghast_spawn_egg` reference: – Models: `minecraft:dried_ghast`, `minecraft:music_disc_tears`, `minecraft:happy_ghast_spawn_egg` – Lang keys: `block.minecraft.dried_ghast`, `item.minecraft.music_disc_tears`, `item.minecraft.happy_ghast_spawn_egg` – Sound event `minecraft:tears` Please verify that: 1. `.json` block / item models are present under `assets/…/models`. 2. Texture PNGs exist. 3. `en_us.json` (and other locales) contain the translate keys. 4. The new `tears` sound is referenced in the sound registry and included in the pack. Missing any of these will crash resource loading. Also applies to: 14814-14830, 10497-10512 </details> <details> <summary>Obsidian.API/Commands/ArgumentParsers/NumericArgumentParsers.cs (1)</summary> `47-47`: **Verify that multiple numeric types should share the same brigadier identifier.** Seven different numeric parser types all use `"brigadier:integer"`: - SignedIntArgumentParser (int) - UnsignedByteArgumentParser (byte) - SignedByteArgumentParser (sbyte) - SignedShortArgumentParser (short) - UnsignedShortArgumentParser (ushort) - UnsignedIntArgumentParser (uint) - DecimalArgumentParser (decimal) This seems incorrect, especially for `DecimalArgumentParser` which handles floating-point decimals, not integers. ```web What are the valid brigadier argument parser types in Minecraft 1.21.6 protocol?Also applies to: 57-57, 67-67, 77-77, 87-87, 97-97, 147-147
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Obsidian.API/_Types/HexColor.cs (1)
34-47: Consider consistency in color initialization and assess breaking change impact.The refactor from RGB byte values to hex string literals improves readability and aligns with the broader move towards string-based resource identifiers. However, there are two concerns:
- Inconsistent initialization:
BlackandWhitestill use RGB byte initialization while all other colors now use hex strings. Consider updating them for consistency:-public static readonly HexColor Black = new(0, 0, 0); +public static readonly HexColor Black = new("#000000"); -public static readonly HexColor White = new(255, 255, 255); +public static readonly HexColor White = new("#FFFFFF");
- Breaking changes: The renamed color constants (
DarkCyan→DarkAqua,Purple→DarkPurple,BrightGreen→Green,Cyan→Aqua,Pink→LightPurple) could break existing code that references the old names.Please run the following script to check for any remaining references to the old color names:
#!/bin/bash # Description: Check for references to old color constant names that were renamed. echo "Searching for references to old color constant names..." rg -i "HexColor\.(DarkCyan|Purple|BrightGreen|Cyan|Pink)\b" --type cs -A 2 -B 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
Obsidian.API/Obsidian.API.csproj(2 hunks)Obsidian.API/_Types/HexColor.cs(1 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.Tests/Nbt.cs(1 hunks)Obsidian/Commands/Modules/MainCommandModule.cs(2 hunks)Obsidian/Net/NetworkBuffer.Writing.cs(2 hunks)Obsidian/Net/NetworkBuffer.cs(1 hunks)Obsidian/Obsidian.csproj(1 hunks)Obsidian/Utilities/Extensions.Nbt.Chat.cs(4 hunks)
✅ Files skipped from review due to trivial changes (4)
- Obsidian.Nbt/Obsidian.Nbt.csproj
- Obsidian/Obsidian.csproj
- Obsidian/Net/NetworkBuffer.cs
- Obsidian.SourceGenerators/Obsidian.SourceGenerators.csproj
🚧 Files skipped from review as they are similar to previous changes (3)
- Obsidian.API/Obsidian.API.csproj
- Obsidian.Nbt/RawNbtWriter.cs
- Obsidian/Utilities/Extensions.Nbt.Chat.cs
🔇 Additional comments (7)
Obsidian/Commands/Modules/MainCommandModule.cs (2)
71-82: LGTM! Improved chat message formatting approach.The refactoring from embedded color codes to using the
Colorproperty on theChatMessageobject is a cleaner approach that better separates content from styling. This makes the code more maintainable and follows better design principles.
88-92: LGTM! Structured description formatting.The change from concatenated color strings to using
ChatMessage.Simple()with structured color application via theColorproperty is a good improvement. This approach is more readable and maintainable than embedding color codes in strings.Obsidian.API/_Types/HexColor.cs (1)
34-47: Hex color values look correct and align with Minecraft standards.The hex color values appear to be standard Minecraft color codes (e.g.,
#0000AA,#55FF55, etc.), which is appropriate for the ObsidianMC project. This change enhances code readability and maintainability.Obsidian/Net/NetworkBuffer.Writing.cs (1)
473-473: Minor whitespace cleanup approved.Obsidian.Nbt/RawNbtWriter.Primitives.cs (2)
1-4: Appropriate using statements for buffer management functionality.The new using statements support the buffer management improvements with
ArrayPool,BinaryPrimitives,Debug.Assert, andCommunityToolkit.HighPerformancefor efficient buffer resizing.
169-191: Excellent buffer management implementation.The
Reservemethod properly ensures buffer capacity before writes, preventing overflows. The implementation correctly:
- Uses
Debug.Assertfor parameter validation- Calculates required capacity including current offset
- Doubles buffer size when resizing for efficient growth
- Leverages
CommunityToolkit.HighPerformancefor safe array resizingThe integration into the
Writemethod ensures all buffer operations are protected.Obsidian.Tests/Nbt.cs (1)
13-181: Excellent addition of comprehensive NBT test coverage.Enabling these test methods significantly improves test coverage for NBT functionality:
ReadBigTestthoroughly validates reading complex NBT structures with nested compounds, lists, arrays, and all primitive typesWriteBigTestcreates and validates complex NBT data, ensuring round-trip serialization works correctly- Both tests cover edge cases and validate the NBT pipeline used throughout the system
This provides crucial validation for the NBT serialization/deserialization functionality that underpins network communication and data storage.
1 participant
Updates the protocol to 1.21.6 and also refactors the ArgumentParser from our command framework. The reason why I did it was to make sure our parser ids get updated automatically, so we don't run into issues where the protocol updates but commands don't load cause of out of date ids. And it makes the parsers api available
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Other Changes