Opened 8 years ago
Closed 6 years ago
#4276 closed defect (fixed)
[PATCH] Attack and Damage bugs
Reported by: | fatherbushido | Owned by: | |
---|---|---|---|
Priority: | Should Have | Milestone: | Alpha 23 |
Component: | UI & Simulation | Keywords: | patch |
Cc: | leper | Patch: |
Description
In this meta ticket, some bugs related to Attack and Damage components are adressed.
Attachments (8)
Change History (48)
by , 8 years ago
Attachment: | 1910_nearbyUnits.diff added |
---|
by , 8 years ago
Attachment: | 1910_splash_strengths.diff added |
---|
comment:3 by , 8 years ago
Cc: | added |
---|
comment:4 by , 8 years ago
I quote leper comments on the other ticket:
First one looks good, that bug seems to have been there for ages, nice find. The second one is the result of me trying to fix something else, then not doing that and leaving that >in. But for actual splash damage the break should be removed too. I didn't test anything, so better ask someone else.
comment:5 by , 8 years ago
Thanks for your inputs! I don't know if we need to remove the break or not (the spash damage is yet done, here we look if we have hit something when we missed our target).
comment:6 by , 8 years ago
Yes, I believe the break should be removed, since all colliding units are supposed to be hit. If you do that, you can even use an early continue in that loop and save a level of indentation for the code :)
comment:7 by , 8 years ago
Yes, splash damage should hit all units in the radius, else eg some explosive shell would only damage the actual target and possibly a single other target in the hit radius. (Note that this bug seems to have been present for ages (possibly since splash damage was introduced).
You should only remove the projectile once, so you should likely use some boolean to indicate that (since no hit should leave the projectile around; though this might be strange in eg the above example).
comment:9 by , 8 years ago
Maybe add a newline after the continue;
but the style looks fine to me. :)
comment:10 by , 8 years ago
There is a 3rd point that I am not sure to understand.
When we call with the timer MissileHit
with the expected position of the target, there is some use of InterpolatedLocation
function wich take account of the lateness.
I don't understand if it's an interpolation backward in time or an expolation forward in time.
The InterpolatedLocation
function is currently done for the first case:
return new Vector3D( (curPos.x * (turnLength - lateness) + prevPos.x * lateness) / turnLength, 0, (curPos.z * (turnLength - lateness) + prevPos.z * lateness) / turnLength);
If it's the second case, then it should be (confirmed by Itms):
return new Vector3D( (curPos.x * (turnLength + lateness) - prevPos.x * lateness) / turnLength, 0, (curPos.z * (turnLength + lateness) - prevPos.z * lateness) / turnLength);
EDIT: In fact, I think I m wrong, InterpolatedLocation should indeed go backward in time. So all is fine.
comment:11 by , 8 years ago
I noticed that the algorithm we use for getting two independent random variables with a standard normal distribution uses cos and sin. There are two Box-Muller transform and I suggest to use the other one. I did some tests of that function. Calling 106 times the function, the total computation times is at least half for the second function.
Btw, perhaps should we move that function to globalscripts in Math ?
by , 8 years ago
Attachment: | 4276_boxmuller.diff added |
---|
comment:14 by , 8 years ago
Keywords: | review removed |
---|---|
Milestone: | Alpha 21 → Alpha 22 |
Priority: | Release Blocker → Should Have |
comment:15 by , 8 years ago
Erm, actually that code isn't related to splash damage at all, dunno why I thought that. And we only ever hit the actual target in case it is hit, so we should do the same in case we hit something else.
So if this change is to stay we should also try to hit multiple targets in case the actual target is hit. Basically remove the variable and add the break, but then again someone should verify that this actually makes sense.
comment:16 by , 8 years ago
we agree. so i will put the break again, as the ents are sorted by distance, i think it s logic to damage the first hit one.
comment:18 by , 8 years ago
Now all should be ok. I should confess I was confused when you spoke of splash stuff (I guess you thought of that in relation to the bug you fixed when it did a kind of splash damage when the target was missed). Thx for monitoring all that anyway!
comment:19 by , 7 years ago
Keywords: | review added |
---|
comment:25 by , 7 years ago
Another bug I will finish to invest is the weird trajectories bug (noticeable with catapult for example). That problem is due to an issue with the prepare time of attack. It's easily reproduceable with any unit by just setting a big prepare time: targetting a unit waiting some time (not the whole prepare time) then moving the target, even if the target is now out of range it will be targeted (with the different accuracy errors, approximation...) leading to weird visual effects.
Btw, I wonder if all prepare time in templates are well sync with actor animations. That need to be check too.
comment:26 by , 7 years ago
So the bug come mainly from the PerformAttack function. Indeed we perform the attack then we compute an expected position of the target (aiming) taking account of his movement and of the spread. But It could occur that the expected target pos is badly out of the attack range.
See http://trac.wildfiregames.com/attachment/ticket/4276/fireoutofrange.mkv
It's easy to fix, but what is the expected behavior ?
A) Cancelling the attack At L524 of Attack
if (realHorizDistance > elevationAdaptedMaxRange) return
B) hitting in that direction at the range limit ? C) something else ?
comment:27 by , 7 years ago
Another thing about wich I am a bit dubitative.
That part of Attack.CanAttack
// Check if the relative height difference is larger than the attack range // If the relative height is bigger, it means they will never be able to // reach each other, no matter how close they come. let heightDiff = Math.abs(cmpThisPosition.GetHeightOffset() - cmpTargetPosition.GetHeightOffset()); if (heightDiff > this.GetRange(type).max) continue;
Indeed in the whole cost the max range is an 'horizontal' range (I mean the length of the projection in the (x,z) plane) but not in that part of the code.
So imo it seems useless to add that cut off for ranged attack (at least when the attacker is heigher than the target).
by , 7 years ago
Attachment: | fireoutofrange.mkv added |
---|
comment:30 by , 7 years ago
Keywords: | review removed |
---|
Current version has been reviewed in D20. I put it of the review queue.
comment:32 by , 7 years ago
I'll now trying to improve the aiming when performing attack:
- perhaps checking if there aren't 2d/3d inconsistencies.
- computing the true or a more accurate expected target location (when assuming that the target move in the same way as it did in the previous turn).
comment:33 by , 7 years ago
Well in fact we don't gain many operations in not computing a better targetting. Perhaps we could also take care to have projectile speed (for skirmisher for example) a bit higher (at least to prevent having them slower than local speed of moving units).
comment:34 by , 7 years ago
I will change the approach of D20 (independantly of further unitAI change). The current approximation is really weird in some case so it's here that the fix must be: as when computing the exact solution we get at least the case (even if giving the closer expected location is a bit harder) where we know we can't reach the target, currently if we use a slow projectile speed compare to the - radial - unit speed, we can have a 'negative' (as it's not checked) timeToTarget and so a backward fire (we could even get an unlucky division by zero). So it's better imo to handle stuff before. Then do what we can and at last add the gaussian unaccuracy.
comment:35 by , 7 years ago
Another bug is a unitAI bug. In Attacking state, there is a range check done after entering in attacking state or after each attack after each timer tick (if in the timer part the unit is out of range then we chase him or some other stuff). But at the next tick the attack will be done even if while this time (it can be an huge time like 3,5s) the unit has moved and his perhaps out of range. It makes worse the D20 bugs and it is really weird for a melee attack.
elexis informed me of that issue for melee attack and provides the attached replay where a spearman cav hit a out of range unit. (The unit enter in ATTACKING state, check the range then call the timer with (preparetime = 0, repeattime) then the first attack is done, the range is check, the second attack is done but while the first and the second attack the unit has moved :p)
comment:37 by , 7 years ago
I recently notice that when we fire a mirage (for example archers targetting a cc), the attack is performed, projectile are launched but the damage are not dealt.
comment:39 by , 7 years ago
mirage bug: https://code.wildfiregames.com/D605
fail to look for other hit entities when the main target has position anymore: https://code.wildfiregames.com/D597
comment:41 by , 6 years ago
Milestone: | Work In Progress → Alpha 23 |
---|---|
Resolution: | → fixed |
Status: | new → closed |
It was mainly a personal remainder with random thoughts. No need to keep that.
First patch: 1910_splash_strengths.diff
We didn't parse the splash strengths to the damage component (we needed the Ranged and the Ranged.Splash stats in
MissileHit
). The above patch fixes that. We can also return a more complex object inGetAttackStrengths
and use this one.Second patch: 1910_nearbyUnits.diff
When we don't hit the target at the expected location, if we hit a nearby unit. It looks like we invert the test collision check. It is shown in #4241. The patch above fixes that.