fix(Core/Player): properly update m_usedTalentCount

[FingerKnoten]

fix-isse-20231

Changes Proposed:

This PR proposes changes to:

• Core (units, players, creatures, game systems).
• Scripts (bosses, spell scripts, creature scripts).
• Database (SAI, creatures, etc).

Issues Addressed:

• Closes Player::addTalent does not properly update m_usedTalentCount #20231 (comment)

SOURCE:

The changes have been validated through:

• Live research (checked on live servers, e.g Classic WotLK, Retail, etc.)
• Sniffs (remember to share them with the open source community!)
• Video evidence, knowledge databases or other public sources (e.g forums, Wowhead, etc.)
• The changes promoted by this pull request come partially or entirely from another project (cherry-pick). Cherry-picks must be committed using the proper --author tag in order to be accepted, thus crediting the original authors, unless otherwise unable to be found

Tests Performed:

This PR has been:

• Tested in-game by the author.
• Tested in-game by other community members/someone else other than the author/has been live on production servers.
• This pull request requires further testing and may have edge cases to be tested.

How to Test the Changes:

• This pull request can be tested by following the reproduction steps provided in the linked issue
• This pull request requires further testing. Provide steps to test your changes. If it requires any specific setup e.g multiple players please specify it as well.

Known Issues and TODO List:

• [ ]
• [ ]

How to Test AzerothCore PRs

When a PR is ready to be tested, it will be marked as [WAITING TO BE TESTED].

You can help by testing PRs and writing your feedback here on the PR's page on GitHub. Follow the instructions here:

http://www.azerothcore.org/wiki/How-to-test-a-PR

REMEMBER: when testing a PR that changes something generic (i.e. a part of code that handles more than one specific thing), the tester should not only check that the PR does its job (e.g. fixing spell XXX) but especially check that the PR does not cause any regression (i.e. introducing new bugs).

For example: if a PR fixes spell X by changing a part of code that handles spells X, Y, and Z, we should not only test X, but we should test Y and Z as well.

[TheSCREWEDSoftware]

fix(Core/Player/m_talents)?

Issue: #20231

Issue:

Since Player::addTalent does not properly update m_usedTalentCount this causes a wrong state of the player talent representation. This will be fixed with a relogin so only rare cases where this becomes an issue.
E.g. if talents are added using Player::addTalent and then, without relogging, someone tries to reset the talents. This will not work.

Expected Blizzlike Behaviour

Talents that have been added with addTalent should be able to reset without relogging.are added using Player::addTalent and then, without relogging, someone tries to reset the talents. This will not work.

Steps to reproduce the problem

Add talents using Player::addTalent
Try to reset talents (Do not relog in between step 1 and 2)

No AC.Rev

Fix?

This will give the real-time feedback on the changes instead of needing to relog?

---

I will see if i get some time tomorrow to test this.

[Grimdhex]

Please use AC standard title: fix(Core/Player): properly update m_usedTalentCount

[sogladev]

Tested and tried with steps in #20231 (comment)

1. RankID.size() is always 5 https://github.com/FingerKnoten/azerothcore-wotlk/blob/ca33cd715bd86a762021d12fc646b394d59c2aaf/src/server/shared/DataStores/DBCStructure.h#L1927
   GetTalentSpellCost can be used but we can use talentPos->rank + 1 instead as talentPos is already nullchecked https://github.com/FingerKnoten/azerothcore-wotlk/blob/ca33cd715bd86a762021d12fc646b394d59c2aaf/src/server/game/DataStores/DBCStores.cpp#L689

2. m_usedTalentCount is incremented in 2 other places, addTalent is called in the same context so these instances can be removed.

I got something working with this patch

diff --git a/src/server/game/Entities/Player/Player.cpp b/src/server/game/Entities/Player/Player.cpp
index 229c51d1f7..738d0c0b55 100644
--- a/src/server/game/Entities/Player/Player.cpp
+++ b/src/server/game/Entities/Player/Player.cpp
@@ -2930,3 +2930,6 @@ bool Player::addTalent(uint32 spellId, uint8 addSpecMask, uint8 oldTalentRank)
         m_talents[spellId] = newTalent;
-        m_usedTalentCount += talentInfo->RankID.size() - oldTalentRank;
+        if (GetActiveSpecMask() & addSpecMask)
+        {
+            m_usedTalentCount += (talentPos->rank + 1) - oldTalentRank;
+        }
         return true;
@@ -2940,2 +2943,7 @@ bool Player::addTalent(uint32 spellId, uint8 addSpecMask, uint8 oldTalentRank)
 
+        if (GetActiveSpecMask() & addSpecMask)
+        {
+            m_usedTalentCount += (talentPos->rank + 1) - oldTalentRank;
+        }
+
         return true;
@@ -14068,5 +14076,2 @@ void Player::LearnTalent(uint32 talentId, uint32 talentRank, bool command /*= fa
 
-    // xinef: update free talent points count
-    m_usedTalentCount += talentPointsChange;
-
     if (!command)
@@ -15021,6 +15026,2 @@ void Player::_LoadTalents(PreparedQueryResult result)
             ASSERT(talentPos);
-
-            // xinef: increase used talent points count
-            if (GetActiveSpecMask() & specMask)
-                m_usedTalentCount += talentPos->rank + 1;
         } while (result->NextRow());

[Nyeriah]

Tested and tried with steps in #20231 (comment)

1. RankID.size() is always 5 https://github.com/FingerKnoten/azerothcore-wotlk/blob/ca33cd715bd86a762021d12fc646b394d59c2aaf/src/server/shared/DataStores/DBCStructure.h#L1927
   GetTalentSpellCost can be used but we can use talentPos->rank + 1 instead as talentPos is already nullchecked https://github.com/FingerKnoten/azerothcore-wotlk/blob/ca33cd715bd86a762021d12fc646b394d59c2aaf/src/server/game/DataStores/DBCStores.cpp#L689
2. m_usedTalentCount is incremented in 2 other places, addTalent is called in the same context so these instances can be removed.

I got something working with this patch

diff --git a/src/server/game/Entities/Player/Player.cpp b/src/server/game/Entities/Player/Player.cpp
index 229c51d1f7..738d0c0b55 100644
--- a/src/server/game/Entities/Player/Player.cpp
+++ b/src/server/game/Entities/Player/Player.cpp
@@ -2930,3 +2930,6 @@ bool Player::addTalent(uint32 spellId, uint8 addSpecMask, uint8 oldTalentRank)
         m_talents[spellId] = newTalent;
-        m_usedTalentCount += talentInfo->RankID.size() - oldTalentRank;
+        if (GetActiveSpecMask() & addSpecMask)
+        {
+            m_usedTalentCount += (talentPos->rank + 1) - oldTalentRank;
+        }
         return true;
@@ -2940,2 +2943,7 @@ bool Player::addTalent(uint32 spellId, uint8 addSpecMask, uint8 oldTalentRank)
 
+        if (GetActiveSpecMask() & addSpecMask)
+        {
+            m_usedTalentCount += (talentPos->rank + 1) - oldTalentRank;
+        }
+
         return true;
@@ -14068,5 +14076,2 @@ void Player::LearnTalent(uint32 talentId, uint32 talentRank, bool command /*= fa
 
-    // xinef: update free talent points count
-    m_usedTalentCount += talentPointsChange;
-
     if (!command)
@@ -15021,6 +15026,2 @@ void Player::_LoadTalents(PreparedQueryResult result)
             ASSERT(talentPos);
-
-            // xinef: increase used talent points count
-            if (GetActiveSpecMask() & specMask)
-                m_usedTalentCount += talentPos->rank + 1;
         } while (result->NextRow());

So this PR doesnt work?

[sogladev]

So this PR doesnt work?

it doesn't fix the issue

[FingerKnoten]

Thanks for testing and finding a working solution @sogladev .
I applied your patch and tested myself. Seems to work without any issues now.

[sogladev]

Tested and tried with steps in #20231 (comment) . I can successfully add talents and then reset talents ✅

codestyle fail Trailing whitespace found: /home/runner/work/azerothcore-wotlk/azerothcore-wotlk/src/server/game/Entities/Player/Player.cpp at line 2931

[sudlud]

Can you please address the codestyle issue so CI checks will pass