-
Notifications
You must be signed in to change notification settings - Fork 480
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
Implement combat reach based pathing for chase and charge #2386
Conversation
@@ -15696,8 +15696,8 @@ bool Player::IsAllowedToLoot(Creature const* creature) | |||
|
|||
float Player::GetMaxLootDistance(Unit const* pUnit) const | |||
{ | |||
float distance = GetCombatReach() + 1.333333373069763f + pUnit->GetCombatReach(); |
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.
But this function is based on the client, it uses this constant and adds the combat reach of both. Why is the constant not used in your new code? Are you making the loot distance be different server side than it is client side?
This the client's check whether target is in loot distance, decompiled by IDA.
double v2; // st7
float v4; // [esp+8h] [ebp+8h]
v2 = *(float *)&this->UnitBase.UnitData->unitFields.UNIT_FIELD_COMBATREACH
+ 1.333333373069763
+ *(float *)&a2->UnitData->unitFields.UNIT_FIELD_COMBATREACH;
if ( v2 >= 5.0 )
v4 = v2;
else
v4 = 5.0;
return v4;
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 same function used by the client for loot distance is used for melee spells:
highest of the two: (Player combat reach + the melee reach constant + target combat reach) or ( 5 yards)
I made it call to that function to signify to people looking at the code in the future that the two ranges are the same and determined by client.
I should change BASE_MELEERANGE_OFFSET from 1.33f to the full value extracted from the client.
{ | ||
Vector3 dirVect; | ||
pTarget->GetPosition(dirVect.x, dirVect.y, dirVect.z); | ||
dirVect -= m_pathPoints[i - 1]; | ||
float targetDist = pTarget->GetDistance(m_pathPoints[i - 1].x, m_pathPoints[i - 1].y, m_pathPoints[i - 1].z) - meleeReach; | ||
float targetDist = pTarget->GetDistance(m_pathPoints[i - 1].x, m_pathPoints[i - 1].y, m_pathPoints[i - 1].z, SizeFactor::None); | ||
targetDist -= meleeReach; |
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.
Shouldn't you have some guards against targetDist
becoming negative?
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.
It can be negative only if m_pathPoints[0] (the start pos of the movement) is inside the target's meleeReach. Normally the CanReachWithMeleeAutoAttack check that happens before should catch that and return to make the unit charge in place.
Untitled.video.-.Made.with.Clipchamp.3.mp4
2023-12-28.19-27-02.mp4
I tested removing the CanReachWithMeleeAutoAttack check and charging from inside melee range and this is what happens:
2023-12-28.18-26-12.mp4
2023-12-28.18-28-49.mp4
Nevertheless I suppose it's safer to check a second time and interrupt the charge to make double sure the charger doesn't path away from the target?
Though I'm curious to know what exactly do mobs in classic do when they charge a unit that is already in their melee range. All the examples I built this on is charging from a distance. In the case of players all charges have a 8 yard minimum range so it's impossible to make an analogy to mobs.
src/game/Threat/ThreatManager.cpp
Outdated
float attackDistance = pAttacker->GetMaxChaseDistance(target); | ||
// second choice targets are: immune to attacker's autoattack damage school / is secondary threat target (fear, gouge etc) / | ||
// is outside of attacker's caster chase distance if rooted caster / is unreachable by attacker's melee attacks if rooted melee. | ||
if ( target->IsImmuneToDamage(pAttacker->GetMeleeDamageSchoolMask()) || (target->IsSecondaryThreatTarget()) || |
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.
why is (target->IsSecondaryThreatTarget()) wrapped in brackets, its just 1 function call, doesn't need brackets
Brackets brack off
to exact value found by decompiling the client
@@ -10202,7 +10202,7 @@ bool Unit::GetRandomAttackPoint(Unit const* attacker, float &x, float &y, float | |||
float angle = GetAngle(attacker); | |||
float sizeFactor = GetObjectBoundingRadius() + attacker->GetObjectBoundingRadius(); |
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.
Did you forget to change this to Combat Reach or is it supposed to be Bounding Radius?
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 spreading (along with the backpedal) behavior is to be done later. I have videos that show it's current behavior is wrong, but not enough to make a new method of doing it.
Right now the mobs come at the player from a random angle that grows with the number of attackers. This video from classic shows the initial attack point is simply the closest point and not at a random angle, it's only AFTER they reach the player that they start spreading out.
It is meant to be bounding radius at the moment (that's supposedly the hitbox used for collisions) but it will likely get removed entirely later if we get info on how mobs spread (what i need the most is examples of mobs with big bounding radius, like pulling a big number of the Withered Protector mob and recording how they spread around the player)
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.
@balakethelock Can you reply please so we can merge this? Is this an oversight or is it intended?
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.
…angos#2386)" This reverts commit fa09f08.
This PR seems to cause issues with targetted creature movement - if the player is running straight towards the creature. Video-Fri-Mar-29-2024-19-13-32.webm |
Let's wait for @balakethelock to respond. If he can't fix it then we can revert the commit. |
🍰 Pullrequest
Changed the way chase and charge find the end point from bounding radius to combat reach.
Current vmangos behavior: https://www.youtube.com/watch?v=ynGYsxdajfo
Also fixed rooted mob target selecting targets that are slightly outside melee range
https://github.com/vmangos/core/assets/111737968/c9d8188f-2328-43ed-994b-b9ce547329ca
Proof
mobs path much closer to the edge of the hitbox (combat hitbox determinedby combatreach) than currently in vmangos
https://www.youtube.com/watch?v=bh9ecsBsYqc&t=318s
https://www.youtube.com/watch?v=9TPz_mAyYWY&t=36s
mobs can make very small readjustements if the target moves out of their autoattack range, not necessarily walking all the way up to the target's bounding radius
https://www.youtube.com/watch?v=eqkZuxdngbI&t=489
charge takes the unit charging much closer to the edge of the hitbox than currently in vmangos (takes the unit all the way inside the other unit based on bounding radius)
https://youtu.be/jNngovFMofQ?si=eFcR9Xz8BGziIDTL&t=1184
Issues
How2Test
https://www.youtube.com/watch?v=Zx9l4yH2u-A
Todo / Checklist