-
Notifications
You must be signed in to change notification settings - Fork 237
Twink server trinity eluna #31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…anguage spells Co-authored-by: Kobiesan <[email protected]>
Co-authored-by: Kobiesan <[email protected]>
Fix language menu not updating immediately when learning language spells
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds support for a new "Linguistics" profession to the game and significantly modifies the creature assistance system in combat. The changes span multiple core game systems including skills, quests, items, and creature AI behavior.
Key Changes:
- Adds Linguistics as a new secondary profession with associated enums across skill, quest, and item systems
- Implements a periodic creature assistance check system that continuously calls for help during combat
- Modifies creature faction assistance logic to allow NPCs to help defend against neutral players who initiate combat
Reviewed changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/server/shared/SharedDefines.h | Adds Linguistics skill/quest definitions and new spell custom error messages for faction reputation requirements |
| src/server/game/Spells/SpellMgr.h | Updates IsProfessionSkill() to recognize Linguistics as a profession |
| src/server/game/Quests/QuestDef.h | Adds Linguistics to the quest trade skill enum |
| src/server/game/Entities/Item/ItemTemplate.h | Adds Linguistics book as a new recipe item subclass |
| src/server/game/Entities/Creature/Creature.h | Adds m_assistCheckTime member variable for periodic assistance checks |
| src/server/game/Entities/Creature/Creature.cpp | Implements periodic assistance system, modifies faction-based assistance logic, and initializes assistance timer |
| src/server/game/AI/ScriptedAI/ScriptedGossip.h | Adds Linguistics to the tradeskill enum for gossip system |
| src/common/Collision/Maps/MapDefines.h | Improves code formatting for static_assert readability |
| .gitignore | Adds bin/ directory to ignored files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| SKILL_LINGUISTICS = 789 | ||
| }; | ||
|
|
||
| #define MAX_SKILL_TYPE 789 |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The MAX_SKILL_TYPE constant is incorrectly set to 789, but it should be 790 since the highest skill ID (SKILL_LINGUISTICS) is now 789. The max constant should be one greater than the highest enum value to accommodate array sizing and iteration.
| #define MAX_SKILL_TYPE 789 | |
| #define MAX_SKILL_TYPE 790 |
| ITEM_SUBCLASS_LINGUISTICS_BOOK = 12 | ||
| }; | ||
|
|
||
| #define MAX_ITEM_SUBCLASS_RECIPE 12 |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The MAX_ITEM_SUBCLASS_RECIPE constant is incorrectly set to 12, but it should be 13 since the highest recipe subclass ID (ITEM_SUBCLASS_LINGUISTICS_BOOK) is now 12. The max constant should be one greater than the highest enum value to accommodate array sizing and iteration.
| #define MAX_ITEM_SUBCLASS_RECIPE 12 | |
| #define MAX_ITEM_SUBCLASS_RECIPE 13 |
| if (!IsPet() && !IsCharmed() && !GetCharmerOrOwnerGUID().IsPlayer() && !IsTotem() && !IsTrigger()) | ||
| { | ||
| CallAssistance(); | ||
| } |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CallAssistance() is being called here when a creature engages in combat. However, with the new periodic assistance check added in the Update() method (lines 807-833), creatures will continuously call for help throughout combat. This could result in duplicate or excessive assistance calls. Consider whether both the initial call here and the periodic calls in Update() are both necessary, or if one should be removed to avoid redundant behavior.
| if (!IsPet() && !IsCharmed() && !GetCharmerOrOwnerGUID().IsPlayer() && !IsTotem() && !IsTrigger()) | |
| { | |
| CallAssistance(); | |
| } | |
| // Assistance is now handled by the periodic check in Update(), so no immediate call here. |
|
|
||
| float radius = sWorld->getFloatConfig(CONFIG_CREATURE_FAMILY_ASSISTANCE_RADIUS); | ||
|
|
||
|
|
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Unnecessary extra blank line. There should only be one blank line here for consistency with the rest of the codebase.
| if (IsEngaged() && IsAlive() && !IsPet() && !IsCharmed() && !IsTotem() && !IsTrigger()) | ||
| { | ||
| // Only check for creatures that can actually call for help | ||
| if (!GetCharmerOrOwnerGUID().IsPlayer()) | ||
| { | ||
| if (diff >= m_assistCheckTime) | ||
| { | ||
| float radius = sWorld->getFloatConfig(CONFIG_CREATURE_FAMILY_ASSISTANCE_RADIUS); | ||
| if (radius > 0.0f) | ||
| { | ||
| // Use CallForHelp which directly engages nearby creatures | ||
| CallForHelp(radius); | ||
| } | ||
|
|
||
| // Set next assistance check time | ||
| m_assistCheckTime = sWorld->getIntConfig(CONFIG_CREATURE_FAMILY_ASSISTANCE_DELAY); | ||
| } | ||
| else | ||
| m_assistCheckTime -= diff; | ||
| } | ||
| } |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This periodic CallForHelp check in the Update loop could cause significant performance issues. The function is called every CONFIG_CREATURE_FAMILY_ASSISTANCE_DELAY milliseconds for every engaged creature, which performs radius-based grid searches. This is expensive when there are many creatures in combat. Consider whether this periodic assistance calling is necessary, given that CallAssistance() is already called in AtEngage() at line 3349.
| if (IsEngaged() && IsAlive() && !IsPet() && !IsCharmed() && !IsTotem() && !IsTrigger()) | |
| { | |
| // Only check for creatures that can actually call for help | |
| if (!GetCharmerOrOwnerGUID().IsPlayer()) | |
| { | |
| if (diff >= m_assistCheckTime) | |
| { | |
| float radius = sWorld->getFloatConfig(CONFIG_CREATURE_FAMILY_ASSISTANCE_RADIUS); | |
| if (radius > 0.0f) | |
| { | |
| // Use CallForHelp which directly engages nearby creatures | |
| CallForHelp(radius); | |
| } | |
| // Set next assistance check time | |
| m_assistCheckTime = sWorld->getIntConfig(CONFIG_CREATURE_FAMILY_ASSISTANCE_DELAY); | |
| } | |
| else | |
| m_assistCheckTime -= diff; | |
| } | |
| } | |
| // Removed periodic CallForHelp check for performance reasons. Assistance is now only called at engagement. |
| SPELL_CUSTOM_ERROR_CANT_MOUNT_WITH_SHAPESHIFT = 99, // You can't mount while affected by that shapeshift. | ||
| SPELL_CUSTOM_ERROR_INTRO_QUEST_ACTIVE = 100,// You cannot use that until you've completed the introduction quest. | ||
| SPELL_CUSTOM_ERROR_STORMWIND_NEUTRAL = 101,// You must be at least Neutral with Stormwind to use that. | ||
| SPELL_CUSTOM_ERROR_IRONFORGEORGNOME_NEUTRAL = 102,// You must be at least Neutral with Ironforge or Gnomeregan Exiles to use that. |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent naming: IRONFORGEORGNOME should have underscores between the words for better readability, like IRONFORGE_OR_GNOME to match the naming pattern used elsewhere in the codebase.
| SPELL_CUSTOM_ERROR_IRONFORGEORGNOME_NEUTRAL = 102,// You must be at least Neutral with Ironforge or Gnomeregan Exiles to use that. | |
| SPELL_CUSTOM_ERROR_IRONFORGE_OR_GNOME_NEUTRAL = 102,// You must be at least Neutral with Ironforge or Gnomeregan Exiles to use that. |
| SPELL_CUSTOM_ERROR_IRONFORGEORGNOME_NEUTRAL = 102,// You must be at least Neutral with Ironforge or Gnomeregan Exiles to use that. | ||
| SPELL_CUSTOM_ERROR_DARNASSUS_NEUTRAL = 103,// You must be at least Neutral with Darnassus to use that. | ||
| SPELL_CUSTOM_ERROR_EXODAR_NEUTRAL = 104,// You must be at least Neutral with Exodar to use that. | ||
| SPELL_CUSTOM_ERROR_ORGRIMMARORDARKSPEAR_NEUTRAL = 105,// You must be at least Neutral with Orgrimmar or Darkspear Trolls to use that. |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent naming: ORGRIMMARORDARKSPEAR should have underscores between the words for better readability, like ORGRIMMAR_OR_DARKSPEAR to match the naming pattern used elsewhere in the codebase.
| SPELL_CUSTOM_ERROR_ORGRIMMARORDARKSPEAR_NEUTRAL = 105,// You must be at least Neutral with Orgrimmar or Darkspear Trolls to use that. | |
| SPELL_CUSTOM_ERROR_ORGRIMMAR_OR_DARKSPEAR_NEUTRAL = 105,// You must be at least Neutral with Orgrimmar or Darkspear Trolls to use that. |
| } | ||
| } | ||
|
|
||
| // Initialize assistance check timer to trigger first check after 5 seconds |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment says "trigger first check after 5 seconds", but the actual delay is determined by CONFIG_CREATURE_FAMILY_ASSISTANCE_DELAY which may not be 5 seconds. The comment should either be generic (e.g., "Initialize assistance check timer based on config") or be updated to match the actual config value.
| // Initialize assistance check timer to trigger first check after 5 seconds | |
| // Initialize assistance check timer based on config value |
| if (!IsPet() && !IsCharmed() && !GetCharmerOrOwnerGUID().IsPlayer() && !IsTotem() && !IsTrigger()) | ||
| { | ||
| m_assistCheckTime = sWorld->getIntConfig(CONFIG_CREATURE_FAMILY_ASSISTANCE_DELAY); | ||
| } | ||
|
|
||
| // Call for assistance from nearby creatures | ||
| // Only do this for non-player controlled creatures in normal world content | ||
| if (!IsPet() && !IsCharmed() && !GetCharmerOrOwnerGUID().IsPlayer() && !IsTotem() && !IsTrigger()) | ||
| { |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicated conditional check: Lines 3340 and 3347 contain the exact same complex condition !IsPet() && !IsCharmed() && !GetCharmerOrOwnerGUID().IsPlayer() && !IsTotem() && !IsTrigger(). These two blocks should be combined into a single if statement to improve code maintainability and readability.
| if (!IsPet() && !IsCharmed() && !GetCharmerOrOwnerGUID().IsPlayer() && !IsTotem() && !IsTrigger()) | |
| { | |
| m_assistCheckTime = sWorld->getIntConfig(CONFIG_CREATURE_FAMILY_ASSISTANCE_DELAY); | |
| } | |
| // Call for assistance from nearby creatures | |
| // Only do this for non-player controlled creatures in normal world content | |
| if (!IsPet() && !IsCharmed() && !GetCharmerOrOwnerGUID().IsPlayer() && !IsTotem() && !IsTrigger()) | |
| { | |
| // Call for assistance from nearby creatures | |
| // Only do this for non-player controlled creatures in normal world content | |
| if (!IsPet() && !IsCharmed() && !GetCharmerOrOwnerGUID().IsPlayer() && !IsTotem() && !IsTrigger()) | |
| { | |
| m_assistCheckTime = sWorld->getIntConfig(CONFIG_CREATURE_FAMILY_ASSISTANCE_DELAY); |
| // Also check combat manager for any faction targets | ||
| if (!hasAttackedFaction) | ||
| { | ||
| std::vector<Unit*> combatTargets; |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable combatTargets is declared but never used. It's created as an empty vector and then the code directly iterates over enemy->GetCombatManager().GetPvECombatRefs() without populating or using this vector. This unused variable should be removed.
| std::vector<Unit*> combatTargets; |
Changes proposed:
Target branch(es): 335/6x
Issues addressed: Fixes #
Tests performed: (Does it build, tested in-game, etc)
Known issues and TODO list: