Offense Stat Line Limitations - #306
Conversation
WalkthroughThis pull request updates the offense attribute handling in the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant ISC as ItemStatsCalculator
Caller->>ISC: GetStaticOption(item, job, statsType, pick)
ISC-->>Caller: Return Option or Failure
sequenceDiagram
participant Caller as Caller
participant ISC as ItemStatsCalculator
participant IV as IsValidStat
Caller->>ISC: GetRandomOption(itemOption, itemType, statsType, count, presets)
ISC->>IV: Validate offense stat count
IV-->>ISC: Validity result
ISC-->>Caller: Return random 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
Maple2.Server.Game/Util/ItemStatsCalculator.cs (1)
472-503: Prevent potential infinite loop when no valid stat can be added.
Because the code continues looping until statResult.Count + specialResult.Count == total, but skips offense stats when IsValidStat fails, there's a risk of never exiting if all candidates are invalid.Consider adding a maximum iteration safeguard. For example:
while (statResult.Count + specialResult.Count < total) { + const int maxAttempts = 1000; + int attempts = 0; + + // ... + while (statResult.Count + specialResult.Count < total && attempts < maxAttempts) { + attempts++; ItemOption.Entry entry = option.Entries.Random(); if (statsType == ItemStats.Type.Random && itemType.IsArmor && !IsValidStat(total, statResult, specialResult, entry)) { continue; } // ... + } + if (attempts >= maxAttempts) { + Log.Warning("Reached maximum iteration attempts while filling item stats."); + break; + } }This ensures the code eventually exits, even if no valid offense attributes remain.
🧹 Nitpick comments (2)
Maple2.Server.Game/Util/ItemStatsCalculator.cs (2)
365-373: Consider renaming or refactoring for clarity.
Invoking “RandomItemOption” from “GetStaticOption” could be confusing for future maintainers, as “RandomItemOption” is also used for truly random scenarios. A more neutral helper name (e.g., “GenerateItemOption”) could reduce confusion.
394-396: Simplify or remove the pass-through function.
GetRandomOption immediately delegates to RandomItemOption, adding a minor layer of indirection. If no extra logic is planned, consider removing it in favor of calling RandomItemOption directly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Maple2.Server.Game/Util/ItemStatsCalculator.cs(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (4)
Maple2.Server.Game/Util/ItemStatsCalculator.cs (4)
88-90: Looks good.
Using the new parameter (ItemStats.Type.Static) on GetStaticOption appears consistent with the method signature update.
94-94: No issues found.
Passing ItemStats.Type.Random here matches the intended usage for random options.
139-139: No issues found.
The additional parameters for GetRandomOption appear consistent with the rest of the changes.
544-559: Confirm intended inequality for the offense line threshold.
The code uses “< OFFENSE_LINE_MAX_THRESHOLD,” allowing fewer than 50% offense stats but rejecting exactly 50%. This blocks one offense line on a 2-line item and two offense lines on a 4-line item, etc.Is it your intention to disallow exactly 50% offense lines? If not, consider using “≤” to permit up to half the lines as offense lines.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Maple2.Server.Game/Util/ItemStatsCalculator.cs (1)
556-583: Optimize the validation logic with early returns.The method can be simplified for better readability:
private static bool IsValidStat(ItemType itemType, int statLineCount, IDictionary<BasicAttribute, BasicOption> statDict, IDictionary<SpecialAttribute, SpecialOption> specialDict, ItemOption.Entry entry) { - if (itemType.IsAccessory) { - int currentElementalStats = specialDict.Keys.Count(stat => elementalDamageAttributes.Contains(stat)); - if (entry.SpecialAttribute != null && elementalDamageAttributes.Contains((SpecialAttribute) entry.SpecialAttribute)) { - currentElementalStats++; - } - return currentElementalStats < 2; - } + // Early return for non-armor/accessory items + if (!itemType.IsArmor && !itemType.IsAccessory) { + return true; + } - if (itemType.IsArmor) { - int offenseStatCount = statDict.Keys.Count(stat => offenseBasicAttributes.Contains(stat)); - offenseStatCount += specialDict.Keys.Count(stat => offenseSpecialAttributes.Contains(stat)); + // Handle accessories + if (itemType.IsAccessory) { + int currentElementalStats = specialDict.Keys.Count(stat => elementalDamageAttributes.Contains(stat)); + return !(entry.SpecialAttribute != null && + elementalDamageAttributes.Contains((SpecialAttribute)entry.SpecialAttribute) && + currentElementalStats >= 1); + } - if (entry.BasicAttribute != null && offenseBasicAttributes.Contains((BasicAttribute) entry.BasicAttribute)) { - offenseStatCount++; - } else if (entry.SpecialAttribute != null && offenseSpecialAttributes.Contains((SpecialAttribute) entry.SpecialAttribute)) { - offenseStatCount++; - } + // Handle armor + int offenseStatCount = statDict.Keys.Count(stat => offenseBasicAttributes.Contains(stat)) + + specialDict.Keys.Count(stat => offenseSpecialAttributes.Contains(stat)); - return (float) offenseStatCount / statLineCount <= OFFENSE_LINE_MAX_THRESHOLD; + if (entry.BasicAttribute != null && offenseBasicAttributes.Contains((BasicAttribute)entry.BasicAttribute)) { + offenseStatCount++; + } else if (entry.SpecialAttribute != null && offenseSpecialAttributes.Contains((SpecialAttribute)entry.SpecialAttribute)) { + offenseStatCount++; } - return true; + return (float)offenseStatCount / statLineCount <= OFFENSE_LINE_MAX_THRESHOLD; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Maple2.Server.Game/Util/ItemStatsCalculator.cs(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
Maple2.Server.Game/Util/ItemStatsCalculator.cs (2)
23-86: Use valid C# initialization syntax for offense attributes.The current collection expression syntax (
[...]) requires C# 12. For better compatibility, use standard collection initializers.
377-409: LGTM! Method signature changes are well-structured.The addition of
itemTypeandstatsTypeparameters provides necessary context for offense line validation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Maple2.Server.Game/Util/ItemStatsCalculator.cs (2)
416-418: Consider reordering parameters for better readability.The parameter order could be improved by grouping related parameters together. Consider moving
countnext topresetssince they're both optional parameters.-private ItemStats.Option GetRandomOption(ItemOption itemOption, in ItemType itemType, ItemStats.Type statsType, int count = -1, params LockOption[] presets) +private ItemStats.Option GetRandomOption(ItemOption itemOption, in ItemType itemType, ItemStats.Type statsType, params LockOption[] presets, int count = -1)
565-590: Improve method documentation and readability.
- Add parameter documentation to explain the purpose of each parameter.
- Consider extracting the offense stat count calculation into a separate method for better readability.
/// <summary> /// Verifies if the new attribute being added meets the offense line threshold. /// </summary> + /// <param name="itemType">The type of item being validated</param> + /// <param name="statLineCount">Total number of stat lines</param> + /// <param name="statDict">Dictionary of basic attributes</param> + /// <param name="specialDict">Dictionary of special attributes</param> + /// <param name="entry">The new entry being validated</param> + /// <returns>True if the stat is valid according to offense line limitations</returns> private static bool IsValidStat(ItemType itemType, int statLineCount, IDictionary<BasicAttribute, BasicOption> statDict, IDictionary<SpecialAttribute, SpecialOption> specialDict, ItemOption.Entry entry) { int damageTypeStatCount = specialDict.Keys.Count(stat => damageTypeAttributes.Contains(stat)); if (entry.SpecialAttribute != null && damageTypeAttributes.Contains((SpecialAttribute) entry.SpecialAttribute)) { return damageTypeStatCount < 1; } if (!itemType.IsCombatPet && !itemType.IsWeapon && !itemType.IsAccessory) { - int offenseStatCount = statDict.Keys.Count(stat => offenseBasicAttributes.Contains(stat)); - offenseStatCount += specialDict.Keys.Count(stat => offenseSpecialAttributes.Contains(stat)); - - if (entry.BasicAttribute != null && offenseBasicAttributes.Contains((BasicAttribute) entry.BasicAttribute)) { - offenseStatCount++; - } else if (entry.SpecialAttribute != null && offenseSpecialAttributes.Contains((SpecialAttribute) entry.SpecialAttribute)) { - offenseStatCount++; - } + int offenseStatCount = CalculateOffenseStatCount(statDict, specialDict, entry); + return (float)offenseStatCount / statLineCount <= OFFENSE_LINE_MAX_THRESHOLD; } return true; } + private static int CalculateOffenseStatCount( + IDictionary<BasicAttribute, BasicOption> statDict, + IDictionary<SpecialAttribute, SpecialOption> specialDict, + ItemOption.Entry entry) + { + int count = statDict.Keys.Count(stat => offenseBasicAttributes.Contains(stat)); + count += specialDict.Keys.Count(stat => offenseSpecialAttributes.Contains(stat)); + + if (entry.BasicAttribute != null && offenseBasicAttributes.Contains((BasicAttribute)entry.BasicAttribute)) + { + count++; + } + else if (entry.SpecialAttribute != null && offenseSpecialAttributes.Contains((SpecialAttribute)entry.SpecialAttribute)) + { + count++; + } + + return count; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Maple2.Server.Game/Util/ItemStatsCalculator.cs(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
Maple2.Server.Game/Util/ItemStatsCalculator.cs (1)
23-95: Add XML documentation for attribute lists.Consider adding XML documentation to explain the purpose and usage of each list, especially the distinction between
offenseBasicAttributes,offenseSpecialAttributes, anddamageTypeAttributes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Maple2.Server.Game/Util/ItemStatsCalculator.cs (2)
22-22: Consider making the threshold configurable.The offense line threshold is currently hardcoded. Consider making it configurable through game settings or configuration files for easier tuning.
571-574: Document the damage type attribute limitation.Add a comment explaining why only one damage type attribute is allowed per item. This will help future maintainers understand the game design decision.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Maple2.Server.Game/Util/ItemStatsCalculator.cs(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (3)
Maple2.Server.Game/Util/ItemStatsCalculator.cs (3)
23-40: LGTM! Well-organized attribute categorization.The attributes are logically grouped into offense basic attributes, offense special attributes, and damage type attributes. The categorization appears comprehensive and accurate.
Also applies to: 41-78, 79-95
386-418: LGTM! Method signature changes are consistent.The addition of
ItemStats.TypeandItemTypeparameters enables proper validation of offense attributes. All callers have been updated accordingly.
565-590:⚠️ Potential issueFix the logical condition for item type validation.
The condition
itemType is { IsCombatPet: false, IsWeapon: false, IsAccessory: false }is incorrect. It will skip offense line validation for items that should have it applied.Apply this fix:
- if (itemType is { IsCombatPet: false, IsWeapon: false, IsAccessory: false }) { + if (!itemType.IsCombatPet && !itemType.IsWeapon && !itemType.IsAccessory) {Additionally, consider adding a comment explaining which item types should have offense line validation applied.
Likely invalid or redundant comment.
Needs testing/verification
Summary by CodeRabbit