Skip to content

Conversation

@Tereneckla
Copy link
Contributor

@Tereneckla Tereneckla commented Oct 21, 2025

Changes Proposed:

This PR proposes changes to:

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

https://trinitycore.info/en/database/335/world/spell_group
https://trinitycore.info/en/database/335/world/spell_group_stack_rules

About the removal of IsNeedModSpellDamagePercent, IsNeedModMeleeDamagePercent and IsNeedModHealPercent Hooks.
An alternative proposal is the removal of the DoneTotalMod parameter, but I don't see a use case in disabling specific SPELL_AURA_MOD_DAMAGE_PERCENT_DONE or SPELL_AURA_MOD_HEALING_DONE_PERCENT effects by script.

Originally they were added for modules from MaelstromCore here 2b3d46b
But I don't see usage in the mentioned modules 3v3-soloqueue and 1v1 arena.
I am unable to find the mentioned pvestats module or the usage of those hooks in other modules by @Winfidonarleyan

New handling of voidwalker glyph would be in line with wowhead comments.

This glyph does not have an immediate effect on your voidwalker if he's out when you apply it. If you resummon him, then you'll see the extra health.

Usage of the previous HandleStatModifier with a PCT value (now ApplyStatPctModifier) should be heavily discouraged as that will only be kept until the next time an AuraEffect modifying that stat is removed

Issues Addressed:

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.

Tereneckla and others added 16 commits October 20, 2025 17:50
Co-Authored-by: treeston <treeston.mmoc@gmail.com>
Co-Authored-by: Trisjdc <trisjdc@gmail.com>
Co-Authored-by: QAston <none@none>
Cherry Pick TrinityCore/TrinityCore/commit/8b0122e7ede6d22bb81869008d35b9338c5e408a
Co-Authored-by: ariel <ariel-@users.noreply.github.com>
@github-actions github-actions bot added DB related to the SQL database CORE Related to the core file-cpp Used to trigger the matrix build labels Oct 21, 2025
@github-actions github-actions bot added the Script Refers to C++ Scripts for the Core label Oct 21, 2025
@Tereneckla Tereneckla requested a review from sogladev October 21, 2025 19:53
@Tereneckla Tereneckla added Waiting to be Tested Ready to be Reviewed Requires WIKI Update Wiki sources will need to be updated after merging this PR. labels Oct 21, 2025
@Winfidonarleyan
Copy link
Member

Winfidonarleyan commented Oct 22, 2025

pvestats it was private, now azerothshard

@Tereneckla
Copy link
Contributor Author

pvestats it was private, now azerothshard

So the purpose of the hook is to dynamically modify the effect value of your timewalking aura so that spells are stronger or weaker dependent on the level of timewalking and the player.

Can I propose to instead do it like mod-zone-difficulty and modify the end result.
https://github.com/azerothcore/mod-zone-difficulty/blob/da48d07cfa98bee90353d454f032bbcdf9a1c103/src/mod_zone_difficulty_scripts.cpp#L261
It has the same outcome in making spells stronger or weaker, requires less calls on the hook and in my opinion is cleaner

Copy link
Contributor

Copilot AI left a 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 PR implements TrinityCore's spell group and spell group stack rules system to replace AzerothCore's existing custom implementation. The changes standardize spell stacking behavior (such as elixirs, blessings, buffs) using database-driven rules instead of hardcoded logic.

Key Changes:

  • Replaced custom spell stacking system with TrinityCore's spell_group and spell_group_stack_rules implementation
  • Removed obsolete unit script hooks (IsNeedModSpellDamagePercent, IsNeedModMeleeDamagePercent, IsNeedModHealPercent)
  • Refactored aura aggregation functions to use lambda predicates for better code reusability

Reviewed Changes

Copilot reviewed 25 out of 25 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
SpellMgr.h/cpp Core implementation of spell groups and stack rules system
SpellInfo.h/cpp Removed legacy spell stacking checks and integrated new system
SpellAuras.cpp Updated aura stacking logic to use new spell group rules
Unit.cpp/h Refactored aura modifier calculation functions with predicate support
spell_warlock.cpp Fixed voidwalker glyph to apply stats on summon instead of using TOTAL_PCT
spell_paladin.cpp Removed hardcoded Blessing of Sanctuary stat values
SpellInfoCorrections.cpp Removed Blessing of Sanctuary workaround
ScriptMgr.h/UnitScript.h/cpp Removed obsolete script hooks
SQL file Database schema and data updates for spell groups

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Tereneckla and others added 7 commits October 25, 2025 22:33
- Amount is now multiplied only on apply; on unapply, iterate through auras and reset the counter
- Fixes many cases of rounding error due to applying/unapplying of small factors
- Allows amounts to be zeroed (ie with an AuraEffect of amount -100)
- Reorder _ApplyWeaponDependentAuraMods

Cherry-Pick TrinityCore/TrinityCore@21d95a1
Cherry-Pick TrinityCore/TrinityCore@66ac150
Cherry-Pick TrinityCore/TrinityCore@c69a7d1
Cherry-Pick TrinityCore/TrinityCore@f93a23a
Cherry-Pick TrinityCore/TrinityCore@6dc37a9

Co-Authored-By: ariel- <ariel-@users.noreply.github.com>
Co-Authored-By: Shauren <shauren.trinity@gmail.com>
Tereneckla added a commit to Tereneckla/mod-ale that referenced this pull request Oct 28, 2025
Tereneckla added a commit to Tereneckla/mod-autobalance that referenced this pull request Oct 28, 2025
Tereneckla added a commit to Tereneckla/mod-solocraft that referenced this pull request Oct 28, 2025
Tereneckla added a commit to Tereneckla/mod-zone-difficulty that referenced this pull request Oct 28, 2025
@MacGuzzler
Copy link

Where is the status of this? Can we give it a bump?

@Tereneckla
Copy link
Contributor Author

waiting to be tested and reviewed
the labels are accurate

Copy link
Member

@sogladev sogladev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I applied SQL codestyle changes. There are some minor cpp codestyle changes I suggested.

I checked with TC335 and the changes look good overall, minus:

  • The value for the astromancer script may be wrong

This issue wasn’t mentioned, but I’ll test the DP + ToW stacking #3790. update: works ✅

blinkysc added a commit to blinkysc/azerothcore-wotlk that referenced this pull request Nov 28, 2025
…spell group stack rules

- Unit tests verify SpellGroupStackRule enum values
- Integration tests run on worldserver startup (enable with SpellGroupStackRulesTest.Enable = 1)
- Tests 11,503 same-group spell pairs across 50 groups
- Tests 100 cross-group pairs for DEFAULT return
- Verifies fixes for issues azerothcore#22619, azerothcore#19324
@blinkysc
Copy link
Contributor

@Nyeriah Nyeriah merged commit 2f7f9bd into azerothcore:master Nov 28, 2025
15 of 18 checks passed
Nyeriah pushed a commit to azerothcore/mod-autobalance that referenced this pull request Nov 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CORE Related to the core DB related to the SQL database file-cpp Used to trigger the matrix build Ready to be Reviewed Requires WIKI Update Wiki sources will need to be updated after merging this PR. Script Refers to C++ Scripts for the Core Waiting to be Tested

Projects

None yet

6 participants