Feat: Building on outdoor plots - #286
Conversation
WalkthroughThe pull request introduces several modifications across multiple classes in the Maple2 codebase. Key changes include enhancements to the Changes
Possibly related PRs
Suggested reviewers
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: 1
🧹 Outside diff range and nitpick comments (8)
Maple2.Server.Game/Manager/Field/FieldManager.cs (3)
56-56: LGTM: New AccelerationStructure property added.The addition of the
AccelerationStructureproperty with a public getter and private setter is appropriate. It maintains encapsulation while allowing external read access.Consider adding a brief XML documentation comment to explain the purpose and usage of this property.
106-110: LGTM: Initialization of AccelerationStructure added.The new code block appropriately initializes the
AccelerationStructureproperty and handles the case where it can't be loaded. The error logging is a good practice for tracking issues.Consider using string interpolation for the error log message to improve readability:
logger.Error("Failed to load acceleration structure for map {MapId}", MapId);
AccelerationStructureis initialized but not used withinFieldManager.cs. Consider removing it if it's unnecessary or ensure it's utilized appropriately within the class.🔗 Analysis chain
Line range hint
1-669: Verify usage of AccelerationStructure in the codebase.The
AccelerationStructureproperty is initialized but not used within this file. It's important to ensure that it's properly utilized in other parts of the codebase where field acceleration is needed.Please run the following script to check for usage of
AccelerationStructureacross the codebase:If there are no results, consider adding appropriate usage of
AccelerationStructurewhere field acceleration optimizations are needed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of AccelerationStructure across the codebase # Search for AccelerationStructure usage echo "Searching for AccelerationStructure usage:" rg "AccelerationStructure" --type csharp -g "!**/FieldManager.cs" # Check if FieldAccelerationStructure is used in other files echo "Checking for FieldAccelerationStructure usage:" rg "FieldAccelerationStructure" --type csharp -g "!**/FieldManager.cs"Length of output: 6648
Maple2.Model/Game/Field/FieldAccelerationStructure.cs (3)
199-210: Good implementation, but consider some improvements.The new
FirstSellableTilemethod is well-implemented and integrates nicely with the existingQueryCellsmethod. However, there are a few points to consider:
- The vertical search range (3000 units) is hardcoded. Consider making this a parameter or a configurable constant for better flexibility.
- The method stops at the first matching tile. While this is efficient, it might not always be the desired behavior. Consider adding an option to find the "best" match if needed.
- There's no null check on the predicate function. Adding one would prevent potential NullReferenceExceptions.
Here's a suggested improvement:
- public FieldSellableTile? FirstSellableTile(Vector3 position, Func<FieldSellableTile, bool> predicate) { - Vector3 size = new Vector3(0, 0, 3000); // Only search in same column, we are looking for ground tiles + public FieldSellableTile? FirstSellableTile(Vector3 position, Func<FieldSellableTile, bool> predicate, float verticalSearchRange = 3000f) { + if (predicate == null) throw new ArgumentNullException(nameof(predicate)); + Vector3 size = new Vector3(0, 0, verticalSearchRange); Vector3 min = position - 0.5f * size; Vector3 max = position + 0.5f * size; FieldSellableTile? result = null; QueryCells(min, max, entity => { if (result is not null) return; if (entity is not FieldSellableTile tile || !predicate(tile)) return; result = tile; }); return result; }
211-211: Consider reviewing similar query methods for consistency.The addition of the
FirstSellableTilemethod is well-placed within the class. However, it might be worthwhile to review other similar query methods (e.g.,QuerySellableTiles,QueryFluids, etc.) to ensure consistency in naming conventions, parameter usage, and error handling across all query methods.
Line range hint
61-1000: Consider refactoring for improved maintainability and separation of concerns.The
FieldAccelerationStructureclass is quite large and handles multiple responsibilities, including entity management, spatial queries, and serialization. This complexity might make the class difficult to maintain and test as the project grows.Consider the following suggestions:
- Split the class into smaller, more focused classes. For example, separate the entity management, spatial querying, and serialization into distinct classes.
- Use composition to combine these smaller classes into a facade that provides the current functionality.
- Consider introducing interfaces for the different types of queries to allow for easier extension and testing.
This refactoring could improve code maintainability, testability, and adherence to the Single Responsibility Principle.
Maple2.Server.Game/Manager/HousingManager.cs (2)
362-371: Consider adding unit tests for the new ground height logicThe introduction of the
groundHeightcalculation based onFieldSellableTileenhances the precision of cube placement. To ensure this new logic behaves correctly across different scenarios, consider adding unit tests that cover various plot types and positions.
374-374: Replace magic number with a named constant for clarityThe threshold value
0.1used in calculatingisOnGroundis a magic number. Defining a named constant would improve readability and maintainability.Apply this diff to implement the suggestion:
+ const float GROUND_THRESHOLD = 0.1f; - bool isOnGround = Math.Abs(position.Z - groundHeight) < 0.1; + bool isOnGround = Math.Abs(position.Z - groundHeight) < GROUND_THRESHOLD;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- Maple2.Model/Common/Vector.cs (1 hunks)
- Maple2.Model/Game/Field/FieldAccelerationStructure.cs (2 hunks)
- Maple2.Server.Game/Manager/Field/FieldManager.cs (3 hunks)
- Maple2.Server.Game/Manager/HousingManager.cs (3 hunks)
🧰 Additional context used
🔇 Additional comments (5)
Maple2.Model/Common/Vector.cs (3)
18-20: Improved robustness inConvertFromIntmethodThe changes to the
ConvertFromIntmethod enhance its robustness and correctness. Here's why these changes are beneficial:
- The use of
uncheckedallows for intentional overflow without throwing exceptions, which is appropriate when dealing with potential overflow insbyteconversion.- The bitwise AND operation with 0xFF explicitly masks the values to fit within the 8-bit signed range, ensuring that only the least significant 8 bits are considered.
- Performing the bitwise AND before casting to
sbyteguarantees that the value is properly masked before conversion.These improvements make the method more resilient to edge cases and prevent unintended behavior when dealing with values at the boundaries of the
sbyterange.
Line range hint
1-94: Summary of changes and overall impactThe changes in this file focus on improving the
ConvertFromIntmethod of theVector3Bstruct. These modifications enhance the robustness of the conversion process by properly handling potential overflow and ensuring correct masking of values.The rest of the file, including the
Vector3Sstruct, remains unchanged. This targeted fix aligns with the PR objective of addressing the "Vector3B ConvertFromInt function" issue.Overall, these changes represent a positive improvement to the codebase, increasing its reliability without introducing apparent regressions. However, as suggested earlier, it's important to verify the impact on existing callers and consider applying similar improvements to analogous methods if they exist elsewhere in the codebase.
18-20: Verify impact and maintain consistency across the codebaseWhile the changes to
ConvertFromIntare localized and improve robustness, it's important to consider their broader impact:
- Verify that all callers of
ConvertFromIntstill behave as expected, especially for edge cases.- Consider the slight performance impact of the additional bitwise operations, though it's likely negligible.
- Check for similar conversion methods elsewhere in the codebase that might benefit from this improved approach for consistency.
To help with this verification, you can run the following script:
This script will help identify usages of
ConvertFromIntand similar conversion methods that might benefit from the same robustness improvements.✅ Verification successful
Impact Verified and Codebase Consistent
The changes to
ConvertFromInthave been verified:
ConvertFromIntis only utilized withinFunctionCubeHandler.cs, and all usages function as expected.- The additional bitwise operations have a negligible performance impact.
- No other conversion methods require modifications, ensuring consistency across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of ConvertFromInt and similar conversion methods echo "Usages of ConvertFromInt:" rg --type csharp "ConvertFromInt\(" -A 2 -B 2 echo "\nSimilar conversion methods:" rg --type csharp "Convert.*FromInt" -A 5 -B 2Length of output: 2332
Maple2.Server.Game/Manager/Field/FieldManager.cs (1)
10-10: LGTM: New using directive added.The addition of
using Maple2.Model.Game.Field;is appropriate, likely to support the newFieldAccelerationStructuretype introduced in this file.Maple2.Server.Game/Manager/HousingManager.cs (1)
9-11: Added necessary using directives for new functionalityThe added
usingstatements forMaple2.Model.Game.FieldandMaple2.Model.Metadata.FieldEntityare appropriate and necessary for the updated logic involvingFieldSellableTileand related metadata classes.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor