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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Core/Movement: Correct the allowed distance to target before a repositioning is necessary. #20173 #21193

Merged

Conversation

chaodhib
Copy link
Member

@chaodhib chaodhib commented Jan 7, 2018

Correct the allowed distance to target before a repositioning is necessary. #20173

Also getting rid of the wordserver config parameter 'TargetPosRecalculateRange' since it is no longer needed.

Changes proposed:

Let's say a creature needs to AA a non-moving unit:

  1. it moves closer to its target until it is in melee range.
  2. Once the chaser is close enough to its (non-moving until this point) target to attack it, it stops.
  3. Then, let's say the target moves away from the chaser, the chaser will not repositioning immediately: it will wait until the distance between itself and its target is beyond a certain threshold to start repositioning.

The waiting is an optimization, used in order to reduce the number of times the path is recomputed when the target is moving. The issue is as follow: the threshold's value was too high. The target could move away to a certain distance, outside of melee range and the mover will not reposition.

With this PR, this is now fixed. The threshold new value is correctly set to the melee attack range.

I'm also talking about the issue HERE and HERE.

Target branch(es):

  • 3.3.5
    master. Maybe?

Issues addressed: Closes #20173

Tests performed: Tested IG with a melee attacker (The Lich King in The Frozen Throne)

Known issues and TODO list:

  • Need to investigate to see if this change will be also valid for range attackers (like archer NPCs). EDIT: Done. There should be no impact.
  • This change might not be needed for MoveFollow (a unit following another unit). Maybe I will rollback the change for that type of movement since the new value incur a performance cost. EDIT: Done.

@ccrs
Copy link
Member

ccrs commented Jan 7, 2018

The interesting research here is to decide if the distance should be adjusted depending on the target situation.
Example, if its not in melee range, look for half the melee range distance to ensure the result.

Or even, make an algorithm to search for the most suitable point to move, that is, the point in which it should be attacking the target, not the one that is closer.

Many ideas come, yet regarding this PR, I think its a nice improvement to the current situation. Though take a look into what I said first, making the distance sort of dynamic depending on the situation.

@jackpoz
Copy link
Member

jackpoz commented Jan 7, 2018

Slightly unrelated to the PR: I was thinking a couple of days ago if we should use the result of MeleeAttacks/Spells about the target being out of range/out of LoS and handle it next update instead of proactively checking every now and then (100 ms) if the target is still in range/in LoS.

For now reducing the distance sounds good enough. Could you please change the "4.0f / 3.0f" magic numbers to something more self-explainatory ? even a #define keeping the same formula would be good, at first sight it's not clear what 4 and 3 mean in that context.

@chaodhib chaodhib force-pushed the bugfix-out_of_melee_range_chasing branch from b8caae8 to 648ed25 Compare January 7, 2018 14:03
@chaodhib
Copy link
Member Author

chaodhib commented Jan 7, 2018

I made a mistake in the new distance to check. It is fixed now. This guarantees that the target will always be in melee range.

@ccrs I'm not sure I understand what you mean in the first part.

@jackpoz Regarding the magic numbers, it is no longer need after latest changes. Regarding your first point, it could be an interesting optimization. The only downside that I see is that if the chaser and the target have the same speed, the target will be able to kite the mover by alternating between moving and stopping (thus getting in and out of range, making the attack miss time to attack). Which is a downside of the current implemenation as well.

@chaodhib chaodhib force-pushed the bugfix-out_of_melee_range_chasing branch 3 times, most recently from 6805c7a to 94cc016 Compare January 7, 2018 15:24
…tioning is necessary. TrinityCore#20173

Also getting rid of the wordserveur config parameter 'TargetPosRecalculateRange' since it is no longer needed.
@chaodhib chaodhib force-pushed the bugfix-out_of_melee_range_chasing branch from 94cc016 to 8c168a9 Compare January 7, 2018 15:36
@chaodhib
Copy link
Member Author

chaodhib commented Jan 7, 2018

All the remaining points are solved. Imo, it's ready to be merged. Will merge in a few days if no further comments.

@ccrs
Copy link
Member

ccrs commented Jan 7, 2018

What I meant is, whats better, moving to the point in which we exactly are in melee range, or move half the distance closer so the repositioning is less likely? That is, if the conditions are met, we detect a slope or obstacles or w/e.

But like I said, this is a nice improvement for current state.

@chaodhib
Copy link
Member Author

chaodhib commented Jan 7, 2018

@ccrs It's already the case: NPCs move at mover->CombatReach + target->CombatReach + 0.5y away from the target. meanwhile, the melee range is mover->CombatReach + target->CombatReach + 1.333y. So the target can move up to 0.8333 yards away from the mover before triggering a repositioning of the mover.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants