fix(ChangeAttributes): Fix wrong bonus value gen by Value Re-roller

[zbian99]

…f two-handed weapons

Changes

Maple2.Server.Game\PacketHandlers\ChangeAttributesScrollHandler.cs

• Modified the handling logic for the in-game item "Bonus Value Re-roller" (e.g., ID 31001985) to align with the structure of "Bonus Re-roller"
• Implemented calls to the new processing interface UpdateFixedOption (as counterpart to UpdateRandomOption)

Maple2.Server.Game\Util\ItemStatsCalculator.cs

• Created the UpdateFixedOption function following a similar structure to UpdateRandomOption
• The updated Option directly uses the Basic and Special values from the original Option, along with the multiplyFactor from the metadata's RandomTable

Reason for Changes

Bug Symptom: When using the "Bonus Value Re-roller" item (e.g., ID 31001985), two-handed weapons would receive abnormal values that were lower than the minimum expected range.

Root Cause: The original logic failed to transfer the multiplyFactor between old and new Options, causing two-handed weapons to use the default multiplyFactor = 1 and generate new values within the same random range as one-handed weapons.

Change 1:

Maple2.Server.Game\PacketHandlers\ChangeAttributesScrollHandler.cs

Standardized the abstraction level by unifying attribute modification logic into the ItemStatsCalculator.cs

Change 2:

Maple2.Server.Game\Util\ItemStatsCalculator.cs

Correctly implemented the transfer of multiplyFactor between Options

Summary by CodeRabbit

• New Features

  • Attribute Change Scrolls now apply fixed-attribute updates for both locked and unlocked stats, yielding more predictable reroll results.
  • Locked basic and special attributes are preserved during rerolls.

• Bug Fixes

  • Unified, clearer error responses on reroll failures.
  • Removes reliance on missing option data that could cause rare failures, improving stability and consistency.

[coderabbitai]

Walkthrough

Replaces metadata-driven attribute randomization with a fixed-option update flow: ChangeAttributesScrollHandler now calls ItemStatsCalculator.UpdateFixedOption (optionally with LockOption presets) and signals failures via ChangeAttributesPacket.Error; item option metadata lookups were removed.

Changes

Cohort / File(s)	Summary
ChangeAttributes handler Maple2.Server.Game/PacketHandlers/ChangeAttributesScrollHandler.cs	Removed ItemOptionRandomTable lookup and metadata pre-validation. Replaced randomization branches with calls to ItemStatsCalculator.UpdateFixedOption, passing LockOption presets when a lock item exists. Standardized error responses to ChangeAttributesPacket.Error(...).
ItemStatsCalculator addition Maple2.Server.Game/Util/ItemStatsCalculator.cs	Added public bool UpdateFixedOption(ref Item item, params LockOption[] presets) which builds a fixed option using ItemOption.MultiplyFactor, randomizes values into that fixed option, preserves locked Basic/Special attributes, and writes back to item.Stats.Random. Includes early-exit validation.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant C as Client
  participant H as ChangeAttributesScrollHandler
  participant ISC as ItemStatsCalculator
  participant P as ChangeAttributesPacket

  C->>H: Request change attributes (roll/fixed, optional lock item)
  H->>ISC: UpdateFixedOption(ref item, [LockOption...])
  alt ISC returns true
    ISC-->>H: success
    H->>P: Success (send updated item)
    P-->>C: OK
  else ISC returns false
    ISC-->>H: failure
    H->>P: Error(s_itemremake_error_server_default)
    P-->>C: Error
  end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• Fix: Item Stats #381 — Modifies ItemStatsCalculator randomization/get-value logic that UpdateFixedOption depends on.
• Offense Stat Line Limitations #306 — Changes option selection/validation signatures used by item option construction and may affect UpdateFixedOption.
• Differentiate Random Stats by Random Make Type #302 — Refactors item option randomization and handlers; closely related to the reroll/fixed-option flow changes.

Suggested reviewers

• Zintixx
• AngeloTadeucci

Pre-merge checks (2 passed, 1 warning)

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title clearly identifies the main change — fixing incorrect bonus values produced by the Value Re-roller — and references the affected subsystem (ChangeAttributes), which aligns with the PR objectives to transfer multiplyFactor and restore correct value ranges; it is concise and focused. The wording is directly related to the changeset, though the abbreviation "gen" is informal and the word "Fix" appears both in the conventional prefix and the message body, which are minor stylistic issues.

Poem

I nibble code and spare no hop,
Locked bits stay, the rest I mop.
Fixed options set, no metadata chase,
A tidy reroll — snug in place. 🐇✨

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

• Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
• Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a420bba and 45f19f5.

📒 Files selected for processing (2)

• Maple2.Server.Game/PacketHandlers/ChangeAttributesScrollHandler.cs (1 hunks)
• Maple2.Server.Game/Util/ItemStatsCalculator.cs (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

• Maple2.Server.Game/Util/ItemStatsCalculator.cs
• Maple2.Server.Game/PacketHandlers/ChangeAttributesScrollHandler.cs

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: build
• GitHub Check: build

✨ Finishing touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

Maple2.Server.Game/PacketHandlers/ChangeAttributesScrollHandler.cs (1)

119-132: Replace incorrect packet type in ChangeAttributesScrollHandler error paths
ChangeAttributesScrollHandler.cs (lines 120, 125, 131, 140, 145): currently calls ChangeAttributesPacket.Error(ChangeAttributesError…), but should call ChangeAttributesScrollPacket.Error(ChangeAttributesScrollError…) to match this handler’s other error paths.

🧹 Nitpick comments (1)

Maple2.Server.Game/Util/ItemStatsCalculator.cs (1)

187-187: Fix dotnet-format whitespace error.

Add a space before the opening brace.

-public bool UpdateFixedOption(ref Item item, params LockOption[] presets){
+public bool UpdateFixedOption(ref Item item, params LockOption[] presets) {

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e50f0c8 and a420bba.

📒 Files selected for processing (2)

• Maple2.Server.Game/PacketHandlers/ChangeAttributesScrollHandler.cs (1 hunks)
• Maple2.Server.Game/Util/ItemStatsCalculator.cs (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

Maple2.Server.Game/Util/ItemStatsCalculator.cs (4)

Maple2.Model/Game/Item/Item.cs (4)

• Item (9-269)
• Item (50-90)
• Item (92-117)
• Item (119-144)

Maple2.Model/Game/Item/ItemOption.cs (4)

• LockOption (35-38)
• LockOption (40-43)
• TryGet (45-54)
• TryGet (56-65)

Maple2.Model/Game/Item/ItemStats.cs (6)

• Option (98-124)
• Option (106-110)
• ItemStats (9-125)
• ItemStats (28-33)
• ItemStats (35-43)
• ItemStats (45-53)

Maple2.Tools/Extensions/EnumerableExtensions.cs (1)

• TryGetValue (74-82)

Maple2.Server.Game/PacketHandlers/ChangeAttributesScrollHandler.cs (3)

Maple2.Server.Game/Util/ItemStatsCalculator.cs (1)

• UpdateFixedOption (187-223)

Maple2.Model/Game/Item/ItemOption.cs (2)

• LockOption (35-38)
• LockOption (40-43)

Maple2.Server.Game/Packets/ChangeAttributesPacket.cs (1)

• ChangeAttributesPacket (10-43)

🪛 GitHub Actions: Format

Maple2.Server.Game/Util/ItemStatsCalculator.cs

[error] 187-187: dotnet format whitespace check failed for step: dotnet format whitespace --verify-no-changes --exclude 'Maple2.Server.World/Migrations/*.cs'. Issue found in Maple2.Server.Game/Util/ItemStatsCalculator.cs:187: WHITESPACE: Fix whitespace formatting. Insert '\s'.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: build
• GitHub Check: build