Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰 [Bug Report] Hammer of Wrath always misses #3525

Closed
f0n1x opened this issue Aug 15, 2023 · 5 comments
Closed

馃悰 [Bug Report] Hammer of Wrath always misses #3525

f0n1x opened this issue Aug 15, 2023 · 5 comments
Labels
Info: Needs Replication Issue needs replication before further action.

Comments

@f0n1x
Copy link

f0n1x commented Aug 15, 2023

Bug Details

Casting the Paladin ability, Hammer of Wrath, will always miss.
Video

Steps to Reproduce

  1. Create a Paladin
  2. Level to at least level 44 to learn Hammer of Wrath (Rank 1)
  3. Fight a mob and reduce it's health to =< 20%
  4. Cast Hammer of Wrath

Expected behavior

Hit and damage.

Suggested Workaround

No response

Crash Log

N/A

Core SHA1 Commit Hash

b788602466d629c15dd630aa7f03d6900352d0e9

Database SHA1 Commit Hash

c6a92ebd20b29abf3d84b186e187627fc8e46763

Operating System

Slackware64-15.0

Client Version

1.12.1 (Classic)

@f0n1x f0n1x added the Info: Needs Replication Issue needs replication before further action. label Aug 15, 2023
@f0n1x
Copy link
Author

f0n1x commented Aug 27, 2023

Interesting update here... Hammer of Wrath does hit it's target in a duel with a player.

killerwife added a commit to cmangos/mangos-tbc that referenced this issue Sep 23, 2023
@killerwife
Copy link

I linked a commit in tbc branch, that will eventually make it to vanilla branch as well 1:1. Use that if you want to try test this again.

@f0n1x
Copy link
Author

f0n1x commented Sep 24, 2023

Thanks KW, appreciate all your work! Will test later and report back.

@f0n1x
Copy link
Author

f0n1x commented Sep 25, 2023

There is no matching line in classic for Unit::CalculateEffectiveMissChance() for weapon (only ranged)?
const bool weapon = (!ability || ability->EquippedItemClass == ITEM_CLASS_WEAPON);
should be @ line 3690.
I tried applying the patch anyway by removing line 56, but Hammer of Wrath is still missing.

killerwife added a commit to cmangos/mangos-classic that referenced this issue Sep 25, 2023
@f0n1x
Copy link
Author

f0n1x commented Sep 26, 2023

Confirmed fixed with d6b4106.

@f0n1x f0n1x closed this as completed Sep 26, 2023
killerwife added a commit to cmangos/mangos-wotlk that referenced this issue Sep 28, 2023
BadgerBoy81 pushed a commit to BadgerBoy81/mangos-classic that referenced this issue Jan 5, 2024
BadgerBoy81 added a commit to BadgerBoy81/mangos-classic that referenced this issue Jan 6, 2024
* Scripts: Implement string_id system

* [z2808] Add creature_spawn_data_template Name

* Creature: Reorganize code to be able to default to GameEventCreatureData entry if none set

* Creature/Dynguid: Fix previous commit having wrong order

* ScriptId: Fix startup of core not noticed on tbc

* [z2810] Implement UnitCondition, CombatCondition and WorldstateExpression and integrate into spell lists

Implement UnitCondition backbone

Implement CombatConditions

Fix compilation of UnitConditionMgr::Meets

Implement CombatConditions

Add implementation for UnitCondition types

Implement WorldStateExpression

Taken from TC master

Add data for combat condition and worldstate expression

Adjust WSE_FUNCTION_REGION

Implement UnitCondition and CombatCondition columns for spell lists

Add worldstate_expression data

Fix tabs and fully move to enum class

Implement safeties and reloading

Tryfix linux compilation

Fix sql syntax

Fix bugs and stupidities

* Scripts: Implement SCRIPT_COMMAND_SET_STRING_ID

* Creature: Implement and rename NO_MELEE_FLEE

* Creature: Implement SESSILE

* AI: Fix death prevention crash due to CreatureStaticFlags::UNKILLABLE

* Prevent using dangling pointer to HonorStanding.
Closes cmangos#500

* Fix an old crash that happen randomly especially at startup

Using lock_guard to let the compiler decide when its time to unlock seem to fix the issue.

* Adjust ExtractResources script to be POSIX-compliant again

Also set CMAKE to copy the extraction scripts over with the executable flag set on Linux.

Co-authored-by: Dwyriel <Dwyriel@users.noreply.github.com>

* Fixed error handling in ExtractResources.sh

* Changed ExtractResources.sh to not prompt for any settings when started with the "a" parameter

Also changed the high-resolution prompts to indicate no is the default value.

* Fixed all occurrences of 'SC2086 - Double quote to prevent globbing and word splitting' in the extraction scripts

Also fixed some SC2006 style warnings.

* Extractor scripts: fix bad command arg generation

* [SD] Move SetStandState for npc_dorius_stonetenderAI

Add 1 aggro text

* Creature: Fix spawn data template equipment for tempsummons

* UnitCondition: Make NUMBER_OF_MELEE_ATTACKERS work using threat list for creatures

* WorldState: Change core defined worldstates to use new numbering scheme

* Spell/Creature: Make guardians use spell lists when set in ctemplate

* CombatCondition: Fix logical typo

* AI: Fix probability rolls leading to overflow of uint

* ScriptMgr::LoadScripts: fix missing breaks.

* ExecuteDbscriptCommand: don't unconditionally cast WorldObject to Unit.

The WorldObject pointers given to ScriptAction::ExecuteDbscriptCommand()
could be pointing to an actual Unit or to a GameObject, depending on the
script. In the SCRIPT_COMMAND_CAST_SPELL case however, an unconditional
downcast of the target WorldObject to Unit was present, which triggers
undefined behavior if the target is not actually a Unit. Because the
result of the downcast was also passed to API that needs a valid Unit
object, this typically also caused a crash later on.

To change avoids the wrong downcast and implements a hopefully sensible
version of the spell cast in case the target WorldObject is actually a
GameObject.

This fixes cmangos/issues#3464.

* Fix 'use after free' when deleting triggered item spells.

Spells originating from using items have the item as a member, and when
these spells trigger other spells, the triggered spells 'inherit' the
item member. When an item spell consumes the item and deletes it, this
item member is reset again - see Spell::ClearCastItem() which is called
by Spell::TakeCastItem(). However, this does not reset anything in the
triggered spells, so their item member simply becomes a dangling
pointer at that point, and we need to avoid using it!

* Creature: add missing GetCombatData() override.

* AI: Fix infinite loop for probability 0

* AI: Add some warning on load for creature_spell_list

* AI: Add one more to previous commit

* AI: Execute actions before spell lists to give them higher priority

* DB: Remove obsolete unused table

* DB: Delete more deprecated tables

* Spell: Fix spell_script_target misleading errors

* AI/StaticFlags: Implement CreatureStaticFlags::CALLS_GUARDS and modernize its implementation

* Guard/AI: Remove redundant parts of guard ai to be added in DB

* Map: Add single creature helpers for SD2 for stringId

* ScriptAction::ExecuteDbscriptCommand: spell cast code streamlining.

* Fix GCC/clang 'statement is misleadingly indented' compiler warnings.

* Replace std::unique_ptr<T>(new T(...)) with std::make_unique<T>(...).

* Fix 'use after free' when deleting battlegrounds.

BattleGroundMap::Update() is calling Update() on the BattleGround
associated with the map, which is typically the Update() call of a
derived class like BattleGroundAB. These start by calling the base
function BattleGround::Update(). So when BattleGround::Update() decides
to delete the battleground and return, this is kind of sawing off the
branch we're still sitting on! If the object gets deleted, the Update()
call of the derived class can't doing anything any more, but in practice
it was happily continuing with its own Update() code, leading to all
kinds of nasty things (and usually a crash).

* Naxx/Gothik: fix code putting adds in combat when the gates open.

* GameEvent: Add game event checks for nonexistant guids

* Unit: Rename SpellEntry usage to common spellInfo rather spellProto

* Unit/Spell: Refactor naming inside CalculateDamageAbsorbAndResist

* Creature/Movement: Include random movement in MotionMaster::PauseWaypoints

* AI: Set ranged mode automatically for guards who have spell lists

* Chat: Disallow tping to bg or arena using go xyz command

* Chat/Creature: Fix removing a dynguid npc using .creature delete and prevent spawn group creature being removed

* Chat/Creature: Fix removing a dynguid gameobject and prevent spawn group gameobject from being removed

* EAI: Fix EVENT_T_DEATH not working with two layers deep PCs and fix npcs not being checked against cond

* Unit: Move LOS check from just EAI to general spell target picking for AI

* Fix warden scans terminator logic.

vmangos/core@eeb6d45 - all credit goes to this

* Spell: Implement SCRIPT_LOCATION_CRIT_CHANCE and rename CalculateSpellCritChance

* Mage: Implement ShatterMage using SCRIPT_LOCATION_CRIT_CHANCE

* Unit: Match Unit.h signatures to recent renames in .cpp

* Spell/Aura: Implement OnAffectCheck

* Shaman: Reimplement Healing Way using spell script

* Spell/Paladin: Add attacker to OnDamageCalculate hook and reimplement Blessing of Light

* Spell: Add safeguard against affect checking nullptr spell

Case for scripting melee affect using override scripts

* Spell: SPELL_ATTR_EX3_IGNORE_CASTER_MODIFIERS should not affect armor application

* Map: Fix ScriptedInstance::RespawnDbGuids for spawn group

* Map: Fix linux compilation of previous commit

* Spell/Mage: Implement m_damageDoneMultiplier and use it for Ice Lance frozen bonus

* Unit: Propagate spell_increased_healing_done_dummy to more spells

* Paladin: Reimplement Judgement of Light and Wisdom using intermediate spells

* Unit/Spell: Remove last batch of SCRIPT_LOCATION_SPELL_DAMAGE_DONE

* Port lord kazzak spells to spell scripts to mirror tbc

* GameObject: Generic check for ignoring GO los by marking it m2 is incorrect

Tested stratholme back entrance on official and vs ours and it is meant to produce LOS

If some GOs are confirmed to ignore los, need to go the per go route

* Unit/Spell: Resolve weapon skill usage bugs

cmangos/issues#3525

* Unit/Spell: Fix AnyFriendlyUnitInObjectRangeCheck missing assistability condition

* Unit/Spell: Add missing hook into CalculateEffectiveCritChance for SCRIPT_LOCATION_CRIT_CHANCE

* Spell/Wand: Make wands use weapon skill for hit and crit chance calc

TODO: Confirm whether wanding hit shares die with resist in vanilla and tbc or separate die

* Map: Properly pass instance id to dk map

cmangos/issues#3528

* Player: Make GetReqKillOrCastCurrentCount const

* TimerAI: Do not reset timers on evade for TIMER_ALWAYS

* SpawnGroup: Disallow aggro of same group member through CREATURE_GROUP_AGGRO_TOGETHER

* SpawnGroup: Allow spawn group aggro from previous commit for MC/Charm case

* [Spell] Ghost Visual Spells should not be removed on Evade

* Add backfacing, frontfacing and evade attributes

* Fix memory leak in G3D::Array

Bug introduced in cmangos/mangos-wotlk@2fa08e3

* Spell: Move FacingCasterFlags to spell_facing

* QueryCallback: simplify/modernize. Callback: remove, no longer needed.

We've been using C++11 (and later) for many years now, so we no longer
need a bunch of boiler plate code to get something like a function with
a varying number of parameters that we can use as a callback; that is
covered by standard functionality like std::function, std::bind, etc.

* Improve Database API to avoid memory leaks in the future, part 1.

Given the caller takes ownership of the query result that's returned by
Database::Query(), let's return a unique_ptr instead of a raw pointer.
This makes the ownership clear in the API and avoids memory leaks when
the caller doesn't delete the pointer in all scenarios (think early
return or exceptions).

* Improve Database API to avoid memory leaks in the future, part 2.

Given the caller takes ownership of the query result that's returned by
Database::PQuery(), let's return a unique_ptr instead of a raw pointer.
This makes the ownership clear in the API and avoids memory leaks when
the caller doesn't delete the pointer in all scenarios (think early
return or exceptions).

* Improve Database API to avoid memory leaks in the future, part 3.

This commit reworks SqlQueryHolder to return a unique_ptr instead of a
raw pointer to the query's result in its API.

It also fixes a memory leak in Player::_LoadForgottenSkills() because
the incoming query result was not deleted.

* Update GitHub Workflows

Updates functions that use soon to be deprecated Node.js 12 action and action commands

* Update Github workfows spellings

Fix misspellings, and organize verbiage.

* Update Github Workflows to boost 1.83

* Should fix win build

* Spell/Unit: Add absorb information to proc system

* Paladin: Eye for an eye should also use absorbed damage in reflection

* Spell/Unit: Make PROC_EX_ABSORB tagged hits also proc on full absorb

* Spell: Fix passing of wrong proc flag to PROC_EX_CAST_END event and minor logic simplification

* Spell: Fix spells with negative SPELL_EFFECT_THREAT and SPELL_ATTR_EX_NO_THREAT

* Unit: Another batch of renames

* Spell: Add DamageEffectType damageType parameter to OnAbsorb

* GameObject: Despawn/Refill chests 5 min after first opening even if not looted

* Spell: Add spellInfo to OnCritChanceCalculate

* Spell: Remove redundant code

* Spell: Add castItem to OnAuraValueCalculate

* Spell/Auras: Implement last custom aura damage switchcase using spell scripts

Actually a functional change because now it will be used during stacking step

* Spell/Warlock: Port healthstone script to spell scripts

* Scripts: Fix SCRIPT_FLAG_BUDDY_BY_GUID with dynguid

* Fix unit_condition not being insertable on db end

* BRD: Implement s.27673

* Unit/Creature: Implement CreatureStaticFlags::DESPAWN_INSTANTLY

* Another round of GCC/Clang 'unused parameter' compiler warning fixes.

* Fix excessive arguments for a format string.

This fixes a GCC/clang 'warning: too many arguments for format
[-Wformat-extra-args]' compiler warning.

* npc_daphne_stilwellAI: remove superfluous checks.

The member m_wave is an unsigned integer and those are always positive.
This also fixes a pair of GCC/clang 'warning: comparison is always false
due to limited range of data type [-Wtype-limits]' compiler warnings.

* world_map_kalimdor: add missing initializer creating a GhostOPlasmEvent.

Practically speaking this makes no difference at all: for a non-union
aggregate each element that is not an explicitly initialized element,
has no default member initializer, and is not a reference, the element
is copy-initialized from an empty initializer list anyway. But it fixes
a GCC/clang compiler waring: 'warning: missing initializer for member
'GhostOPlasmEvent::summonedMagrami' [-Wmissing-field-initializers]'.

* AI: Implement SPELL_LIST_FLAG_NON_BLOCKING

* BRD: Remove FiveFatFingerExplodingHeartTechnique on effect execute

* Improve WorldPacket API.

* Remove the (user defined) copy constructor, because a member wise copy
  is fine here. As a result, the compiler will now implicitly declare a
  defaulted move constructor and move assignment. Yay!
* Remove the constructor with a time_point. It's not used anywhere.
* Make all members private.

* Fix sending empty packet for locked servers caused by wrong variable.

* Narrow the scope of (WorldPacket) variables to avoid a shadow variable.

* Fix GetCreatureCooldown() to use a reference for the out parameter.

* SpawnGroup: Fix crash due to enemy not existing on assist aggro

* Spell 27978 should not apply UNIT_AURAFLAG_ALIVE_INVISIBLE

* Spell: Add missing backport for script s.26264

* [SD] Add missing Aura Passives to npcs

Correct Thrash Aura Passive for Soulflayer 11359 - npc_soulflayer

Add missing Thrash for Maws 15571 - boss_maws

Add missing Frost Armor for Maleki the Pallid 10438 - boss_maleki_the_pallid

* Player: Implement CONFIG_BOOL_DISABLE_INSTANCE_RELOCATE

* Stormwind: Modernize Dashel Stonefist to support spell lists

* Stormwind: Add detail to dashel stonefist

* Player: Make shapeshift forms which dont override attack speed use weapon damage

* Spell: Disable sending packet with aura removal for self when possessed

* Chat: Implement .npc despawn command

* Dynguid: Refactor non spawn group dynguid code to account for despawn on grid load

* StringId: Fix string id objects not being cleared on despawn

* Unit: Adjust IsAttackSpeedOverridenShapeShift for vanilla

* Spell: Also use Damage Done Multiplier script setting in healing

* Fix combat_condition not being insertable on db end

* Creature: Add error if creature_template_spells is conflicting with creature_spell_list_entry

* Creature: Fix UpdateEntry protocol not working whilst being ridden by player

* SD2: Script for Q.8519 Removed

Fixed and moved to DB.

+ small fix for 18728

* Spell: Clear item pointer on consumption

Closes cmangos/mangos-wotlk#476

* Player: Refactor corpse reclaim delay code to fix crash
Closes cmangos/mangos-wotlk#477

* Spell: Implement m_effectSkipMask

* Spell: Enforce los ignore due to range index 13 for single target targeting

* SQL: Fix spell.sql syntax errors

* Adjust max creature level for dummy npcs

* Group: Add missing packet fill for difficulty in

@ratkosrb for discovery

SMSG_GROUP_LIST

* Don't generate pet number twice on tame.

vmangos had same issue vmangos/core@b1e49ba

* Prevent unneeded item_instance update when loading from db.

* Remove suspicious comma

* Fix/merge upstream (#13)

* Anticheat: Handle fall reset opcode

Taken from vmangos/core@ddefce1

* Core/Vmaps: Fix inconsistency of hitInstance and hitModel to cause wrong area ids

Partly needed
TrinityCore/TrinityCore@c52f69e

* Scripts: Implement string_id system

* Scripts: Implement targeting using stringId for spell_script_target and dbscripts

* [z2808] Add creature_spawn_data_template Name

* GameEvent/Creature: Minor renames for GetCreatureUpdateDataForActiveEvent

* SpawnGroup: Fix SPAWN_GROUP_DESPAWN_ON_COND_FAIL imposing a respawn delay

* Creature: Reorganize code to be able to default to GameEventCreatureData entry if none set

* Creature/Dynguid: Reorganize code to get rid of dependancy in WorldObject::SpawnCreature

* Creature/Dynguid: Fix previous commit having wrong order

* Remove defines related to old GCC versions we no longer support.

Back when we introduced support for C++11 in the core, we also added
this workaround to allow GCC versions before 4.7 that supported most of
C++11 (Ubuntu 12.04 LTS shipped with GCC 4.6 for example). But we've
dropped support for these old versions of GCC years ago because they
are not able to build the core any more, so this workaround can go too.

* Fix a server crash when a targeted creature dies.

The functions Unit::SendMeleeAttack{Start,Stop} were basically unable
to handle a nullptr argument, yet this is exactly what happened to be
passed by WorldSession::HandleAttackSwingOpcode(), leading to a crash.

Semantically speaking, these functions should probably also not be able
to handle a nullptr argument, because it makes little sense to say one
starts or stops attacking 'nothing'. So rather than handle a nullptr
argument, it seems to make more sense to not allow 'nothing' as a target
in the first place, so let's adjust the API to make this clear by using
a reference instead of a pointer.

It is then basically up to the callers to ensure they are passing a
valid target to Unit::SendMeleeAttack{Start,Stop}(), which already was
the case everywhere in the core except for one easily fixable call in
WorldSession::HandleAttackSwingOpcode().

This is the TBC part for cmangos/issues#3291 and cmangos/issues#3307.

* Fix unhandled ByteBufferException exceptions causing a server crash.

When opcode handlers are not using the correct packet structure, reading
from packets can cause an opcode handler to read data that isn't present
in the packet, causing a ByteBufferException.

These exceptions were mostly handled by the core, but not in the case of
player bot packets or immediately processed packets, where the unhandled
exception caused a std::terminate(), which users see as a core crash.

This is a TBC part for cmangos/issues#3281.

* Improve ByteBuffer API.

* Combine both constructors into one constructor that takes an initial
  storage size, with a default value, and make the constructor explicit.
  After all we don't want integers to implicitly convert to ByteBuffers.
* Add a default virtual destructor, because this class is used as a base
  class (see WorldPacket) - although not as a polymorphic class - in
  addition to being used by itself, so a protected and non-virtual
  destructor doesn't work here.
* Add a move constructor and move assignment operator because the user
  defined destructor blocks implicit moves otherwise. Note that we
  can't use default versions here, because that could leave the source
  in an invalid state (for example an empty _storage but _wpos and/or
  _rpos > 0), which is generally a bad idea.
* Make the copy constructor default. The implementation was a member
  wise copy anyway, which is indeed fine here.
* Add a default copy assignment operator as well.
* Make all members private. Derived classes can also use ByteBuffer API
  rather than mess with the class internals directly.
* UpdateData: follow these API changes. In particular, construct the
  wanted ByteBuffer directly instead of relying on the integer 0 to be
  implicitly converted to an empty sized ByteBuffer and assigning that
  to a default constructed one...

Closes cmangos/mangos-tbc#586.

* Remove unused parameter of ObjectMgr::IsVendorItemValid().

This also fixes a few more 'unused parameter' [-Wunused-parameter]
GCC/Clang compiler warnings.

* Fix a handful of memory leaks in Playerbot, leaking spells.

Note that the non-throwing versions of operator new should never return
nullptr (they throw an exception, typically std::bad_alloc) so we don't
have to check for that.

* SpawnGroup: Add safeguards for SPAWN_GROUP_DESPAWN_ON_COND_FAIL when using grid unload

* GameObject: Fix wrong lootstate being set for doors and buttons

* Generate all build output in CMAKE_BINARY_DIR.

This fixes builds when the repository is on a read-only file system.

* Remove escort state when 'End()' gets called

* Send message on areatrigger teleport fail

Message can be localised

* SpawnGroup: Fix logical typo in Go group despawn

Closes cmangos/issues#3390

* SD2: Removed Script for Q. Curing the Sick

handled in DB

* Spell: Add out of range error to TARGET_GAMEOBJECT_SCRIPT_NEAR_CASTER

* Spell/ZG: Disable Spirit of Zandalar stacking

* Spell: Fix bool logic typo for Spirit of Zandalar

* ScriptId: Fix startup of core not noticed on tbc

* Fix 'use after free' when unloading all grids at server shutdown.

This is basically the same issue that cmangos/mangos-tbc@365c6342 fixes,
but in a different context. Unloading grids not only happens when idle
grids expire after some time, but the core also unloads all grids when
it shuts down - see Map::UnloadAll(). However, in this case dynamic
objects weren't removed from creatures before the grid is unloaded,
triggering the original problem again (see the referenced commit for the
details).

So let's move the dynamic object removal to Map::UnloadGrid() to make
sure we're doing it in all cases before unloading a grid.

This fixes the reported crash in cmangos/issues#3382.

* Fix 'use after free' when battleground queue events are processed.

When a player joins a battleground, new battleground queue events are
added to its EventProcessor - see BattleGroundQueue::InviteGroupToBg().
When the battleground map is updated, these events are processed by an
EventProcessor via a chain BattleGroundMap::Update() > Map::Update() >
Player::Update() > Unit::Update() > EventProcessor::Update(), and the
event's Execute() is called (assuming the event is not aborted).

In the case of BgQueueInviteEvent and BgQueueRemoveEvent, Execute()
merely schedules a lambda to be executed later and unconditionally
returns true, meaning the EventProcessor can delete the event. But the
lambda was using 'this' (the event) and captured it by reference (if
implicitly captured, the current object is always captured by reference,
even if the capture default is =), so by the time the scheduled lambda
was actually executed, 'this' was very likely a dangling reference
because the EventProcessor already deleted the event object!

So let's give the lambda a copy of the event object so it's guaranteed
to have a valid object. Note that the implicit capture of *this when the
capture default is = is deprecated since C++20, so capturing the current
object explicitly is a 'future proof' improvement anyway. A capture
like [*this] would also work since C++17 (which the core is using now)
but making it obvious we need a copy of the current event instead of
relying on using 'this' and hoping it won't be a dangling reference
doesn't seem like a bad idea.

This fixes a second issue, specific to battlegrounds, in cmangos/issues#3382.

* Fix out of bounds accesses in ConditionEntry.

For better or worse, ConditionType and ConditionSource enum values are
used as indexes in fixed arrays. When ConditionType CONDITION_WORLDSTATE
and ConditionSource CONDITION_FROM_WORLDSTATE were introduced however,
these arrays were not extended with values for the newly introduced
enumerators as expected. This causes an out of bounds access when the
new enumerators are used in the database (which is the case nowadays)
and all the nasty things that come with that (such as using garbage
data instead of the expected array values, crashes, etc).

* Another round of GCC/Clang 'unused parameter' compiler warning fixes.

* Anticheat: Delete merge error goodness

* Playerbot: fix parsing of SMSG_LOOT_RESPONSE packets.

This is the Classic part fixing the root cause of cmangos/issues#3281.

* Remove unused 'entry' parameter from ObjectMgr helper function.

The changes in 9f79cc6aed46c4c5 made this parameter obsolete.

* Fix database query result memory leaks.

Database::Query() passes ownership of the QueryResult to the caller,
so it's the caller's responsibility to delete the QueryResult when
it's no longer needed, but this didn't happen everywhere in the core.

* Remove unused CreatureInfo parameter from Helper_CreateWaypointFor().

* Fix memory leaks from SpellScripts and AuraScripts.

These leaks aren't a big problem per se because the scripts are created
once and then remain alive until the core quits anyway, after which the
operating system reclaims the memory. But we have quite a long list of
scripts in use nowadays, and having them all leak makes it much harder
than necessary to see the real memory leaks...

* Give SpellScript and AuraScript a virtual destructor.

This is needed to call the correct destructor when SpellScripts and
AuraScripts are deleted via a pointer to base class, something that
now actually happens after the previous commit.

* Remove unused members from TileAssembler.

* Fix loaded instance encounters memory leaks.

Note this is now similar to what's already done for other maps in the
object manager like those for GossipMenuItems, GossipMenus, etc.

* Give (Warden) Scan class a virtual destructor.

This is needed to call the correct destructor when Scan objects are
deleted via a pointer to base class, which is exactly what happens when
WardenScanMgr's m_scans member is destroyed.

* Fix map count reported by the mmap generator. Also fixes memory leaks.

In Mapbuilder::discoverTiles() we first search for maps based on the
contents of "maps" (as generated by the extractor) and then continue
the search based on the contents of "vmaps" (as generated by the vmap
assembler). Before this commit however, we didn't always check if a map
already had a tile set, so we also counted duplicates and eventually
reported a wrong number of discovered maps (106 instead of 72 for TBC).

Unfortunately, trying to add a tile set for a map that already had one
had another bad side effect: the newly heap allocated set would not
really be inserted in the map, which leaks the associated memory.
While the added check also avoids this memory leak, there is no reason
why we can't use an std::set in the map (rather than a pointer to one),
so this change also reworks the code a bit to easily avoid such memory
leaks in the future.

* Fix GCC/Clang 'member will be initialized after' compiler warnings.

In C++, the base classes and members of a class are initialized in the
order in which they are declared in the class, regardless of the order
in which they appear in the initializer list. To avoid any possible
confusion (and compiler warnings) let's specify the initializers in
declaration order.

* Catch exceptions by reference, especially polymorphic types.

* Fix another database query result memory leak.

* Another round of GCC/Clang 'unused parameter' compiler warning fixes.

* Fix a handful of typos.

* Move GetMoveTypeStr() function to the one source file where it's needed.

Apart from not 'polluting' the MotionMaster.h header file, this also
avoids a bunch of '<function> defined but not used' [-Wunused-function]
compiler warnings.

* Fix MMapManager leaking the NavMesh(Queries) for loaded GOs.

* Improve MMapManager to avoid memory leaks in the future.

Even though loaded MMap data is currently cleaned up as it should, we
might as well get rid of the explicit memory management for MMap data
too, just like we did for MMap GO data in 40e281e.

* [z2809] Creature: Drop equipment_id as scheduled

* [z2810] Implement UnitCondition, CombatCondition and WorldstateExpression and integrate into spell lists

Implement UnitCondition backbone

Implement CombatConditions

Fix compilation of UnitConditionMgr::Meets

Implement CombatConditions

Add implementation for UnitCondition types

Implement WorldStateExpression

Taken from TC master

Add data for combat condition and worldstate expression

Adjust WSE_FUNCTION_REGION

Implement UnitCondition and CombatCondition columns for spell lists

Add worldstate_expression data

Fix tabs and fully move to enum class

Implement safeties and reloading

Tryfix linux compilation

Fix sql syntax

Fix bugs and stupidities

* ZG: Port hakkar to using new worldstate expression combat conditions

* Scripts: Implement SCRIPT_COMMAND_SET_STRING_ID

* Fix UnitCondition result in Unit::MeetsSelectAttackingRequirement not being used

* Fix formatting of WorldStateExpressionFunctions

* Fix dire maul DungeonEncounter conflict

* Fix 'dynguid' creatures being spawned multiple times when grids reload.

When grids are (re)loaded, a grid is (re)spawned by an ObjectGridLoader
that spawns things based on what the object manager said should be
spawned on that grid. For creatures, we're initially assuming that a
Creature will need to be (re)spawned, we call Creature::LoadFromDB() to
fully construct the creature given its database guid, and eventually add
it to the map when everything works out (and discard it otherwise). One
of the things LoadFromDB() does is check if a creature really needs to
be respawned; after all a creature could have moved to a different grid
and still exist in the world, despite the grid it originally spawned on
going through an unload/load cycle.

Unfortunately this check was flawed when creatures use dynamic guids, as
is typically the case when they are in a spawn group, because the check
was done using a function that checks based on actual guids instead of
database guids, so there was never a match. As a result, these creatures
were always respawned, showing up as duplicates in the world.

This fixes the TBC part of cmangos/issues#3271.

* Implement enum class helper
Heavy inspiration from EnumClass from TC with C++17 upgrade

* [z2811] Creature: Implement creature static flags

* Creature/GameObject: Move creature check and add gameobject check for dbguid uniqueness

* Fix typoe in previous commit

* Creature: Remove redundant comments and fix linux compilation of static flags

* Unit: Add UNIT_STAT_AI_ROOT designation to enable AI script root to persist through root aura removal

* EAI: Fix ranged AI interfacing with spell lists

* Fixup mangos.sql after recent additions

* Creature: Implement and rename NO_MELEE_FLEE

* Creature: Implement SESSILE

* Spell/Unit: Fix linux compilation

* AI: Fix death prevention crash due to CreatureStaticFlags::UNKILLABLE

* Creature: Implement CreatureStaticFlags::CREATOR_LOOT

* Creature: Fix the fix of the duplicate respawn fix fix

*smh

* fix multithread access segmentation fault

* SpawnGroup: Also fix GO groups similarly to previous commit

* Add needed virtual destructor to the (map) Worker class.

This is needed because MapUpdater::WorkerThread() can delete objects
of derived classes, like MapUpdateWorker, via a pointer to base class,
and without a virtual destructor this isn't calling the correct
destructor!

Closes cmangos#499

* Prevent using dangling pointer to HonorStanding.
Closes cmangos#500

* Remove duplicated code block from TrainerSpell.

* Fix PlayerBot crashes caused by mishandling UTF8 chat messages

Closes cmangos/issues#3324

* [SD] AQ40: Update Flesh Tentacle 15802 Summon Locations

* Fix large GOs visibility override map visibility

* prevent gizletone  spawn_group going into normal movement on point 19 when doing southside quest.

* small correction, just be false again when quest finished

* fix small error that blocked the text before walking normal path

* Allow .go warp command values to use decimals

Allow .go warp command values to use decimals for more precision

* [Spell] Bloodlust s.6742 & Lava Splash Auras should not be removed on Evade

Also correct LowLevelHideDiff in config - missed to change with cmangos/mangos-tbc@80a6d4b

Also increase GuidReserveSize.Creature & GuidReserveSize.GameObject for development reasons

Remove Mortal Strike Aura Passive from not being removed on Evade, used
by : npc_anubisath_sentinel which will set a new aura each time they are
pulled.

Add s.36630 due to c.22000

* [Quest] Rescript 'Rescue From Jaedenar' quest

* missing change

when entry gets changed (on wp 37) she can use spells in combat

* [Quest] Fix Suntara Stones - Escort

* [Spell] Moss Hide s.8852 should not be removed on Evade

* Player/Quest: Do not set quest slot counter for item objective

@AnonXS you asked for this :P

* [z2812] Creature: Merge visibility back to static flags where it initially originated

* [z2813] NPC: Implement trainer ability requirement

* NPC: Add Player::GetTrainerSpellState check for reqAbility

* AuctionHouseBot: Fix invalid gameobject loot query by removing state condition

* Spell: Default to 0 for all shapeshifts and enable overriding in script through GetAuraScriptCustomizationValue

* -Only save polyRef if it is useful (new) and actually use the saved polyRefs

* Fix Possess action bars regression

Also expand the comment a bit on why we grant control in this context

* GameObject: Fix typo from porting in GAMEOBJECT_TYPE_SPELLCASTER

Closes cmangos/issues#3308

* make it possible for bots to take escort quests (#5)

* allow bots to complete repeatable quests (#6)

* check error before loot (#7)

Co-authored-by: MOSASAURUS\Andreas <andreas.garne@gmail.com>

* use item if no menu options (#8)

Co-authored-by: MOSASAURUS\Andreas <andreas.garne@gmail.com>

* respect fishing config settings (#9)

* fix: hardcode default auctioneer to guid 7 (#10)

* fix: dont buy ah toons stuff (#11)

* fix: set max soul shards to 2 (#12)

* rebase upstream

---------

Co-authored-by: killerwife <killerwife@gmail.com>
Co-authored-by: evil-at-wow <evil.at.wow@gmail.com>
Co-authored-by: Miraco <daaniel743@gmail.com>
Co-authored-by: celguar <celguar@gmail.com>
Co-authored-by: Grz3s <Grz3s@users.noreply.github.com>
Co-authored-by: cdkr <775481991@qq.com>
Co-authored-by: mostlikely4r <sebastiaan.keek@gmail.com>
Co-authored-by: Warlockbugs <warlockbugs@outlook.com>
Co-authored-by: AnonXS <anonxsdev@gmail.com>
Co-authored-by: CyberMist <cybermist2@gmail.com>
Co-authored-by: Exxenoz <exxenoz@gmx.at>
Co-authored-by: MOSASAURUS\Andreas <andreas.garne@gmail.com>

* Fix/merge upstream 4 (#17)

* Scripts: Implement string_id system

* [z2808] Add creature_spawn_data_template Name

* Creature: Reorganize code to be able to default to GameEventCreatureData entry if none set

* Creature/Dynguid: Fix previous commit having wrong order

* ScriptId: Fix startup of core not noticed on tbc

* Scripts: Implement SCRIPT_COMMAND_SET_STRING_ID

* Creature: Implement and rename NO_MELEE_FLEE

* Creature: Implement SESSILE

* missing break

* Spell: Fix tabs

* [Spell] s.18163 & s.18167 used be c.11018 should not be removed on Evade

Add s.12896

* Fix Wheaty not outputting dumps

* Fix [Cosmetic] Spells of unlearned professions are not deleted clientside until relog
Closes cmangos/mangos-tbc#589
Fixing [Bug Report] [Cosmetic] Spells of unlearned professions are not deleted clientside until relog
cmangos/issues#2810

* [z2814] SpawnGroup: Add worldstate expression support to spawn group as alternative

Avoids us having to define conditions on our end for everything.

* NPC: Interrupt casting on selling an item

Yes, buying doesnt interrupt, buyback doesnt interrupt, repair doesnt interrupt, but selling does.

* Fix a buffer overflow when spawning static creatures with TotemAI.

The core currently does not support spawning creatures that use TotemAI
as AIName via the creature table.

That's because when a grid is loaded, the core spawns the creatures in
that grid via a call to ObjectGridLoader::Visit(CreatureMapType&), which
delegates the actual spawning to a LoadHelper<Creature>() call. Notice
that the template parameter T in this call is always 'Creature', so when
this call creates the C++ object associated with the creature ('new T'),
it will always create a Creature object, never any derived type. When
initializing the Creature object afterwards, via Creature::LoadFromDB(),
this will also set up the creature's AI via AIM_Initialize(), which
could end up selecting TotemAI as the AI based on the creature's AIName.
This is a problem because Unit::Update() also updates the AI via a call
to AI()->UpdateAI(), and in the case of TotemAI that means calling
TotemAI::UpdateAI(), a call that assumes the associated m_creature is
actually a Totem object rather than just a Creature object! If this is
not the case however, then TotemAI trying to access Totem class members
is basically accessing data out of bounds...

Because the core obviously can't control the data that's present in the
database, let's detect this case and avoid the problem by not loading
such creatures.

* Fix a log format string to match with the number of arguments.

This also fixes a GCC/clang 'warning: too many arguments for format
[-Wformat-extra-args]' compiler warning.

* EAI: Implement EVENT_T_SPELL_CAST

* AI: Ignore cat cooldown for initial CD of spells

* Spell: Fix SPELL_TARGET_TYPE_STRING_ID not being able to be used

* UnitConditon: Fix logical typo

* AI: Fix bad data for casters after resetting spell list for whatever reason

* UnitCondition: Fix one more logical typo

* UnitCondition: Fix two more logical issues

* AI: Properly fill DisabledForAI for creature_template_spells and fix disabled spells starting loop on combat enter

Closes cmangos/mangos-tbc#613

* AI/Map: Fix reloading of spell lists not interfacing with template spells properly

* Spell: Implement SPELL_ATTR_SS_FACING_BACK and assign it to backstab s.15582

* Change backstab attributes to new serverside attribute

* Implement script go_rune_of_the_defiler to disable rune when not having ward of the defiler

cmangos/issues#3436

* Combat: Make immobilized passive mobs ignore last pos requirement

* [z2815] Creature: Implement Pursuit

* EscortAI/Felwood: Fix Arkonarin glitching back to spawn

* Chat: Add warnings to quest add command

* AntispamMgr: make the shut down member atomic.

This makes sure it's properly synchronized between threads. The member
is continuously read from the AntispamMgr's worker loop, which runs in
its own thread, while it is set in the AntispamMgr's destructor, which
gets called when the AntispamMgr's singleton instance is destroyed,
typically at the end of the main thread.

* WorldSession: make the latency member atomic.

This makes sure it's properly synchronized between threads. The member
is set in WorldSession::SetLatency() when handling ping packets, which
happens in one of the network threads, while it is repeatedly read in
WorldSession::GetLatency(), which is usually running in a different
thread (for example the WorldRunnable thread doing an Update() and
calling World::GeneratePacketMetrics() as part of that).

* Make World::ExecuteForAllSessions() const.

It's just iterating over all the sessions and not modifying the session
map or any other members, so it can be const. This also allows to remove
a const_cast in the process.

* Remove a few unnecessary const_cast conversions.

* Socket: make m_address and m_remoteEndpoint members non-const.

This avoids needing const_cast conversions when modifying them, which
is actually undefined behavior in C++ because we were modifying a const
object.

* SqlStmtParameters: cleanup special member functions.

The private-but-not-implemented copy constructor is a relic from the
past to make the class non-copyable, but these days we can just make it
public and deleted. That said, I don't really see a compelling reason to
make this non-copyable.

I also see no reason to explicitly add a default destructor given this
class is not a base class. That way, it nicely follows the so called
"rule of zero" (or the C++ core guideline "if you can avoid defining
default operations, do").

This also avoids a bunch of [-Wdeprecated-copy] GCC/clang compiler
warnings when using recent versions, because an implicitly declared copy
constructor (which is actually used, for example in SqlStatement's copy
constructor) is deprecated when you have the copy assignment declared.

* Spell/GameObject: Fix crashes due to GO cast time casting

* Pet: Ensure pet cooldowns are visible

* BossAI: Add automatic delayed open/close of encounter doors

* Spell: Tryfix trajectory distance calculation by using 2d distance

* Move SpellMods out of Handlers

* Unit/Spell: Resolve weapon skill not being applied to a couple melee abilities

* [Core/Build]Add an option for CMAKE to set dev folder on windows

Dev binary directory is only for windows and help devellopers by collecting all required files to be able to quicly test changes in visual studio without installing it.

Maybe it would be better to enforce the installation?

* [Core/Build] Increase minimum requirement for CMAKE version

To silence end of support warning.

* [Tools]Fix click on RecastDemoMod

No priorities now on map over mmap, taking the more near point.

* [Core/MMAP] Sync some default value for recast with tc for better result

Slope seem to be better by keeping it at 60
WalkClimb is too hight at 6 and we have seen better result with 4
This reduce by the same time some not connected polygons that was seen in some different place.

* [Core/MMAP] update recast navigation

Update to https://github.com/recastnavigation/recastnavigation/tree/ee39124e5f6cb23086b4aa9eebd18454c7342f4f

Also replace provided CMakeList by our.

* [Core/Build]Fix Detour and Recast case

They now use uppercase for their first letter.

* [Core/Build]Fix lib name in debug mode

remove the "-d" added by lib in debug mode for builded .lib

* [Core/MMAP]Change default bit size.

Some part of the  world have havy difference in heigh so this is mandatory.

* [Core/MMAP] Reduce chances of breaking tile connections
Reduce chances of breaking tile connections caused by floating point rounding issues.

* [Core/MMAP] Bump mmaps file version

* [Tool] Add hit for liquid mesh

We can now simulate path follow on liquid navmesh too

* [Core/Build] Fix make install

Try to avoid recastnavigation lib install

* [CI] Add cmake install to ubuntu workflow

* fix upstream merge sep 1

---------

Co-authored-by: killerwife <killerwife@gmail.com>
Co-authored-by: celguar <celguar@gmail.com>
Co-authored-by: AnonXS <anonxsdev@gmail.com>
Co-authored-by: Niam5 <ebakkedahl@gmail.com>
Co-authored-by: CyberMist <cybermist2@gmail.com>
Co-authored-by: evil-at-wow <evil.at.wow@gmail.com>
Co-authored-by: insunaa <dev.insuna@gmail.com>
Co-authored-by: insunaa <insuna@no.capfr.fr>
Co-authored-by: Cyberium <cyberium@users.noreply.github.com>
Co-authored-by: jackpoz <giacomopoz@gmail.com>

* fix bad rebase

* fix wonky rebase

---------

Co-authored-by: killerwife <killerwife@gmail.com>
Co-authored-by: mostlikely4r <sebastiaan.keek@gmail.com>
Co-authored-by: Cyberium <cyberium@users.noreply.github.com>
Co-authored-by: Thorsten42 <Thorsten42@users.noreply.github.com>
Co-authored-by: Dwyriel <Dwyriel@users.noreply.github.com>
Co-authored-by: Muehe <Muehe@users.noreply.github.com>
Co-authored-by: Dwyriel <dwyriel333@hotmail.com>
Co-authored-by: AnonXS <anonxsdev@gmail.com>
Co-authored-by: evil-at-wow <evil.at.wow@gmail.com>
Co-authored-by: insunaa <dev.insuna@gmail.com>
Co-authored-by: Niam5 <ebakkedahl@gmail.com>
Co-authored-by: Jimmy B <jimmybrancaccio@icloud.com>
Co-authored-by: Grz3s <Grz3s@users.noreply.github.com>
Co-authored-by: Karth <karthxyver@proton.me>
Co-authored-by: insunaa <insuna@no.capfr.fr>
Co-authored-by: celguar <celguar@gmail.com>
Co-authored-by: gromchek <gromchek@protonmail.com>
Co-authored-by: Miraco <daaniel743@gmail.com>
Co-authored-by: cdkr <775481991@qq.com>
Co-authored-by: Warlockbugs <warlockbugs@outlook.com>
Co-authored-by: CyberMist <cybermist2@gmail.com>
Co-authored-by: Exxenoz <exxenoz@gmx.at>
Co-authored-by: MOSASAURUS\Andreas <andreas.garne@gmail.com>
Co-authored-by: jackpoz <giacomopoz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Info: Needs Replication Issue needs replication before further action.
Projects
None yet
Development

No branches or pull requests

2 participants