CPP: Fix Classic Inscription skill progression and skill-up notifications

[CDBrodie]

Changes Proposed:

This PR fixes Classic Inscription skill progression and restores the missing native skill-up chat notification.

Testing showed that Inscription is learned correctly from trainers and displays correctly in the Professions window using the parent profession skill:

SKILL_INSCRIPTION = 773

However, some Classic Inscription recipes reference the expansion-specific child skill in their DB2 SkillupSkillLineID:

SKILL_INSCRIPTION_2 = 2514

The player does not normally store Classic Inscription progression on skill 2514. Because Player::UpdateCraftSkill() used SkillupSkillLineID directly, these recipes attempted to increase a skill the character did not possess and therefore awarded no profession skill.

For example, Classic Inscription recipe spell 52843 reported:

SkillLine            = 773
SkillupSkillLineID   = 2514
TrivialSkillLineLow  = 45
TrivialSkillLineHigh = 75
NumSkillUps          = 1

The fix resolves SKILL_INSCRIPTION_2 to SKILL_INSCRIPTION before checking and increasing the player's crafting skill.

This preserves the correct BFA profession representation while allowing Classic Inscription recipes to increase the player's actual Inscription skill.

This PR also restores the missing client skill-up notification from Player::UpdateSkillPro().

Previously, UpdateSkillPro() successfully changed the player's skill rank but did not send a corresponding skill chat message. Profession skill therefore increased internally and in the profession window without displaying the normal blue skill-up notification.

After a successful skill increase, the core now sends the message using the native CHAT_MSG_SKILL chat type and the localized skill name.

Example:

Your skill in Inscription has increased to 7.

Milling was also tested during investigation. The existing Milling implementation already progresses SKILL_INSCRIPTION correctly and therefore does not require modification.

This PR proposes changes to:

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

Issues Addressed:

• Classic Inscription recipes referencing SKILL_INSCRIPTION_2 do not increase the player's actual Inscription skill.
• Player::UpdateCraftSkill() attempts to progress skill 2514 even though the player normally stores Inscription progression on skill 773.
• Successful skill gains through Player::UpdateSkillPro() do not display the normal client skill-up message.
• Manually adding or synchronizing skill 2514 is not required and can interfere with the profession displayed by the BFA client.

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

The behavior was validated directly against the current BFA-HavenCore implementation and its DB2 skill-line data during in-game testing.

Observed Classic Inscription recipe data before resolving the skill line:

spell=52843
abilitySkillLine=773
skillupSkillLineID=2514
low=45
high=75
numSkillUps=1

After resolving the recipe skill line to the player's stored Inscription skill:

rawSkill=2514
resolvedSkill=773
current=3
max=75
chance=1000
gained=1
newValue=4

Milling was also verified independently to increase the stored Inscription skill:

773: 1 -> 2

No 2514 skill entry was required on the player.

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.

Testing was performed on Windows using a locally compiled RelWithDebInfo build.

The following scenarios were tested:

• Learned Inscription normally from a profession trainer.
• Confirmed Inscription appears correctly in the Professions window.
• Confirmed Milling is available normally.
• Confirmed the character stores Inscription using skill 773.
• Crafted Classic Inscription recipes.
• Confirmed recipes referencing SkillupSkillLineID = 2514 now increase skill 773.
• Confirmed skill 2514 does not need to exist in character_skills.
• Milled low-level herbs.
• Confirmed Milling increases Inscription normally.
• Confirmed successful crafting skill increases persist after save/logout.
• Confirmed successful Milling skill increases persist after save/logout.
• Confirmed successful skill increases display the native blue CHAT_MSG_SKILL notification.
• Confirmed the skill name displayed in chat is obtained from the localized SkillLineEntry.

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.

1. Create or use a character that does not currently know Inscription.

2. Learn Inscription normally from a profession trainer.

3. Open the Professions window and confirm Inscription appears normally and Milling is available.

4. If desired, save and log out and verify the stored skill:

SELECT guid, skill, value, max
FROM character_skills
WHERE guid = <character_guid>
  AND skill IN (773, 2514);

The expected profession skill is 773.

Skill 2514 should not need to be manually created on the character.

5. Learn or use a Classic Inscription recipe that awards profession skill.

Recipe spell 52843 was used during testing.

6. Craft the recipe.

7. Confirm that the Inscription profession skill increases.

8. Confirm the client displays a blue skill-up message similar to:

Your skill in Inscription has increased to 4.

9. Mill a low-level herb that can grant Inscription skill.

10. Confirm Milling also increases the same Inscription profession skill.

11. Confirm the Milling skill increase also displays the normal blue skill-up message.

12. Save and log out, then verify the persisted value:

SELECT guid, skill, value, max
FROM character_skills
WHERE guid = <character_guid>
  AND skill IN (773, 2514);

The expected result is progression on skill 773 without requiring a 2514 skill entry.

13. Because Player::UpdateSkillPro() is a generic skill progression function, regression testing should also be performed on other skills that use this path.

Recommended professions to verify include:

• Mining
• Herbalism
• Skinning
• Jewelcrafting
• Fishing

Confirm that normal skill progression still works and that successful skill increases display the native CHAT_MSG_SKILL notification.

Known Issues and TODO List:

• Some individual profession recipes have SkillupSkillLineID = 0 in DB2 data. These recipes cannot award profession skill through UpdateCraftSkill() and are separate data issues from the Inscription progression problem addressed by this PR.
• Additional profession regression testing is recommended because UpdateSkillPro() is a generic skill progression path.

How to Test BFA-HavenCore 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.

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.

[CDBrodie]

Closing until I fix a little error I found.

[Hextv]

Closing until I fix a little error I found.

Alright

[CDBrodie]

This has been Superseded by PR #272 . 271 (This PR) Can be ignored/deleted