[PATCH] Attack and Damage bugs

[fatherbushido]

In this meta ticket, some bugs related to Attack and Damage components are adressed.

[fatherbushido]

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 in GetAttackStrengths 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.

[fatherbushido]

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.

[fatherbushido]

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).

[Itms]

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 :)

[fatherbushido]

adress Itms advice

[leper]

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).

[fatherbushido]

style can perhaps be improved

[fatherbushido]

Thanks! I completely missed that.

[Itms]

Maybe add a newline after the continue; but the style looks fine to me. :)

[fatherbushido]

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.

[fatherbushido]

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 10⁶ 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 ?

[fatherbushido]

In 18827:

Fix an issue with splash damage. The ranged attack strengths were parsed instead of the splash strengths. Refs #4276.

[fatherbushido]

In 18828:

Fix an issue with ranged attack: wrong units were damaged. Refs #4276. Commented by leper and Itms.

[leper]

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.

[fatherbushido]

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.

[fatherbushido]

In 18829:

Fix r18828: damage only one unit when the main target is missed by a projectile and others units are reached, to be coherent when the main target is hit. Commented by leper. Refs #4276.

[fatherbushido]

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!

[fatherbushido]

removes a useless variable and fix a typo

[elexis]

<review/> for that last patch

[elexis]

In 18929:

Remove two pointless references and fix a typo. Patch by fatherbushido, refs #4276.

[elexis]

Guess the review keyword is for attachment:4276_boxmuller.diff​​

[Stan]

Is there anyway to have more explicit variable names than a b and s ?

[fatherbushido]

(4276_boxmuller.diff: missing space L455)

[fatherbushido]

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 repeat time, prepare time in templates are well sync with actor animations. That need to be check too.

[fatherbushido]

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

For example, with a catapult, and a fleeingn cav (fleeing radially), we can have that:

WARNING: selfPosition({x:617.5589752197266, y:20, z:606.6230926513672}) WARNING: targetPosition({x:550.9756622314453, y:19.999374389648438, z:676.3333740234375}) WARNING: previousTargetPosition({x:553.6228485107422, y:19.999374389648438, z:674.0438385009766}) WARNING: elevationAdaptedMaxRange80.03369140625 WARNING: rand[0.607085014306797, 0.6238512424861701] WARNING: realHorizDistance255.58346950339396

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 ?

[fatherbushido]

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).

[elexis]

Moving to the new WIP milestone.

[fatherbushido]

​https://code.wildfiregames.com/D20

[fatherbushido]

Current version has been reviewed in D20. I put it of the review queue.

[fatherbushido]

refs r19136

[fatherbushido]

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).

[fatherbushido]

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).

[fatherbushido]

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.

[fatherbushido]

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)

[fatherbushido]

elexis replay

[fatherbushido]

refs r19366

[fatherbushido]

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.

[fatherbushido]

refs r19432

[fatherbushido]

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

[fatherbushido]

(Refs r19791 for the mirage issue)

[fatherbushido]

It was mainly a personal remainder with random thoughts. No need to keep that.