Warrior fixes and more...

[AGandrup]

Changes proposed:

• Added two new checks in CombatManager::CanBeginCombat, one that checks if both units are allowed to engage in combat, and another one that checks if a player (game masters with .gm on) has FACTION_FRIENDLY set as their faction. The latter was done to fix an issue where players with .gm on would get disconnected when trying to melee attack NPCs of the opposite faction. Apparently checking with IsGameMaster() is not enough.
• Fixed an issue with Hide Helmet and Hide Cloak not working properly after relogging.
• Fixed an issue with Warrior Charge where first auto attack after Charge would be skipped.
• Additionally, implemented a fix for auto attacking being broken after using focus Charge macro.
• Fixed Warrior Bloodthirst so it now heals for 0.5% instead of 5%.
• Removed unnecessary code which caused Warrior Execute to never consume more than 20 rage. It now consumes a maximum of 30 rage as intended.
• Fixed an issue with Warrior Slam not consuming Battle Trance talent. In case that both Bloodsurge and Battle Trance is active, only Bloodsurge will be consumed.
• Fixed an issue with Warrior Victory Rush and Impending Victory where both could be active at the same time and Impending Victory always being prioritized, causing the Warrior to heal less. These two effects with now properly override each other which also solves the prioritizing issue.
• Fixed an issue with Warrior Heroic Fury not resetting the cooldown of Intercept.
• Fixed an issue with Warrior Pummel being unable to trigger Gag Order talent.

Issues addressed:

• Fixes Interface - Hiding Helmet and Cloak options do not save #94

Tests performed:

• It builds.
• Everything works as intended.

Known issues and TODO list:

• None.

[AGandrup]

Ehh, just realized I forgot to upload sql file, which includes a couple more fixes - will do that tomorrow at some point.

[Ovahlord]

Do not bloat this PR please. Do single PR for single fixes so they can be reviewed properly and merged one by one

[AGandrup]

Do not bloat this PR please. Do single PR for single fixes so they can be reviewed properly and merged one by one

Alright, I guess I'll delete this PR and split it up then. Will take a look at it tonight.

[Ovahlord]

I've ported / did my own versions of the spell related changed for the warrior. The core changes, such as the charge and macro part will need more work because first of all: I need an reliable way of reproducing the issues that were suposedly fixed.

[AGandrup]

I've ported / did my own versions of the spell related changed for the warrior.

Alright, thats great. The project as well as the C++ language is all very new to me, so I figured there were probably better ways to fix these things.

The core changes, such as the charge and macro part will need more work because first of all: I need an reliable way of reproducing the issues that were suposedly fixed.

I understand. Do you still want me to create new PR for the last two core changes?

The two new checks added in the CombatManager::CanBeginCombat doesn't really matter much I guess.
The first check:
if (a->IsIgnoringCombat() || b->IsIgnoringCombat()) return false;
just makes sure that any unit that is set to ignore combat can't attack nor be attacked. I stumbled upon this check when I looked at some changes in the 3.3.5 branch and master branch where it can be found as well, though on these branches it goes by a different name, but it does the exact same. By the looks of it, the units that are flagged as ignoring combat are all just "bunnys" used for spell scripts, events and whatnot, so not units that players will or should ever interact with. I guess its just "nice to have".

The second check:
if ((playerA && playerA->GetFaction() == FACTION_FRIENDLY) || (playerB && playerB->GetFaction() == FACTION_FRIENDLY)) return false;
was to fix an issue I experienced, where I would get disconnected if I right-clicked (in melee range to start melee auto attack) on NPCs that were of the opposite faction e.g. Alliance such as Ironforge Guard, while having gm mode active (.gm on).
Theres not really a reason not to have this check unless you want to get disconnects or has a better way of fixing it. The thing is, there should not ever be a case where FACTION_FRIENDLY should to be able to engage in combat because this faction is supposed to be friendly with literally everyone, and there also shouldn't ever be a case where a non-gm player would be of this faction.

Moving on to the Charge and macro part.
So first things first, Charge should not affect attack swing timer by any means. You'll find that, without this fix, everytime you Charge you will have to wait for the amount of time equal to your Main-hand weapon speed. Like, take this random video from 2013, https://youtu.be/K6wkgWfqbMM?t=10 watch at 0:13 when he Charge, he immediately does a white attack - he basically swings his weapon mid-charge. This issue was recently dicussed and fixed on 3.3.5 branch as well, see: TrinityCore/TrinityCore#26860

As for the macro part, this is also something that has been discussed on 3.3.5. See: TrinityCore/TrinityCore#17589 Here you can also see the steps to reproduce the issue.

[Ovahlord]

I'd like to skip the ignoreCombat part because I did not port this on purpose. I was involved in the discussions back then when 335 was adding it since I was getting the impression that ignoring combat in this sense means that the creature wont engage in combat unless it's explicitely told to do so. Hence, we parted ways on that and I sticked with my implementation which is suprisingly promising.

[AGandrup]

Fair enough. I also only added that part because I thought it was missing and I thought it wouldn't hurt anything.

[AGandrup]

I guess I'll close this as most of it has been implemented.

As for the things that has not been implemented, I will open a new PR at some point - at least for the Warrior Charge changes.