Differentiate Random Stats by Random Make Type #302
Conversation
WalkthroughThis pull request comprises updates across various components. The Maple2.File.Ingest project now references a newer parser version. In mapper files, explicit type casting and changes to collection types enable multiple item variations. Model components have been updated with a new Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant ISC as ItemStatsCalculator
participant GV as GetValue (Helper)
participant GIV as GetItemVariationValue (Helper)
C->>ISC: Call RandomizeValues(type, itemOption, itemMetadata, ref option, rollMax)
ISC->>GV: Compute value based on item metadata and random type
GV-->>ISC: Return computed value
ISC->>GIV: Calculate item variation details using computed value
GIV-->>ISC: Return updated variation option
ISC-->>C: Return modified ItemStats.Option
Possibly related PRs
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
Maple2.Model/Metadata/Table/ItemOptionTable.cs (2)
57-57: Clarify or rename the 'Variation' field.The newly added 'Variation' field can be confusing. Consider naming it something more descriptive, such as 'Delta', 'Offset', or 'RangeDelta', to enhance clarity regarding its usage and intent.
66-66: Consider a more descriptive name for 'Set'."Set" often implies a collection with distinct elements, which may cause confusion. A more descriptive name such as 'WeightedValue' or 'ValueWeightEntry' would better convey the purpose of holding a value with a weight.
Maple2.Model/Enum/ItemOptionMakeType.cs (1)
3-6: Add XML documentation for the enum members.Consider adding XML doc comments explaining when to use 'Base' vs. 'Range' to make the meaning of each value clear. This helps maintainers and future contributors, especially if the enum is expanded later.
Maple2.File.Ingest/Mapper/TableMapper.cs (1)
531-536: Consider adding weight configuration.The TODO comment about weight suggests that this feature is incomplete. Consider implementing the weight configuration or creating a follow-up issue.
Would you like me to help create an issue to track the implementation of the weight configuration?
Maple2.Server.Game/Util/ItemStatsCalculator.cs (3)
82-83: Check concurrency for multiple calls.These lines retrieve an ItemOption from a shared table and then call GetRandomOption and RandomizeValues. If multiple threads update item metadata or read from the same table simultaneously, confirm that TableMetadata.ItemOptionRandomTable is thread-safe. Also confirm item references are safely updated in a locked context if needed.
Also applies to: 86-86, 88-88
113-122: Expand doc comments for 'RandomizeValues'.Although the summary is present, consider documenting the different behaviors for RandomMakeType to enhance maintainability. An overview of how each parameter affects the final randomization logic would help future contributors.
320-356: Consider potential concurrency issues with Random.Shared.Random.Shared is threadsafe, but heavy concurrent usage can cause unexpected number distributions. If precise random distribution is required, consider a dedicated Random instance per thread or method.
Maple2.Server.Game/PacketHandlers/ChangeAttributesScrollHandler.cs (1)
105-105: Double-check “impossible” conditions.These lines indicate states that should never occur if data is consistent. Adding logs or asserts is helpful, but ensure the end-user receives a clear message if something truly impossible ever arises.
Also applies to: 109-110, 114-114, 120-120
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Maple2.File.Ingest/Maple2.File.Ingest.csproj(1 hunks)Maple2.File.Ingest/Mapper/ItemMapper.cs(1 hunks)Maple2.File.Ingest/Mapper/TableMapper.cs(4 hunks)Maple2.Model/Enum/ItemOptionMakeType.cs(1 hunks)Maple2.Model/Metadata/ItemMetadata.cs(1 hunks)Maple2.Model/Metadata/Table/ItemOptionTable.cs(1 hunks)Maple2.Server.Game/PacketHandlers/ChangeAttributesScrollHandler.cs(6 hunks)Maple2.Server.Game/Util/ItemStatsCalculator.cs(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- Maple2.File.Ingest/Maple2.File.Ingest.csproj
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (20)
Maple2.Model/Metadata/Table/ItemOptionTable.cs (2)
52-55: Ensure multi-range usage is handled downstream.Previously these dictionaries stored a single Range value per attribute; now they store arrays of Range values. Make sure that all consumers of these fields fully account for multiple entries. If downstream code was relying on a single range, it must now handle iterating through the array to avoid incorrect assumptions or index errors.
61-64: Verify handling of new 'Set[]' structures.These properties were updated from arrays of primitives (int[]/float[]) to arrays of 'Set' records. Confirm that any logic accessing or modifying these arrays now handles both 'Value' and 'Weight' correctly. Also, verify whether these weights are aggregated, normalized, or used elsewhere in ways that differ from simple numeric arrays.
Maple2.Model/Metadata/ItemMetadata.cs (1)
94-94: Validate external data for Enum usage.Changing the property type from int to RandomMakeType is a good improvement for type safety. Verify any external data or legacy code that sets this field to ensure valid enum values are always provided, preventing unexpected behavior from out-of-range assignments.
Maple2.File.Ingest/Mapper/ItemMapper.cs (1)
126-126: LGTM! Verify enum values match the data.The explicit casting to
RandomMakeTypeenum improves type safety. However, ensure that the enum values inRandomMakeType(BaseandRange) correctly correspond to the values indata.option.randomMakeType.Run the following script to verify the values:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify that the RandomMakeType enum values match the data. # Test: Search for randomMakeType values in the codebase rg -A 5 'randomMakeType' # Test: Search for the RandomMakeType enum definition ast-grep --pattern $'enum RandomMakeType { $$$ }'Length of output: 685
I'll run a new script to locate the definition of the RandomMakeType enum and verify its values.
🏁 Script executed:
#!/bin/bash # Search for the definition of the RandomMakeType enum in the repository rg "enum RandomMakeType" -n .Length of output: 104
🏁 Script executed:
#!/bin/bash # Display content of the file containing RandomMakeType enum definition cat Maple2.Model/Enum/ItemOptionMakeType.csLength of output: 207
Enum value mapping confirmed: The definition in Maple2.Model/Enum/ItemOptionMakeType.cs shows that RandomMakeType has Base = 0 and Range = 1—exactly as required. The explicit casting in Maple2.File.Ingest/Mapper/ItemMapper.cs ensures that a numeric value in data.option.randomMakeType is correctly interpreted as either Base or Range. No discrepancies were found.
Maple2.File.Ingest/Mapper/TableMapper.cs (6)
463-466: LGTM! Dictionary value types changed to support multiple variations.The change from single instances to lists enables storing multiple variations for each attribute, which aligns with the PR objective.
475-481: LGTM! Proper handling of multiple variations.The implementation correctly handles both new and existing attributes by:
- Checking if the attribute exists
- Appending to existing list or creating new list
- Using collection initializer syntax for better readability
483-487: LGTM! Consistent error handling for special attributes.The error handling for special attributes mirrors the basic attributes implementation, maintaining consistency in the codebase.
514-519: LGTM! Proper conversion to immutable arrays.The implementation correctly converts lists to arrays before returning, ensuring immutability of the returned collections.
524-527: LGTM! Consistent type changes in ParseItemEquipVariation.The changes to support multiple variations are consistently applied to the equip variation parsing.
548-551: Consider adding weight configuration.Similar TODO comment about weight in float entries. Consider implementing the weight configuration or creating a follow-up issue.
Maple2.Server.Game/Util/ItemStatsCalculator.cs (4)
9-9: No issue with the new import statement.The addition of "using Maple2.Tools" is presumably needed for classes or extensions introduced by that namespace. This looks fine.
40-42: Confirm handling when TryGetValue fails.When TableMetadata.ItemOptionRandomTable does not contain the key, the code simply skips setting Random stats. Consider logging or returning the partial stats object so the caller understands that no random option was applied.
129-135: Validate the usage of Random indices.The code repeatedly calls “Random.Shared.Next(2, 18)” but never uses the 'index' variable. If this is intentional, consider removing the unused calls or clarifying why the index is being generated.
Also applies to: 139-145
299-300: Method wrapper seems fine.Wrapping RandomItemOption behind GetRandomOption centralizes calls and clarifies the API.
Maple2.Server.Game/PacketHandlers/ChangeAttributesScrollHandler.cs (6)
12-12: Confirm that the extension methods are needed.The new using Maple2.Tools.Extensions suggests extension methods are now used. If they’re no longer used, you can remove this import for cleanliness.
60-60: Error enum usage ensures clearer code.Replacing direct references or magic constants with structured error codes (e.g., s_itemremake_scroll_error_invalid_scroll) is a good practice for maintainability. Make sure these error codes map to user-friendly messages on the client side.
Also applies to: 66-66, 70-70, 74-74
83-83: Lock attribute existence checks.Lines 83-97 check if a locked attribute exists. This logic looks correct. Consider unit tests for corner cases (e.g., zero locked attributes, invalid attribute in multi-lock scenario).
Also applies to: 85-85, 90-90, 97-97
138-138: Scroll consumption checks.The code calls Inventory.Consume for the scroll and lock item. If a concurrency or partial failure occurs between them, confirm whether a transaction or rollback logic is needed.
Also applies to: 142-142
154-154: Finalize item with new stats.When “Select” is confirmed, the code sets “item.Stats[ItemStats.Type.Random]” from “session.ChangeAttributesItem”. This final assignment is straightforward. Confirm that session.ChangeAttributesItem is always cleared or updated to avoid accidental reuse.
Also applies to: 161-161, 167-167
178-178: Scroll compatibility checks.These lines validate the target item’s rarity, type, level, and scroll conditions. Everything seems logically consistent. If future logic changes the valid item range, remember to update these checks.
Also applies to: 181-181, 184-184, 189-189, 192-192, 196-196, 199-199, 202-202
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Maple2.Server.Game/Util/ItemStatsCalculator.cs (2)
151-238: Consider extracting range logic into separate methods.The
GetValuemethod is quite long and handles multiple concerns. Consider extracting the range-based logic into separate methods for better maintainability.+ private int GetRangeValue(int levelFactor, ItemEquipVariationTable.Set<int>[]? tableValues, ItemEquipVariationTable.Set<float>[]? tableRates, bool rollMax) { + var (start, end, maxIndex) = GetRangeBounds(levelFactor); + if (rollMax) { + if (tableValues != null) return tableValues[maxIndex].Value; + if (tableRates != null) return (int)(tableRates[maxIndex].Value * 1000); + } + + var weightedValues = new WeightedSet<int>(); + var weightedRates = new WeightedSet<float>(); + + for (int i = start; i < end; i++) { + if (tableValues != null) { + weightedValues.Add(tableValues[i].Value, tableValues[i].Weight); + } else if (tableRates != null) { + weightedRates.Add(tableRates[i].Value, tableRates[i].Weight); + } + } + + return tableValues != null ? weightedValues.Get() : (int)(weightedRates.Get() * 1000); + } + + private static (int start, int end, int maxIndex) GetRangeBounds(int levelFactor) => + levelFactor switch { + < 50 => (0, 3, 2), + < 70 => (2, 10, 9), + _ => (10, 18, 17) + };
321-356: Add XML documentation for GetItemVariationValue method.The method lacks XML documentation explaining its purpose, parameters, and return value.
+ /// <summary> + /// Calculates the variation value for a given attribute based on its type and current value/rate. + /// </summary> + /// <param name="basicAttribute">The basic attribute to calculate variation for</param> + /// <param name="specialAttribute">The special attribute to calculate variation for</param> + /// <param name="value">The current value</param> + /// <param name="rate">The current rate</param> + /// <returns>The calculated variation value. If the input is a rate, the return value will be multiplied by 1000</returns> private int GetItemVariationValue(BasicAttribute? basicAttribute = null, SpecialAttribute? specialAttribute = null, int value = 0, float rate = 0)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Maple2.Server.Game/Util/ItemStatsCalculator.cs(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: format
🔇 Additional comments (2)
Maple2.Server.Game/Util/ItemStatsCalculator.cs (2)
39-44: LGTM! Improved error handling with TryGetValue pattern.The refactoring improves the code by using the safer
TryGetValuepattern and simplifying the random option retrieval logic.
299-301: LGTM! Simplified method signature.The method has been simplified by removing the output parameter and returning the value directly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Maple2.Server.Game/Util/ItemStatsCalculator.cs (1)
363-399: Extract magic number into a named constant.The conversion factor 1000 is used multiple times. Consider extracting it into a named constant for better maintainability.
+ private const int RATE_CONVERSION_FACTOR = 1000; private int GetItemVariationValue(BasicAttribute? basicAttribute = null, SpecialAttribute? specialAttribute = null, int value = 0, float rate = 0) { // ... - int convertedVariation = (int) range.Variation * 1000; - int convertedRate = (int) rate * 1000; + int convertedVariation = (int) range.Variation * RATE_CONVERSION_FACTOR; + int convertedRate = (int) rate * RATE_CONVERSION_FACTOR; // ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Maple2.Server.Game/PacketHandlers/ChangeAttributesScrollHandler.cs(6 hunks)Maple2.Server.Game/Util/ItemStatsCalculator.cs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Maple2.Server.Game/PacketHandlers/ChangeAttributesScrollHandler.cs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (5)
Maple2.Server.Game/Util/ItemStatsCalculator.cs (5)
39-44: LGTM! Improved null safety and method signature.The changes improve the code by using the null-safe TryGetValue pattern and removing the output parameter anti-pattern.
82-88: Add bounds checking for array access.While the null checking is improved, array access should include bounds checking to prevent potential IndexOutOfRangeException.
112-116: LGTM! Improved method documentation.The XML documentation clearly describes the parameters and their purpose.
342-344: LGTM! Simplified method signature.The method now has a cleaner signature without output parameters.
146-281: 🛠️ Refactor suggestionRefactor to use named constants for array indices.
The GetValue function uses multiple hardcoded indices (e.g., 4, 17, etc.) which could lead to maintenance issues and potential bugs.
Consider defining named constants for these indices:
+ private const int PET_LOW_RARITY_MAX_INDEX = 4; + private const int PET_HIGH_RARITY_MAX_INDEX = 17; + private const int LEVEL_FACTOR_LOW_MAX_INDEX = 2; + private const int LEVEL_FACTOR_MID_MAX_INDEX = 9; + private const int LEVEL_FACTOR_HIGH_MAX_INDEX = 17; int GetValue(Item item, ItemOption itemOptionMetadata, BasicAttribute? attribute = null, SpecialAttribute? specialAttribute = null, ItemEquipVariationTable.Set<int>[]? tableValues = null, ItemEquipVariationTable.Set<float>[]? tableRates = null, bool rollMax = false) { // ... - if (tableValues != null) { - return tableValues[4].Value; + if (tableValues != null && tableValues.Length > PET_LOW_RARITY_MAX_INDEX) { + return tableValues[PET_LOW_RARITY_MAX_INDEX].Value; } // ... apply similar changes to other indicesLikely invalid or redundant comment.
Summary by CodeRabbit
New Features
Refactor
Chores