Skip to content

fix(PlayerMethods): corrected SetSkill argument order#391

Merged
iThorgrim merged 1 commit into
azerothcore:masterfrom
Aldori15:set-skill-params
Jul 15, 2026
Merged

fix(PlayerMethods): corrected SetSkill argument order#391
iThorgrim merged 1 commit into
azerothcore:masterfrom
Aldori15:set-skill-params

Conversation

@Aldori15

@Aldori15 Aldori15 commented Jul 8, 2026

Copy link
Copy Markdown

Summary

Fixes Player:SetSkill argument forwarding in ALE.

AzerothCore core expects:

// Player.h
void SetSkill(uint16 id, uint16 step, uint16 currVal, uint16 maxVal);

// Player.cpp
void Player::SetSkill(uint16 id, uint16 step, uint16 newVal, uint16 maxVal)

ALE was reading the Lua arguments in that order, but forwarding them incorrectly here:

uint16 step = ALE::CHECKVAL<uint16>(L, 3);
uint16 currVal = ALE::CHECKVAL<uint16>(L, 4);
uint16 maxVal = ALE::CHECKVAL<uint16>(L, 5);

player->SetSkill(id, currVal, maxVal, step);

And according to the documentation, it matches the expectation from the Core. So this caused Lua calls like this to pass the values into the core as the wrong fields:

player:SetSkill(skillId, step, currentVal, maxVal)
image

Fix

  • Forward the arguments in the correct order as expected by AzerothCore

Summary by CodeRabbit

  • Bug Fixes
    • Corrected skill updates so the current value, maximum value, and step are now applied in the proper order.
    • This helps ensure skill changes made through Lua behave as expected in-game.

AzerothCore core expects:
// Player.h
void SetSkill(uint16 id, uint16 step, uint16 currVal, uint16 maxVal);
// Player.cpp
void Player::SetSkill(uint16 id, uint16 step, uint16 newVal, uint16 maxVal)

But ALE was doing (incorrect):
player->SetSkill(id, currVal, maxVal, step);
@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The SetSkill Lua binding in PlayerMethods.h now calls Player::SetSkill with arguments reordered to (id, step, currVal, maxVal) instead of (id, currVal, maxVal, step).

Changes

SetSkill Argument Order Fix

Layer / File(s) Summary
Reorder SetSkill call arguments
src/LuaEngine/methods/PlayerMethods.h
The player->SetSkill(...) call now passes arguments as (id, step, currVal, maxVal) instead of (id, currVal, maxVal, step).

Estimated code review effort: 1 (Trivial) | ~3 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: correcting SetSkill argument order in PlayerMethods.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

Comment @coderabbitai help to get the list of available commands.

@iThorgrim iThorgrim self-assigned this Jul 9, 2026
@iThorgrim
iThorgrim self-requested a review July 15, 2026 22:50
@iThorgrim
iThorgrim merged commit 9e5b8c6 into azerothcore:master Jul 15, 2026
6 checks passed
@Aldori15
Aldori15 deleted the set-skill-params branch July 15, 2026 23:02
Aldori15 added a commit to Aldori15/mod-ale that referenced this pull request Jul 15, 2026
fix(PlayerMethods): corrected SetSkill argument order (azerothcore#391)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants