Query Sellable Tiles - #273
Conversation
WalkthroughThe changes involve modifications across several classes to enhance entity handling and querying capabilities in the Maple2 game framework. The Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
Maple2.Model/Metadata/FieldEntity/FieldEntity.cs (1)
88-95: LGTM: NewFieldSalableTilerecord added.The addition of the
FieldSalableTilerecord aligns well with the PR objectives for managing salable tiles. The inheritance fromFieldEntityand the constructor pattern maintain consistency with other entity types.Consider adding a brief XML comment to explain the purpose and usage of the
SalableGroupproperty. For example:/// <summary> /// Gets or sets the group identifier for the salable tile. /// Tiles with the same SalableGroup are considered part of the same salable unit. /// </summary> public int SalableGroup { get; init; }Maple2.File.Ingest/Mapper/MapDataMapper.cs (1)
148-161: Implementation for salable tiles looks good, with some suggestions for improvement.The new code segment correctly handles
IMS2CubePropentities and createsFieldSalableTileinstances as required. However, consider the following suggestions:
- The bounding box size is currently hardcoded. Consider parameterizing this or deriving it from the
cubeproperties for more flexibility.- Verify that
placeable.Positionis the correct position for theFieldSalableTile. Depending on your game's coordinate system, you might need to adjust this (e.g., centering the tile or aligning it with the grid).Here's a potential refactor to address these points:
if (entity is IMS2CubeProp cube && cube.CubeSalableGroup != 0) { - entityBounds.Min = -new Vector3(75, 75, 0); - entityBounds.Max = new Vector3(75, 75, 150); + Vector3 tileSize = GetTileSizeFromCube(cube); // New method to determine tile size + entityBounds.Min = -new Vector3(tileSize.X / 2, tileSize.Y / 2, 0); + entityBounds.Max = new Vector3(tileSize.X / 2, tileSize.Y / 2, tileSize.Z); + Vector3 adjustedPosition = AdjustTilePosition(placeable.Position, tileSize); // New method to adjust position fieldEntity = new FieldSalableTile( Id: entityId, - Position: placeable.Position, + Position: adjustedPosition, Rotation: placeable.Rotation, Scale: placeable.Scale, Bounds: entityBounds, SalableGroup: cube.CubeSalableGroup ); break; }This refactor introduces two new methods:
GetTileSizeFromCubeandAdjustTilePosition. You'll need to implement these based on your game's specific requirements.Maple2.Server.Game/Commands/DebugCommand.cs (3)
287-289: Clarify argument descriptions to reflect actual search rangeThe descriptions for the
x,y, andzarguments state "How far along the axis to search from the player," but in the query, these values are multiplied by 2, effectively doubling the search range. Consider updating the argument descriptions to accurately reflect the actual search dimensions.
299-305: Provide user feedback when map data is unavailableCurrently, if the map metadata or map data is not found, the command returns silently without informing the user. Adding error messages will improve user experience by providing feedback when the required data is unavailable.
Apply this diff to add error messages:
if (!session.Field.MapMetadata.TryGet(session.Field.MapId, out MapMetadata? map)) { + ctx.Console.Error.WriteLine("Map metadata not found."); return; } if (!mapDataStorage.TryGet(map.XBlock, out FieldAccelerationStructure? mapData)) { + ctx.Console.Error.WriteLine("Map data not found in storage."); return; }
314-317: Use consistent variable naming for clarityConsider renaming the variable
toCelltocellfor consistency with other parts of the code and to enhance readability.Apply this diff to rename the variable:
- Vector3S toCell = FieldAccelerationStructure.PointToCell(position); ... - ctx.Console.Out.WriteLine($"SalableGroupId {salableTile.SalableGroup} found at {position.X} {position.Y} {position.Z} in cell {toCell.X} {toCell.Y} {toCell.Z}"); + Vector3S cell = FieldAccelerationStructure.PointToCell(position); ... + ctx.Console.Out.WriteLine($"SalableGroupId {salableTile.SalableGroup} found at {position.X} {position.Y} {position.Z} in cell {cell.X} {cell.Y} {cell.Z}");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- Maple2.File.Ingest/Mapper/MapDataMapper.cs (2 hunks)
- Maple2.Model/Game/Field/FieldAccelerationStructure.cs (7 hunks)
- Maple2.Model/Metadata/FieldEntity/FieldEntity.cs (3 hunks)
- Maple2.Server.Game/Commands/DebugCommand.cs (2 hunks)
🔇 Additional comments (10)
Maple2.Model/Metadata/FieldEntity/FieldEntity.cs (2)
1-2: LGTM: Minor change inusingstatement order.The reordering of the
usingstatements is a minor stylistic change that doesn't affect functionality. While it may align with certain coding standards, it has minimal impact on the code.
13-14: LGTM: NewSalableTileenum value added.The addition of
SalableTileto theFieldEntityTypeenum aligns with the PR objectives for managing salable tiles. The placement afterCellmaintains a logical order.To ensure this change doesn't break existing code, please run the following script to check for potential impacts on switch statements or other enum usages:
✅ Verification successful
Verified: The
SalableTileenum value is appropriately handled in existing switch statements withinFieldAccelerationStructure.cs, ensuring no conflicts or issues are introduced.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential impacts of the new enum value on existing code. # Test: Search for switch statements or direct enum value comparisons rg --type csharp -e "switch.*FieldEntityType" -e "case.*FieldEntityType" -e "FieldEntityType\.\w+"Length of output: 1679
Maple2.File.Ingest/Mapper/MapDataMapper.cs (1)
Line range hint
1-379: Overall, the changes align well with the PR objectives.The implementation of salable tile handling is a good addition to the
ParseMapEntitiesmethod. It integrates smoothly with the existing code structure and doesn't introduce any apparent issues to the overall functionality of theMapDataMapperclass.To further improve the code:
- Consider adding unit tests for the new
IMS2CubeProphandling logic to ensure its correctness and prevent regressions.- Update the class or method documentation to reflect the new capability of handling salable tiles.
Maple2.Server.Game/Commands/DebugCommand.cs (1)
153-153: New command added successfullyThe
DebugQuerySalableTileCommandhas been correctly added toDebugQueryCommand.Maple2.Model/Game/Field/FieldAccelerationStructure.cs (6)
8-9: Necessary namespaces imported forFieldSalableTileThe addition of
using Maple2.Model.Metadata;andusing Maple2.Model.Metadata.FieldEntity;is appropriate to support the newFieldSalableTileentity.
21-21: Consistent use of trailing commas in enum definitionsAdding a trailing comma after the last enumeration member in
FieldEntityMembersimproves code maintainability and simplifies future additions.
180-182: Enhanced readability with pattern matching inQueryFluidsUsing property pattern matching
{ IsSurface: true, IsShallow: false }in the type check forFieldFluidEntityenhances the readability and conciseness of the code.
695-696:FieldSalableTilecorrectly associated withFieldEntityType.SalableTileThis addition ensures that
FieldSalableTileentities are properly serialized with the correct entity type in theWriteTomethod.
768-770: Proper serialization ofSalableGroupforFieldSalableTileThe
SalableGroupproperty is correctly written in theWriteTomethod, ensuring thatFieldSalableTileentities include necessary data during serialization.
946-953: Correct deserialization ofFieldSalableTileentitiesIn the
ReadEntitymethod,FieldSalableTileentities are properly deserialized, including theSalableGroupproperty, ensuring consistency with the serialization logic.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- Maple2.File.Ingest/Mapper/MapDataMapper.cs (2 hunks)
- Maple2.Model/Game/Field/FieldAccelerationStructure.cs (7 hunks)
- Maple2.Model/Metadata/FieldEntity/FieldEntity.cs (3 hunks)
- Maple2.Server.Game/Commands/DebugCommand.cs (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Maple2.File.Ingest/Mapper/MapDataMapper.cs
- Maple2.Model/Game/Field/FieldAccelerationStructure.cs
- Maple2.Model/Metadata/FieldEntity/FieldEntity.cs
🔇 Additional comments (2)
Maple2.Server.Game/Commands/DebugCommand.cs (2)
153-153: LGTM: New command added correctlyThe
DebugQuerySellableTileCommandis correctly added to theDebugQueryCommandclass, following the existing pattern for other query commands.
279-320: LGTM: Implementation consistent with other query commandsThe
DebugQuerySellableTileCommandis well-implemented and follows the structure of other query commands. It correctly uses themapDataStorageto query sellable tiles and outputs the results in a consistent format.
This pull request introduces new functionality for handling salable tiles in the game field and includes several code improvements and refactorings. The most important changes are the addition of a new
FieldSalableTileentity, updates to theFieldAccelerationStructureclass to support this new entity, and the addition of a debug command for querying salable tiles.New Functionality:
FieldSalableTileentity to represent salable tiles in the game field. (Maple2.Model/Metadata/FieldEntity/FieldEntity.cs)FieldAccelerationStructureclass to handleFieldSalableTileentities, including methods to query and write these entities. (Maple2.Model/Game/Field/FieldAccelerationStructure.cs) [1] [2] [3]Code Improvements:
DebugQuerySalableTileCommandto query salable tiles in the game field. (Maple2.Server.Game/Commands/DebugCommand.cs) [1] [2]Refactorings:
FieldEntityTypeenum to includeSalableTile. (Maple2.Model/Metadata/FieldEntity/FieldEntity.cs)FieldSalableTileinParseMapEntitiesmethod. (Maple2.File.Ingest/Mapper/MapDataMapper.cs)Summary by CodeRabbit
Release Notes
New Features
FieldSellableTileentities, enhancing entity parsing and querying capabilities.Bug Fixes
Documentation