[3.3.5] Core/Pet: Fix pet action bar loosing autocast settings (#16211)#20110
Conversation
reverts one line of 6c21ddd
| } | ||
|
|
||
| // if remove last rank or non-ranked then update action bar at server and client if need | ||
| if (m_charmInfo->RemoveSpellFromActionBar(spell_id) && !learn_prev && clear_ab) |
There was a problem hiding this comment.
You are changing the sequencing of RemoveSpellFromActionBar w.r.t. learn_prev. Is this intentional?
There was a problem hiding this comment.
I changed it back to what is was. intentionally. order matters in short-circuit boolean evaluation and executing RemoveSpellFromActionBar() in any case broke the pet action bar autocast settings.
The case where it breaks is, when the pet has learned more than one Rank of an auto-castable spell like Growl or Bite.
There was a problem hiding this comment.
order matters in short-circuit boolean evaluation
That's the point. You're changing the order of the RemoveSpellFromActionBar invocation w.r.t. two different bools (clear_ab and learn_prev) - which of the two is the intended change (or is it both).
There was a problem hiding this comment.
Upon looking at diff history - both is intended.
There was a problem hiding this comment.
I want the function call to only happen when both clear_ab == true and !learn_pre == true (learn_prev == false) or rather I want the order to be restored to what it was before commit 6c21ddd. My patch simply reverts one line of this commit and thereby fixes an issue obviously caused by it, i.e it's 100% reproducable/fixed without/with my patch.
I would be happier myself, if I could give a better explanation, but I just started a few days ago to familiarize myself with the TrinityCore source, actually with the intention to fix this (and maybe some other) issue(s). .
|
@necropola btw to avoid cluttering the history of every PR you make by basing them all off the same branch, I'd recommend creating a separate branch based on 3.3.5 for every PR you make (f.ex., |
|
Will do. I still need to learn a lot. but I'm getting there. I actually checked your TC fork yesterday and noticed all those 3.3.5-xyz branches. :-) |
|
No worries, you'll get there. PS: Consider getting on IRC. |
|
My second patch/commit is now in a separate 3.3.5-branch of my fork. |
|
No, sadly. Just keep the PR as-is and start using separate branches going forward. |
3 participants
reverts one line of 6c21ddd
Target branch: 3.3.5
Issues addressed: Closes #16211
Tests performed: build succeeded and tested/verified in game (me and Eronox)