Implement crafting and fix Tags SourceGeneration#494
Conversation
…lock and other registry id
WalkthroughThis update implements server-side crafting logic, refactors inventory click handling, and introduces efficient recipe lookup and matching for crafting grids. It also modernizes many data structures by converting classes and structs to records, enhances null safety, and adds or updates several utility and extension methods relevant to crafting and item handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Player
participant MainEventHandler
participant RecipesRegistry
participant CraftingTable
participant Client
Player->>MainEventHandler: InventoryClick event
MainEventHandler->>CraftingTable: Read crafting grid
MainEventHandler->>RecipesRegistry: FindRecipe(grid)
RecipesRegistry->>RecipesRegistry: Lookup shaped/shapeless match
RecipesRegistry-->>MainEventHandler: Return matching recipe or null
alt Recipe found
MainEventHandler->>CraftingTable: Set result slot (recipe output)
MainEventHandler->>Client: Send slot update packet
else No recipe
MainEventHandler->>CraftingTable: Clear result slot
MainEventHandler->>Client: Send null slot update packet
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
api
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🔭 Outside diff range comments (3)
Obsidian/Net/Packets/Common/UpdateTagsPacket.cs (2)
12-13: Incorrect count written to the stream
writer.WriteVarInt(Tags.Count - 1);unconditionally subtracts 1.
If the"worldgen"entry (or any other invalid entry) is absent, the count becomes off-by-one (or even negative => huge VarInt). The packet will corrupt the wire format.- writer.WriteVarInt(Tags.Count - 1); + var validTags = Tags.Where(kv => !InvalidResourceIds.Contains(kv.Key)).ToArray(); + writer.WriteVarInt(validTags.Length);
14-18: Hard-coded filter duplicatesInvalidslistThe loop skips only the literal
"worldgen"key, ignoring the other entries inInvalids. This duplicates authority and risks future divergence.- foreach (var (resourceId, tags) in Tags) + foreach (var (resourceId, tags) in validTags) { - if (resourceId == "worldgen") - continue;Obsidian/Utilities/Converters/IngredientConverter.cs (1)
28-44: Add null safety check for tag entries.The code uses
tag!.Entrieswithout verifying thattag.Entriesis not null, which could cause a null reference exception.var tag = TagsRegistry.Item.All.FirstOrDefault(x => x.Name == resourceLocation); if (tag != null) { - foreach (var id in tag!.Entries) + foreach (var id in tag.Entries ?? []) { var item = ItemsRegistry.Get(id); ingredient.Add(new ItemStack(item, 1)); } }
🧹 Nitpick comments (17)
Obsidian/Net/Packets/Common/UpdateTagsPacket.cs (1)
6-6: Naming & immutabilityField should follow .NET conventions (
PascalCase) and bereadonlyto prevent runtime mutation.Obsidian.API/Inventory/DataComponents/ConsumableDataComponent.cs (1)
55-55: Remove extra blank line for consistency.Consider removing the extra blank line to maintain consistent formatting.
- }Obsidian/Utilities/Converters/IngredientConverter.cs (1)
4-5: Remove unused imports.The
System.DiagnosticsandSystem.Reflectionimports are not used in this file and should be removed to keep the code clean.-using System.Diagnostics; -using System.Reflection;Obsidian.API/Crafting/Ingredient.cs (1)
25-25: Consider performance optimization for ingredient matching.The
CanBemethod provides useful functionality for recipe matching. However, usingLINQ Any()withEquals()may have performance implications for large ingredient collections.Consider a more efficient implementation if performance becomes an issue:
-public bool CanBe(ItemStack item) => this.items.Any(x => x.Equals(item)); +public bool CanBe(ItemStack item) +{ + for (int i = 0; i < this.items.Count; i++) + { + if (this.items[i].Equals(item)) + return true; + } + return false; +}Alternatively, consider a more descriptive method name like
ContainsorMatches.Obsidian.API/Inventory/ItemStack.cs (1)
83-96: Consider improving the hash combination for RemoveComponents.While the GetHashCode implementation is generally good, using XOR for combining hash codes of collection items can be problematic as it's order-independent and cancels out duplicates.
Consider using a more robust approach:
-int removeComponentsHash = 0; -foreach (var componentType in this.RemoveComponents) - removeComponentsHash ^= componentType.GetHashCode(); +int removeComponentsHash = this.RemoveComponents + .Aggregate(0, (current, componentType) => + HashCode.Combine(current, componentType.GetHashCode()));Obsidian/Events/MainEventHandler.Inventory.cs (3)
43-44: Consider conditional execution of crafting logicThe
HandleCraftingAsyncmethod is called after every click type, which may cause unnecessary processing for non-crafting operations like throwing items or quick crafting. Consider checking if the container is a crafting table before calling this method to avoid redundant recipe lookups.- await this.HandleCraftingAsync(args); + if (args.Container is CraftingTable) + await this.HandleCraftingAsync(args);
233-246: Consider logging levels for productionThe logging statements use
LogInformationwhich might be too verbose for production environments. Consider usingLogDebugorLogTracefor detailed item interaction logs.- logger.LogInformation("Item count: {count} - {type}", player.CarriedItem.Count, player.CarriedItem.Holder.UnlocalizedName); + logger.LogDebug("Item count: {count} - {type}", player.CarriedItem.Count, player.CarriedItem.Holder.UnlocalizedName);- logger.LogInformation("Setting item in container slot: {slot} - {item}", clickedSlot, player.CarriedItem.Holder.UnlocalizedName); + logger.LogDebug("Setting item in container slot: {slot} - {item}", clickedSlot, player.CarriedItem.Holder.UnlocalizedName);
241-244: Clarify ItemStack arithmetic and constructionThe operator overloading
player.CarriedItem - 1might not be immediately clear to other developers. Additionally, the ItemStack construction on line 244 appears complex.Consider adding a comment to clarify the intent:
case 1: + // Subtract one item from the carried stack var newItem = player.CarriedItem - 1; player.CarriedItem = newItem; + // Place a single item in the clicked slot container.SetItem(clickedSlot, new(newItem.Holder, 1, newItem));Obsidian/Registries/RecipesRegistry.cs (5)
35-35: Simplify return statementThe null coalescing operator with null is redundant.
- return shapelessMatch ?? null; + return shapelessMatch;
43-44: Consider using IsNullOrAir pattern for consistencyThe null check might not handle air items correctly. Consider using the
IsNullOrAir()extension method for consistency with other inventory handling code.- if (grid[i] != null) + if (grid[i] != null && !grid[i].IsAir)
151-151: Consider using a more efficient key structureString concatenation for dictionary keys could be replaced with a value tuple or custom struct for better performance.
Instead of:
var key = string.Join(":", ingredientsByOffset.Keys.OrderBy(k => k));Consider using:
var key = new RecipeShape(ingredientsByOffset.Keys.OrderBy(k => k).ToArray());Where
RecipeShapeis a readonly record struct with proper equality implementation.
170-176: Simplify dictionary initialization with GetValueOrDefaultThe TryGetValue pattern can be simplified using modern C# features.
- if (!recipeKeyDictionary.TryGetValue(ingredientCount, out var value)) - { - value = []; - recipeKeyDictionary[ingredientCount] = value; - } - - value.Add(recipe); + recipeKeyDictionary.TryAdd(ingredientCount, []); + recipeKeyDictionary[ingredientCount].Add(recipe);Or even simpler with collection expressions:
var list = recipeKeyDictionary.GetValueOrDefault(ingredientCount) ?? []; list.Add(recipe); recipeKeyDictionary[ingredientCount] = list;
182-182: Consider moving type definition to the topThe
CanonicalReciperecord should be defined at the top of the file with other type definitions for better code organization.Obsidian.SourceGenerators/Registry/RegistryAssetsGenerator.Tags.cs (2)
24-24: Consider documenting or refactoring the flat_level_generator_preset workaround.The comment indicates this is a workaround that will be changed later. Consider adding a TODO comment with more context about why this special case exists and what the proper solution would be.
- //Workaround for flat_level_generator_preset will change this up another time + // TODO: Workaround for flat_level_generator_preset - requires special handling due to [explain reason] + // This should be refactored when [explain condition or timeline]
62-71: Consider extracting the tag compilation name logic for better readability.The nested conditional logic for building tag references could be extracted into a helper method for clarity.
- if(tag.Type == tag.Parent) - builder.Append(tag.Type.ToPascalCase()).Append(".").Append(tag.PropertyName).Append(", "); - else - builder.Append(tag.Parent.ToPascalCase()).Append(".").Append(tag.Type.GetActualType(1).ToPascalCase()).Append(".").Append(tag.PropertyName).Append(", "); + builder.Append(GetTagReference(tag)).Append(", ");Add a helper method:
private static string GetTagReference(Tag tag) { if (tag.Type == tag.Parent) return $"{tag.Type.ToPascalCase()}.{tag.PropertyName}"; else return $"{tag.Parent.ToPascalCase()}.{tag.Type.GetActualType(1).ToPascalCase()}.{tag.PropertyName}"; }Obsidian.SourceGenerators/Registry/Models/Assets.cs (2)
195-195: Consider using constants for special tag names.The hardcoded tag names "minecraft:water" and "minecraft:lava" should be defined as constants for better maintainability.
+ private const string WaterTag = "minecraft:water"; + private const string LavaTag = "minecraft:lava"; + // In GetTags method: - taggables.AddRange(blocks.Where(x => x.Tag is not "minecraft:water" or "minecraft:lava")); + taggables.AddRange(blocks.Where(x => x.Tag is not WaterTag or LavaTag));
210-212: Address the duplicate VerifyTags call.The comment indicates uncertainty about why VerifyTags needs to be called twice. This suggests a potential design issue that should be investigated and properly documented or fixed.
Would you like me to help analyze why the duplicate call might be necessary and suggest a proper solution?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
Obsidian.API/Containers/CraftingTable.cs(0 hunks)Obsidian.API/Crafting/Ingredient.cs(1 hunks)Obsidian.API/Effects/ConsumeEffect.cs(1 hunks)Obsidian.API/Events/ContainerClickEventArgs.cs(1 hunks)Obsidian.API/Inventory/DataComponents/BlockPredicatesDataComponent.cs(2 hunks)Obsidian.API/Inventory/DataComponents/CanPlaceOnDataComponent.cs(1 hunks)Obsidian.API/Inventory/DataComponents/ConsumableDataComponent.cs(2 hunks)Obsidian.API/Inventory/DataComponents/FoodDataComponent.cs(1 hunks)Obsidian.API/Inventory/DataComponents/JukeboxPlayableDataComponent.cs(1 hunks)Obsidian.API/Inventory/DataComponents/MapDecorationDataComponent.cs(2 hunks)Obsidian.API/Inventory/DataComponents/PotionContentsDataComponent.cs(2 hunks)Obsidian.API/Inventory/DataComponents/ToolDataComponent.cs(3 hunks)Obsidian.API/Inventory/Item.cs(2 hunks)Obsidian.API/Inventory/ItemStack.cs(3 hunks)Obsidian.API/Registry/Codecs/ArmorTrims/TrimDescription.cs(1 hunks)Obsidian.API/Registry/Codecs/ArmorTrims/TrimMaterial/TrimMaterialElement.cs(1 hunks)Obsidian.API/Utilities/Extensions.cs(2 hunks)Obsidian.API/_Interfaces/IPlayer.cs(2 hunks)Obsidian.API/_Types/SoundEvent.cs(1 hunks)Obsidian.SourceGenerators/Extensions.cs(1 hunks)Obsidian.SourceGenerators/Registry/Models/Assets.cs(4 hunks)Obsidian.SourceGenerators/Registry/Models/Block.cs(1 hunks)Obsidian.SourceGenerators/Registry/Models/Fluid.cs(1 hunks)Obsidian.SourceGenerators/Registry/Models/ITaggable.cs(1 hunks)Obsidian.SourceGenerators/Registry/Models/Item.cs(1 hunks)Obsidian.SourceGenerators/Registry/Models/Tag.cs(3 hunks)Obsidian.SourceGenerators/Registry/RegistryAssetsGenerator.Tags.cs(4 hunks)Obsidian/Client.cs(1 hunks)Obsidian/Entities/Player.cs(5 hunks)Obsidian/Events/MainEventHandler.Inventory.cs(4 hunks)Obsidian/Events/MainEventHandler.cs(1 hunks)Obsidian/HashedItemStack.cs(1 hunks)Obsidian/Net/ClientHandlers/PlayClientHandler.cs(1 hunks)Obsidian/Net/Packets/Common/UpdateTagsPacket.cs(1 hunks)Obsidian/Net/Packets/Play/Serverbound/ContainerClickPacket.cs(3 hunks)Obsidian/Net/Packets/Play/Serverbound/SetCreativeModeSlotPacket.cs(0 hunks)Obsidian/Registries/RecipesRegistry.cs(2 hunks)Obsidian/Utilities/Converters/IngredientConverter.cs(3 hunks)
💤 Files with no reviewable changes (2)
- Obsidian.API/Containers/CraftingTable.cs
- Obsidian/Net/Packets/Play/Serverbound/SetCreativeModeSlotPacket.cs
🧰 Additional context used
🪛 GitHub Check: build
Obsidian.API/Inventory/Item.cs
[warning] 6-6:
'Item' defines operator == or operator != but does not override Object.GetHashCode()
[warning] 6-6:
'Item' overrides Object.Equals(object o) but does not override Object.GetHashCode()
🔇 Additional comments (45)
Obsidian.API/Inventory/DataComponents/JukeboxPlayableDataComponent.cs (1)
2-2: LGTM: Record conversion enhances equality semantics.Converting to a record class provides value-based equality and immutability benefits that are essential for data components used in crafting and inventory operations.
Obsidian.API/Inventory/DataComponents/ConsumableDataComponent.cs (1)
5-5: LGTM: Record conversion provides better equality for consumable items.Converting to a record class is appropriate for this component as it contains multiple required properties that benefit from value-based equality in crafting and inventory systems.
Obsidian.API/_Types/SoundEvent.cs (1)
2-2: LGTM: Record struct enhances value equality for sound events.Converting to a readonly record struct provides compiler-generated value-based equality and
ToString()while maintaining immutability and value-type performance characteristics.Obsidian.API/Utilities/Extensions.cs (2)
1-2: LGTM: Added necessary imports for inventory operations.The
Obsidian.API.Inventoryimport enables the newIsNullOrAirextension method, andSystem.Diagnosticssupports theUnreachableExceptionusage.
13-14: LGTM: Useful utility method for crafting system.The
IsNullOrAirextension method provides a concise way to check for empty or non-existent item stacks, which is essential for recipe validation in the crafting system implementation.Obsidian.API/Effects/ConsumeEffect.cs (1)
2-2: LGTM: Record struct conversion aligns with API standardization.Converting to a readonly record struct provides value-based equality that's important when this type is used within data components for crafting and inventory operations.
Obsidian.API/Inventory/DataComponents/FoodDataComponent.cs (1)
8-8: LGTM! Clean record conversion.The conversion from
sealed classtosealed record classis appropriate and introduces value-based equality semantics without any breaking changes. This aligns well with the broader standardization of record types across data components.Obsidian.API/Inventory/DataComponents/PotionContentsDataComponent.cs (2)
2-2: LGTM! Appropriate record class conversion.The conversion to
sealed record classprovides value-based equality semantics while maintaining all existing functionality.
50-50: LGTM! Good record struct conversion.Converting the nested
Potionstruct to areadonly record structis appropriate since it already usesinitproperties and benefits from record semantics.Obsidian.API/Inventory/DataComponents/MapDecorationDataComponent.cs (2)
5-5: LGTM! Clean record class conversion.The conversion maintains all existing functionality while adding value-based equality semantics.
46-46: LGTM! Well-suited record struct conversion.The
MapDecorationstruct is well-suited for record conversion with its existinginitproperties and immutable design.Obsidian.API/Inventory/DataComponents/BlockPredicatesDataComponent.cs (4)
1-1: LGTM! Required using statement.The addition of
using Obsidian.Nbt;is necessary for theNbtCompoundtype used in the improvedNbtproperty.
4-4: LGTM! Appropriate abstract record class conversion.Converting to an abstract record class maintains polymorphism while adding record semantics to derived types.
20-20: LGTM! Improved type safety with record conversion.The conversion to
readonly record structis appropriate, and changing theNbtproperty fromobject?toNbtCompound?significantly improves type safety.Also applies to: 32-32
35-35: LGTM! Clean record struct conversion.The
BlockPropertyconversion toreadonly record structis appropriate and maintains existing functionality.Obsidian.API/Inventory/DataComponents/CanPlaceOnDataComponent.cs (3)
2-2: LGTM! Appropriate record class conversion.The conversion to
sealed record classis consistent with the broader refactoring pattern and maintains existing functionality.
9-9: LGTM! Record class conversion is appropriate.The conversion to
sealed record classis consistent with the pattern.
11-11: Fix incorrect DataComponentType.The
CanBreakDataComponent.Typeproperty returnsDataComponentType.CanPlaceOninstead of the expectedDataComponentType.CanBreak. This appears to be a copy-paste error.- public override DataComponentType Type => DataComponentType.CanPlaceOn; + public override DataComponentType Type => DataComponentType.CanBreak;Likely an incorrect or invalid review comment.
Obsidian/Utilities/Converters/IngredientConverter.cs (1)
17-75: LGTM on the refactored deserialization logic.The upfront ingredient initialization and explicit handling of different JSON value kinds improves code clarity and maintainability. The exact tag matching approach aligns well with the updated tag system.
Obsidian/Client.cs (1)
367-368: LGTM on defensive null check.Adding a null check after dequeuing packets prevents potential null reference exceptions and improves the robustness of packet processing. This is good defensive programming practice.
Obsidian/Net/ClientHandlers/PlayClientHandler.cs (1)
89-91: LGTM on SetCarriedItemPacket handling.The addition of packet ID 52 for
SetCarriedItemPacketfollows the established pattern and integrates properly with the async packet handling infrastructure.Obsidian/HashedItemStack.cs (1)
18-22: LGTM on null safety improvement.Making the parameter nullable and adding the null check improves the method's robustness and aligns with the broader null safety enhancements across the codebase.
Obsidian.API/Registry/Codecs/ArmorTrims/TrimDescription.cs (1)
2-2: LGTM: Record conversion enhances data semantics.Converting to a record class provides value-based equality and immutability benefits without changing the existing API surface. This aligns well with the data transfer nature of this type.
Obsidian/Events/MainEventHandler.cs (2)
1-2: LGTM: Added necessary imports for logging and containers.The additional imports support the enhanced event handling capabilities being introduced.
9-9: LGTM: Dependency injection of logger enhances observability.Adding the logger parameter enables better traceability of event processing, which is especially valuable for inventory and container interactions.
Obsidian.API/Registry/Codecs/ArmorTrims/TrimMaterial/TrimMaterialElement.cs (1)
2-2: LGTM: Record conversion improves data semantics.Similar to other record conversions in this PR, this change provides value-based equality and immutability benefits for this data transfer type without breaking existing functionality.
Obsidian.API/Events/ContainerClickEventArgs.cs (1)
10-10: LGTM: Added defensive check for invalid slot access.The conditional check prevents accessing container items when
ClickedSlotis -999 (likely indicating no valid slot), improving robustness of the container interaction system.Obsidian.API/_Interfaces/IPlayer.cs (2)
9-9: LGTM: Added container tracking property.The
CurrentContainerIdproperty supports container state management for the crafting system implementation.
21-24: LGTM: Improved property naming and documentation.Renaming
LastClickedItemtoCarriedItemwith clear documentation better represents the item's role during container interactions. The semantic improvement aligns with the crafting system requirements.Obsidian/Net/Packets/Play/Serverbound/ContainerClickPacket.cs (3)
78-78: Good handling of the special slot -999 case.The slot value -999 represents clicking outside the inventory window in Minecraft protocol. Setting clickedItem to null in this case prevents invalid container access.
81-81: Proper null checks added for item comparison.The addition of null checks for both
CarriedItemandclickedItembefore comparison prevents potential null reference exceptions.
98-108: Improved container selection logic with proper null handling.The container selection based on slot index correctly handles the case where slots exceed the container size, switching to player inventory as expected. The null checks for
hashedItemandcheckedItemensure safe handling of empty slots.Obsidian.API/Inventory/Item.cs (1)
33-33: Clear ToString() implementation.The ToString() override provides a clear representation of the item using its unlocalized name.
Obsidian.API/Inventory/DataComponents/ToolDataComponent.cs (1)
4-4: Good refactoring to record types for value semantics.Converting
ToolDataComponent,ToolRule, andIdSetto record types provides automatic value-based equality, immutability, and other benefits without changing the serialization behavior. This aligns well with the data-oriented nature of these types.Also applies to: 40-40, 63-63
Obsidian/Entities/Player.cs (2)
20-20: Good property refactoring for better encapsulation.Converting the private field to a public property
CurrentContainerIdand renamingLastClickedItemtoCarriedItemimproves API clarity and aligns with the interface changes.Also applies to: 70-70
606-609: Container ID cycling logic is correct.The modulo operation ensures the container ID cycles from 1 to 255, which is the expected behavior for Minecraft container IDs.
Obsidian.API/Inventory/ItemStack.cs (2)
25-25: Good API improvement using IEnumerable.Changing the parameter type from
List<IDataComponent>toIEnumerable<IDataComponent>provides more flexibility for callers without breaking the implementation.Also applies to: 63-63
98-98: Clear ToString() implementation.Good choice to return the unlocalized name of the holder item for string representation.
Obsidian/Events/MainEventHandler.Inventory.cs (1)
68-73: Verify crafting result slot index in ContainerSetSlotPacket
It appears the crafting output is typically mapped to slot 9, yet here we’re sending it as slot 0. Please confirm thatSlotaligns with the container’s expected index so the client renders the result correctly.
- File:
Obsidian/Events/MainEventHandler.Inventory.cs, lines 68–73: currentlySlot = 0; adjust to the proper crafting result slot if needed.Obsidian.SourceGenerators/Extensions.cs (1)
16-18: Verify CompileName branching and GetActualType index safetyPlease double-check that the new equality logic and string-splitting call align with your Tag implementation:
• File: Obsidian.SourceGenerators/Extensions.cs
Method: CompileName (lines 16–18)
– Confirm that swapping fromtag.Parent != tag.Typetotag.Type == tag.Parentstill correctly distinguishes root vs. nested tags based on your Tag.Type and Tag.Parent properties.
– Ensure everytag.Typestring passed toGetActualType(1)actually contains a “/” (and thus at least two segments), or add bounds-checking/fallback to avoid anIndexOutOfRangeException.I wasn’t able to locate the
Tagclass in the repo, so please verify itsType/Parentdefinitions and that no edge cases slip through here.Obsidian.SourceGenerators/Registry/Models/Block.cs (1)
8-9: LGTM! Clean implementation of ITaggable interface.The Type and Parent properties correctly return "block" for both values, which aligns with the updated ITaggable interface contract and maintains consistency across registry models.
Obsidian.SourceGenerators/Registry/Models/Item.cs (1)
11-12: LGTM! Consistent implementation of ITaggable interface.The Type and Parent properties correctly return "item", maintaining consistency with the pattern established in other registry model classes.
Obsidian.SourceGenerators/Registry/Models/ITaggable.cs (1)
5-7: LGTM! Well-designed interface extension for type metadata.The addition of Type and Parent properties to ITaggable provides a clean way to expose type hierarchy information, supporting the tag system refactoring while maintaining backward compatibility with existing members.
Obsidian.SourceGenerators/Registry/Models/Fluid.cs (1)
2-12: LGTM! Excellent modernization with primary constructor.The refactoring to primary constructor syntax is clean and the Type/Parent properties correctly implement the ITaggable interface contract. The code is more concise while maintaining the same functionality.
Obsidian.SourceGenerators/Registry/Models/Tag.cs (1)
14-20: Well-structured refactoring to simplified tag model.The constructor changes and property renaming (PropertyName/Identifier) improve clarity and consistency across the codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
Obsidian.API/Inventory/ItemStack.cs (1)
64-64: LGTM: Correct equality implementationThe Equals method correctly compares the Holder and base components without negation. This appears to have been fixed since the previous review.
Obsidian/Registries/RecipesRegistry.cs (1)
94-96: LGTM: Bounds checking implementedThe bounds check
if (gridSlot >= 9) return false;correctly prevents out-of-bounds access, addressing the concern from the previous review.
🧹 Nitpick comments (1)
Obsidian/Registries/RecipesRegistry.cs (1)
50-52: Consider edge case: multiple minimum slotsWhen multiple slots have the same minimum index,
Min()will return the first one, but the algorithm assumes a unique anchor. This should work correctly, but consider adding a comment explaining this behavior.+ // Use the smallest occupied slot as anchor for consistent relative positioning int anchorSlot = occupiedSlots.Min();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Obsidian.API/Inventory/DataComponents/DataComponentsStorage.cs(2 hunks)Obsidian.API/Inventory/Item.cs(2 hunks)Obsidian.API/Inventory/ItemStack.cs(2 hunks)Obsidian.SourceGenerators/Registry/Models/Assets.cs(4 hunks)Obsidian/Events/MainEventHandler.Inventory.cs(4 hunks)Obsidian/Net/Packets/Play/Serverbound/ContainerClickPacket.cs(3 hunks)Obsidian/Registries/RecipesRegistry.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Obsidian/Net/Packets/Play/Serverbound/ContainerClickPacket.cs
- Obsidian.API/Inventory/Item.cs
- Obsidian/Events/MainEventHandler.Inventory.cs
- Obsidian.SourceGenerators/Registry/Models/Assets.cs
🔇 Additional comments (9)
Obsidian.API/Inventory/DataComponents/DataComponentsStorage.cs (2)
6-7: LGTM: Improved extensibility with protected propertiesConverting the private dictionaries to protected properties enables subclass access while maintaining encapsulation from external classes. This change supports the broader refactoring towards record-based data components.
44-44: LGTM: Clean API additionThe new
ContainsKeymethod provides a clear, consistent way to check for component existence, complementing the existing component management methods.Obsidian.API/Inventory/ItemStack.cs (3)
25-25: LGTM: Improved constructor flexibilityChanging the parameter from
List<IDataComponent>toIEnumerable<IDataComponent>makes the constructor more flexible by accepting various collection types.
66-67: LGTM: Comprehensive hash code implementationThe GetHashCode override properly combines all relevant fields that affect equality, ensuring consistent hash behavior.
76-79: LGTM: Efficient component replacement logicUsing
ContainsKeyand direct assignment toInternalStorageis more efficient than the previous approach and clearly expresses the intent to replace existing components.Obsidian/Registries/RecipesRegistry.cs (4)
10-11: LGTM: Efficient lookup data structuresUsing
FrozenDictionaryfor recipe lookups is an excellent choice for read-heavy operations, providing optimal performance for recipe matching during gameplay.
28-36: LGTM: Clean recipe finding interfaceThe
FindRecipemethod provides a clear entry point that tries shaped recipes first, then falls back to shapeless recipes, which is the correct priority order.
107-122: LGTM: Correct shapeless recipe matchingThe algorithm correctly handles shapeless recipes by removing matched ingredients from a copy of the grid items, ensuring each ingredient is consumed exactly once.
187-187: LGTM: Clean record definitionThe
CanonicalReciperecord appropriately encapsulates the normalized ingredient mapping and original recipe reference.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Obsidian.API/Inventory/DataComponent.cs (1)
2-11: Consider making serialization methods abstract or add documentation.The
DataComponentabstract record introduces a clean foundation for the data component refactoring. However, the virtual methods with empty implementations may be confusing:Consider one of these approaches:
Option 1: Make methods abstract if all components need custom serialization:
- public virtual void Write(INetStreamWriter writer) { } - public virtual void Read(INetStreamReader reader) { } + public abstract void Write(INetStreamWriter writer); + public abstract void Read(INetStreamReader reader);Option 2: Add documentation explaining the default behavior:
+ /// <summary> + /// Writes component data to the network stream. Override for custom serialization. + /// </summary> public virtual void Write(INetStreamWriter writer) { } + /// <summary> + /// Reads component data from the network stream. Override for custom deserialization. + /// </summary> public virtual void Read(INetStreamReader reader) { }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
Obsidian.API/Inventory/AttributeModifier.cs(1 hunks)Obsidian.API/Inventory/DataComponent.cs(1 hunks)Obsidian.API/Inventory/DataComponents/BlockPredicatesDataComponent.cs(1 hunks)Obsidian.API/Inventory/DataComponents/ComponentBuilder.Map.cs(1 hunks)Obsidian.API/Inventory/DataComponents/ComponentBuilder.cs(8 hunks)Obsidian.API/Inventory/DataComponents/ConsumableDataComponent.cs(3 hunks)Obsidian.API/Inventory/DataComponents/DataComponentsStorage.cs(2 hunks)Obsidian.API/Inventory/DataComponents/EquippableDataComponent.cs(4 hunks)Obsidian.API/Inventory/DataComponents/FireworksDataComponent.cs(1 hunks)Obsidian.API/Inventory/DataComponents/FoodDataComponent.cs(2 hunks)Obsidian.API/Inventory/DataComponents/JukeboxPlayableDataComponent.cs(2 hunks)Obsidian.API/Inventory/DataComponents/LodestoneTrackerDataComponent.cs(1 hunks)Obsidian.API/Inventory/DataComponents/MapDecorationDataComponent.cs(3 hunks)Obsidian.API/Inventory/DataComponents/PotionContentsDataComponent.cs(3 hunks)Obsidian.API/Inventory/DataComponents/ProfileDataComponent.cs(1 hunks)Obsidian.API/Inventory/DataComponents/SimpleDataComponent.cs(2 hunks)Obsidian.API/Inventory/DataComponents/ToolDataComponent.cs(6 hunks)Obsidian.API/Inventory/DataComponents/TrimDataComponent.cs(1 hunks)Obsidian.API/Inventory/DataComponents/UseCooldownDataComponent.cs(2 hunks)Obsidian.API/Inventory/Enchantment.cs(1 hunks)Obsidian.API/Inventory/IDataComponent.cs(0 hunks)Obsidian.API/Inventory/ItemStack.cs(3 hunks)Obsidian.API/Registries/ItemsRegistry.cs(1 hunks)Obsidian.API/Registry/Codecs/ArmorTrims/TrimPattern/TrimPatternElement.cs(1 hunks)Obsidian.API/_Interfaces/INetStreamReader.cs(1 hunks)Obsidian.API/_Interfaces/INetStreamWriter.cs(1 hunks)Obsidian.API/_Types/FireworkExplosion.cs(1 hunks)Obsidian.API/_Types/HoverComponent.cs(1 hunks)Obsidian.API/_Types/TradeItem.cs(1 hunks)Obsidian/Commands/CommandNode.cs(1 hunks)Obsidian/Events/MainEventHandler.Inventory.cs(4 hunks)Obsidian/Net/NetworkBuffer.Reading.cs(2 hunks)Obsidian/Net/NetworkBuffer.Writing.cs(1 hunks)Obsidian/Net/Packets/Play/Clientbound/MerchantOffersPacket.cs(1 hunks)
💤 Files with no reviewable changes (1)
- Obsidian.API/Inventory/IDataComponent.cs
✅ Files skipped from review due to trivial changes (6)
- Obsidian.API/Registry/Codecs/ArmorTrims/TrimPattern/TrimPatternElement.cs
- Obsidian.API/_Types/FireworkExplosion.cs
- Obsidian.API/Registries/ItemsRegistry.cs
- Obsidian.API/Inventory/DataComponents/LodestoneTrackerDataComponent.cs
- Obsidian.API/_Types/HoverComponent.cs
- Obsidian.API/Inventory/DataComponents/ComponentBuilder.cs
🚧 Files skipped from review as they are similar to previous changes (8)
- Obsidian.API/Inventory/DataComponents/FoodDataComponent.cs
- Obsidian.API/Inventory/DataComponents/PotionContentsDataComponent.cs
- Obsidian.API/Inventory/DataComponents/JukeboxPlayableDataComponent.cs
- Obsidian.API/Inventory/DataComponents/ConsumableDataComponent.cs
- Obsidian.API/Inventory/DataComponents/BlockPredicatesDataComponent.cs
- Obsidian.API/Inventory/DataComponents/MapDecorationDataComponent.cs
- Obsidian/Events/MainEventHandler.Inventory.cs
- Obsidian.API/Inventory/DataComponents/ToolDataComponent.cs
🔇 Additional comments (41)
Obsidian.API/_Interfaces/INetStreamReader.cs (1)
35-35: LGTM! Array return type improves performance.The change from
List<TValue>toTValue[]aligns with the broader refactoring to standardize collection types. Arrays are more efficient for serialization and provide better performance characteristics for network operations.Obsidian/Net/Packets/Play/Clientbound/MerchantOffersPacket.cs (1)
21-21: LGTM! Consistent with array standardization.The change from
List<TradeEntry>toTradeEntry[]is consistent with the broader refactoring to standardize collection types across the codebase. The serialization logic correctly handles the array type.Obsidian.API/Inventory/AttributeModifier.cs (1)
2-2: Excellent improvement to value semantics.Converting from
readonly structtoreadonly record structprovides compiler-generated value-based equality,ToString(), andwith-expression support while preserving immutability and value-type performance. This enhances the type without breaking existing functionality.Obsidian/Commands/CommandNode.cs (2)
24-24: LGTM! Array conversion aligns with serialization changes.The
.ToArray()call is necessary sinceWriteLengthPrefixedArraynow expects arrays instead of lists. This change is consistent with the broader refactoring of network serialization interfaces.
16-16: Behavioral Implications of HashSet→List ConversionSwitching
Childrenfrom aHashSet<CommandNode>toList<CommandNode>works for ordered serialization but changes behavior:
- Duplicates now possible – neither
AddChildnor directChildren.Addchecks for existing entries.- Order preserved – insertion order is retained (likely intended).
- Contains cost –
Containsdrops from O(1) to O(n).Please review whether child-node uniqueness must be enforced. Here are the key locations to inspect:
- Obsidian/Commands/CommandNode.cs:47
public void AddChild(CommandNode child) => Children.Add(child);- Obsidian/Entities/Player.cs:536
parent.Children.Add(child);If duplicates are undesired, consider adding a guard (e.g.
if (!Children.Contains(child))) or reintroducing a set-based check.Obsidian.API/Inventory/DataComponents/ComponentBuilder.Map.cs (1)
7-7: LGTM! Type alignment with architectural refactoring.The change from
Func<IDataComponent?>toFunc<DataComponent?>correctly aligns with the broader refactoring that replaces theIDataComponentinterface with the abstractDataComponentrecord class.Obsidian.API/Inventory/Enchantment.cs (1)
3-7: Excellent modernization to readonly record struct.Converting
Enchantmentto areadonly record structwithrequired initproperties provides value-based equality semantics, immutability guarantees, and better performance characteristics. This aligns well with the broader architectural shift toward record types.Obsidian.API/_Types/TradeItem.cs (2)
24-24: LGTM! Consistent with data component refactoring.The change from
List<IDataComponent>toDataComponent[]aligns with the broader architectural shift from interface-based to record-based data components, providing better performance and immutability guarantees.
32-32: Constructor parameter updated correctly.The constructor parameter type change matches the property type update and maintains consistency across the record definition.
Obsidian/Net/NetworkBuffer.Reading.cs (2)
388-388: LGTM! Type inference reflects method return type change.Using
varhere is appropriate as the return type is clear from theReadLengthPrefixedArraymethod call, and it automatically reflects the change fromList<int>?toint[]?.
398-407: Excellent performance improvement with arrays.Converting from
List<TValue>toTValue[]provides better performance and lower memory overhead for fixed-size collections. The implementation correctly allocates a fixed-size array and populates it, which is more efficient than using a dynamic list.Obsidian.API/Inventory/DataComponents/TrimDataComponent.cs (3)
5-5: LGTM! Architectural alignment with record-based inheritance.Converting from
IDataComponentinterface implementation toDataComponentrecord inheritance aligns with the broader architectural refactoring and provides record semantics benefits.
7-9: Properties correctly override base class members.Using
overrideforTypeandIdentifierproperties correctly reflects their inheritance from the abstractDataComponentbase record class.
17-23: Methods correctly override base class implementations.The
ReadandWritemethods properly override the virtual methods from theDataComponentbase class, maintaining the same functionality while conforming to the new inheritance model.Also applies to: 25-30
Obsidian.API/_Interfaces/INetStreamWriter.cs (2)
54-54: LGTM! Parameter type change aligns with array standardization.The change from
params List<Enchantment>toparams Enchantment[]is consistent with the broader refactoring effort to standardize on arrays for network serialization.
56-56: LGTM! Generic method signature updated consistently.The parameter type change from
params List<TValue>toparams TValue[]maintains consistency with the enchantment-specific overload and the overall array standardization effort.Obsidian/Net/NetworkBuffer.Writing.cs (2)
422-428: LGTM! Implementation correctly updated for array parameters.The method signature and implementation have been properly updated to use arrays instead of lists. The use of
.Count()extension method is appropriate for array enumeration.
430-438: LGTM! Enchantment method consistently updated.The implementation correctly mirrors the generic method changes, using arrays and the
.Count()extension method appropriately.Obsidian.API/Inventory/DataComponents/ProfileDataComponent.cs (4)
2-2: LGTM! Architectural update to record-based inheritance.The change from implementing
IDataComponentinterface to inheriting fromDataComponentrecord aligns with the broader architectural refactoring mentioned in the AI summary.
4-6: LGTM! Property overrides correctly implemented.The
TypeandIdentifierproperties now properly override the base class declarations with the appropriate modifiers.
12-12: LGTM! Collection type standardized to array.The change from
List<SkinProperty>toSkinProperty[]is consistent with the broader effort to standardize on arrays for better serialization performance and consistency.
14-27: LGTM! Serialization methods correctly updated.The
ReadandWritemethods now properly override the base class methods while maintaining the same serialization logic. The method bodies correctly handle the array-basedPropertiescollection.Obsidian.API/Inventory/DataComponents/EquippableDataComponent.cs (4)
5-5: LGTM! Consistent architectural refactoring.The conversion to inherit from
DataComponentrecord maintains consistency with the broader architectural changes across data components.
7-9: LGTM! Properties correctly override base class.The
TypeandIdentifierproperties are properly implemented with override modifiers.
19-19: LGTM! Collection type updated to array.The change from
List<int>?toint[]?aligns with the standardization on arrays for collections in data components.
28-60: LGTM! Serialization methods properly updated.The
ReadandWritemethods correctly override the base class methods and handle the array-basedAllowedEntitiesproperty appropriately. The serialization logic remains functionally equivalent.Obsidian.API/Inventory/DataComponents/FireworksDataComponent.cs (4)
4-4: LGTM! Consistent record-based inheritance pattern.The conversion to inherit from
DataComponentfollows the same architectural pattern applied consistently across all data components in this refactoring.
6-8: LGTM! Base class properties correctly overridden.The
TypeandIdentifierproperties properly use the override modifier to implement the base class declarations.
12-12: LGTM! Array standardization applied consistently.The change from
List<FireworkExplosion>toFireworkExplosion[]maintains consistency with the broader effort to use arrays for collections in data components.
17-26: LGTM! Serialization methods updated correctly.The
ReadandWritemethods properly override the base class methods and correctly handle the array-basedExplosionsproperty. The serialization logic is maintained while adapting to the new architecture.Obsidian.API/Inventory/DataComponents/UseCooldownDataComponent.cs (2)
4-4: LGTM! Clean transition from interface to abstract base class.The refactoring from
IDataComponentinterface toDataComponentabstract record aligns well with the broader modernization effort across the inventory system.
17-27: Property and method overrides are correctly implemented.The
Type,Identifier,Read, andWritemembers are properly converted to overrides while preserving their original functionality.Obsidian.API/Inventory/ItemStack.cs (6)
8-8: Good transition from record to class for custom equality logic.Converting from
record classtoclasswith explicitIEquatable<ItemStack>implementation allows for proper custom equality logic that considers both theHolderand internal component storage.
27-33: Constructor parameter update aligns with refactoring.The change from
List<IDataComponent>toIEnumerable<DataComponent>is consistent with the broader migration to the new data component hierarchy and provides better flexibility for callers.
35-35: Well-designed copy constructor with proper null safety.The copy constructor provides a clean way to create ItemStack copies with different counts while properly copying all components. The
[DisallowNull]attribute enhances null safety.
70-85: Enhanced component initialization with replacement logic.The updated method correctly handles component replacement when the same type already exists, which is essential for properly overriding default components during initialization.
89-92: Equality implementation is logically correct.The equality check properly compares both the
Holderand the sequence of internal components. The TODO comment on line 67 suggests there may be further refinements needed for DataComponent equality checking.
68-68: Clean implementations of GetHashCode and ToString.Both overrides are appropriately implemented -
GetHashCodecombines the relevant state, andToStringprovides a meaningful string representation using the unlocalized name.Also applies to: 87-87
Obsidian.API/Inventory/DataComponents/SimpleDataComponent.cs (2)
3-26: Excellent refactoring of the generic SimpleDataComponent.The conversion to inherit from
DataComponentis well-implemented with:
- Proper delegate storage as private readonly fields
- Null validation in the constructor
- Correct property overrides
- Clean separation of concerns
53-63: Clean simplification of the non-generic SimpleDataComponent.The refactoring removes unnecessary empty method implementations while properly maintaining the required property overrides through constructor initialization.
Obsidian.API/Inventory/DataComponents/DataComponentsStorage.cs (1)
2-45: Comprehensive and consistent refactoring of storage implementation.All type references have been properly updated from
IDataComponenttoDataComponent:
- Internal storage dictionaries updated
- Method signatures and return types converted
- Generic constraints properly adjusted
- Enumerator interface updated
The changes provide a solid foundation for the new data component hierarchy.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Obsidian/Events/MainEventHandler.Inventory.cs (1)
75-76: Safety check needed for recipe result access.Using
First()without checking if the collection is empty could throw an exception. This was flagged in a previous review.Consider using this approach:
- var result = recipe.Result.First(); + if (!recipe.Result.Any()) + { + logger.LogWarning("Recipe {recipe} has no results", recipe.Identifier); + return; + } + var result = recipe.Result.First();
🧹 Nitpick comments (1)
Obsidian/Events/MainEventHandler.Inventory.cs (1)
15-45: Consider adding error handling for crafting operations.The async conversion looks good, but consider wrapping the crafting call in a try-catch block to handle potential exceptions during recipe lookup and packet sending.
- await this.HandleCraftingAsync(args); + try + { + await this.HandleCraftingAsync(args); + } + catch (Exception ex) + { + logger.LogError(ex, "Error handling crafting for player {Player}", args.Player.Username); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Obsidian.API/Inventory/ItemStack.cs(4 hunks)Obsidian.API/_Interfaces/IPlayer.cs(3 hunks)Obsidian/Entities/Player.cs(6 hunks)Obsidian/Events/MainEventHandler.Inventory.cs(5 hunks)Obsidian/Net/Packets/Play/Serverbound/ContainerClickPacket.cs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Obsidian.API/_Interfaces/IPlayer.cs
- Obsidian/Net/Packets/Play/Serverbound/ContainerClickPacket.cs
- Obsidian/Entities/Player.cs
- Obsidian.API/Inventory/ItemStack.cs
🔇 Additional comments (4)
Obsidian/Events/MainEventHandler.Inventory.cs (4)
1-18: LGTM! Proper async conversion and logging support.The conversion to async handling and addition of logging imports are well-justified by the new crafting functionality that requires asynchronous packet sending.
47-85: Overall crafting logic is sound, but address the safety issue.The crafting handler implementation is well-structured with proper null recipe handling and logging. The async packet sending is appropriate for client communication.
234-275: LGTM! Improved consistency with carried item handling.The conversion to use
player.CarriedItemconsistently improves state management. The added logging will be valuable for debugging inventory interactions, and the logic flow correctly handles both item placement and pickup scenarios.
306-312: LGTM! Good use of enum for magic numbers.Replacing magic numbers with named enum values significantly improves code readability and maintainability. The values likely correspond to specific mouse button states in the Minecraft protocol.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Obsidian/Events/MainEventHandler.Inventory.cs (1)
76-77: Still usingFirst()without emptiness check
(see earlier review on previous commit)Calling
recipe.Result.First()will throw if a malformed recipe has no result.
Add anAny()guard or useFirstOrDefault()with a null check to avoid crashing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Obsidian.API/Containers/BaseContainer.cs(2 hunks)Obsidian.API/Events/ContainerClickEventArgs.cs(2 hunks)Obsidian.API/_Interfaces/IPlayer.cs(2 hunks)Obsidian/Entities/Player.cs(6 hunks)Obsidian/Events/MainEventHandler.Inventory.cs(5 hunks)Obsidian/Net/Packets/Play/Serverbound/ContainerClickPacket.cs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Obsidian.API/Events/ContainerClickEventArgs.cs
- Obsidian/Net/Packets/Play/Serverbound/ContainerClickPacket.cs
- Obsidian.API/_Interfaces/IPlayer.cs
- Obsidian/Entities/Player.cs
🔇 Additional comments (2)
Obsidian.API/Containers/BaseContainer.cs (1)
120-122:GetDifferencemay overflow / mis-map slots
clickedSlot - this.Size + 9is cast back toshort.
IfclickedSlot≥ 32 768 or the subtraction result ≥ 32 768, the cast wraps and returns a negative value, producing wrong indices and client desyncs. Please keep the arithmetic inintand validate the range before casting, or returnintfor safety.Obsidian/Events/MainEventHandler.Inventory.cs (1)
60-68: Verify network vs. internal slot mappingInternally you clear slot
9, but the packet you send reportsSlot = 0.
If the server/world slot indices don’t match the client’s, the UI can desync (ghost items). Double-check the crafting-table mapping and ensure both actions target the same logical slot.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Obsidian.API/Containers/CraftingTable.cs (1)
12-12: Hardcoded titles; consider localization.If your UI supports localization/components, prefer localized resources over hardcoded strings for "Crafting Table"/"Crafter".
I can refactor to use your project’s i18n mechanism if you point me to the Title/translation utilities.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Obsidian.API/Containers/CraftingTable.cs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
Obsidian.API/Containers/CraftingTable.cs (2)
Obsidian.API/Containers/BaseContainer.cs (3)
SetResult(138-138)ItemStack(64-64)ItemStack(140-140)Obsidian.API/Inventory/ItemStack.cs (1)
ItemStack(8-105)
🔇 Additional comments (1)
Obsidian.API/Containers/CraftingTable.cs (1)
7-7: Verify size/layout for Crafter vs CraftingTable (size = 10).Using a fixed size of 10 works for a 3x3 + 1 result layout. For the Crafter block (1.21.4), confirm this matches your UI/packet mapping (does Crafter expose a result slot, or is the result ghost-only?). If layouts differ, consider parameterizing the result slot index or size by
type.Would you like a follow-up patch to parameterize
ResultIndex/size perInventoryType?
| if(type != InventoryType.Crafting || type != InventoryType.Crafter) | ||
| throw new ArgumentException("CraftingTable must be of type InventoryType.Crafting or InventoryType.Crafter", nameof(type)); |
There was a problem hiding this comment.
Constructor guard always throws; use AND instead of OR.
if (type != InventoryType.Crafting || type != InventoryType.Crafter) is always true for any value of type. Replace || with &&.
- if(type != InventoryType.Crafting || type != InventoryType.Crafter)
+ if (type != InventoryType.Crafting && type != InventoryType.Crafter)
throw new ArgumentException("CraftingTable must be of type InventoryType.Crafting or InventoryType.Crafter", nameof(type));Note: Ensure using System; is available in the compilation unit or fully-qualify System.ArgumentException if implicit usings are disabled.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if(type != InventoryType.Crafting || type != InventoryType.Crafter) | |
| throw new ArgumentException("CraftingTable must be of type InventoryType.Crafting or InventoryType.Crafter", nameof(type)); | |
| if (type != InventoryType.Crafting && type != InventoryType.Crafter) | |
| throw new ArgumentException("CraftingTable must be of type InventoryType.Crafting or InventoryType.Crafter", nameof(type)); |
🤖 Prompt for AI Agents
In Obsidian.API/Containers/CraftingTable.cs around lines 9 to 10, the
constructor guard condition uses || (OR) which causes it to always throw an
exception regardless of the type value. Change the condition to use && (AND) so
it only throws if type is neither InventoryType.Crafting nor
InventoryType.Crafter. Also verify that System.ArgumentException is properly
referenced by including 'using System;' or fully qualifying the exception.
|
|
||
| public override ItemStack? GetResult() => throw new NotImplementedException(); | ||
| public override void SetResult(ItemStack? result) => throw new NotImplementedException(); | ||
| public override ItemStack? GetResult() => throw new NotImplementedException(); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
GetResult not implemented; crafting result cannot propagate.
Leaving this as NotImplementedException will break the server-side crafting flow. Implement result-slot access.
Assuming the result occupies a dedicated slot (see note below), here’s a minimal fix:
- public override ItemStack? GetResult() => throw new NotImplementedException();
+ public override ItemStack? GetResult() => this.items[ResultIndex];Additionally, update SetResult accordingly (outside this hunk):
// Add near the top of the class:
private const int ResultIndex = 0; // or 9 depending on your layout
// Replace SetResult body:
public override void SetResult(ItemStack? result) => this.items[ResultIndex] = result;Please confirm which index your container uses for the result:
- Common: result at 0, inputs at 1..9
- Alternative: inputs at 0..8, result at 9
I can adjust the patch once you confirm.
🏁 Script executed:
#!/bin/bash
# Print the top of the class to see field declarations and constructor
sed -n '1,200p' Obsidian.API/Containers/CraftingTable.cs
# Find where the items array is initialized to determine its length
rg -n "new ItemStack" -- Obsidian.API/Containers/CraftingTable.cs
# Locate any direct references to 'items' to confirm slot indexing
rg -n "items" -- Obsidian.API/Containers/CraftingTable.csLength of output: 880
🏁 Script executed:
#!/bin/bash
# Locate the ResultContainer class file
rg -l "class ResultContainer" -n .
# Print its declaration and constructor to see how the items array and result slot are defined
rg -n "class ResultContainer" -A50 Obsidian.API/Containers/ResultContainer.cs
# Search within ResultContainer for any constants or comments mentioning the result slot index
rg -n "Result" -n "Index" -A5 Obsidian.API/Containers/ResultContainer.csLength of output: 442
🏁 Script executed:
#!/bin/bash
# Print the first 200 lines of BaseContainer to inspect items array and any slot mapping
sed -n '1,200p' Obsidian.API/Containers/BaseContainer.cs
# Look for where 'items' is initialized to confirm indexing logic
rg -n "items\s*=" -A3 Obsidian.API/Containers/BaseContainer.csLength of output: 3673
Implement GetResult and SetResult in CraftingTable
The container is initialized with size 10 (base(10,…)), so slots 0–9 are available. By convention the result slot is index 0 and inputs occupy 1–9. Replace the NotImplementedException stubs with direct access to slot 0.
File: Obsidian.API/Containers/CraftingTable.cs
public sealed class CraftingTable : ResultContainer
{
+ // Slot 0 is the crafting result; slots 1–9 are the 3×3 grid
+ private const int ResultIndex = 0;
public CraftingTable(InventoryType type = InventoryType.Crafting) : base(10, type)
@@
- public override void SetResult(ItemStack? result) => throw new NotImplementedException();
- public override ItemStack? GetResult() => throw new NotImplementedException();
+ public override void SetResult(ItemStack? result)
+ => SetItem(ResultIndex, result);
+
+ public override ItemStack? GetResult()
+ => GetItem(ResultIndex);
}🤖 Prompt for AI Agents
In Obsidian.API/Containers/CraftingTable.cs at line 16, replace the
NotImplementedException in GetResult with code that returns the item in slot 0,
which is the result slot. Similarly, implement SetResult to set the item in slot
0 directly. This aligns with the container's size of 10 where slot 0 is the
result and slots 1 to 9 are inputs.
1 participant
Implemented a recipe lookup system for Shaped and Shapeless crafting and also fixed tags source generation as they were returning the wrong registry id for items. Did both of these together due to it being required for me to get item crafting working. I will work on furnaces and other result based tables. This should work for the Crafter block out the box
I do have some quirks to fix but will fix them tomorrow
Closes #470
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor
Chores