Opened 6 years ago

Closed 5 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)

1910_nearbyUnits.diff (1.1 KB ) - added by fatherbushido 6 years ago.
1910_splash_strengths.diff (1.9 KB ) - added by fatherbushido 6 years ago.
1910_nearbyUnits.2.diff (1.4 KB ) - added by fatherbushido 6 years ago.
adress Itms advice
1910_nearbyUnits.3.diff (1.5 KB ) - added by fatherbushido 6 years ago.
style can perhaps be improved
4276_boxmuller.diff (1.2 KB ) - added by fatherbushido 6 years ago.
4276_uselessvar.diff (1.6 KB ) - added by fatherbushido 6 years ago.
removes a useless variable and fix a typo
fireoutofrange.mkv (849.2 KB ) - added by fatherbushido 6 years ago.
unitai_bug.txt (7.0 KB ) - added by fatherbushido 6 years ago.
elexis replay

Download all attachments as: .zip

Change History (48)

by fatherbushido, 6 years ago

Attachment: 1910_nearbyUnits.diff added

by fatherbushido, 6 years ago

Attachment: 1910_splash_strengths.diff added

comment:1 by fatherbushido, 6 years ago

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.

Last edited 6 years ago by fatherbushido (previous) (diff)

comment:3 by fatherbushido, 6 years ago

Cc: leper added

comment:4 by fatherbushido, 6 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 fatherbushido, 6 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).

Last edited 6 years ago by fatherbushido (previous) (diff)

comment:6 by Itms, 6 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 :)

by fatherbushido, 6 years ago

Attachment: 1910_nearbyUnits.2.diff added

adress Itms advice

comment:7 by leper, 6 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).

by fatherbushido, 6 years ago

Attachment: 1910_nearbyUnits.3.diff added

style can perhaps be improved

comment:8 by fatherbushido, 6 years ago

Thanks! I completely missed that.

comment:9 by Itms, 6 years ago

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

comment:10 by fatherbushido, 6 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.

Last edited 6 years ago by fatherbushido (previous) (diff)

comment:11 by fatherbushido, 6 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 fatherbushido, 6 years ago

Attachment: 4276_boxmuller.diff added

comment:12 by fatherbushido, 6 years ago

In 18827:

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

comment:13 by fatherbushido, 6 years ago

In 18828:

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

comment:14 by fatherbushido, 6 years ago

Keywords: review removed
Milestone: Alpha 21Alpha 22
Priority: Release BlockerShould Have

comment:15 by leper, 6 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 fatherbushido, 6 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.

Last edited 6 years ago by fatherbushido (previous) (diff)

comment:17 by fatherbushido, 6 years ago

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.

comment:18 by fatherbushido, 6 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 fatherbushido, 6 years ago

Keywords: review added

by fatherbushido, 6 years ago

Attachment: 4276_uselessvar.diff added

removes a useless variable and fix a typo

comment:20 by elexis, 6 years ago

<review/> for that last patch

comment:21 by elexis, 6 years ago

In 18929:

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

comment:22 by elexis, 6 years ago

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

comment:23 by Stan`, 6 years ago

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

comment:24 by fatherbushido, 6 years ago

(4276_boxmuller.diff: missing space L455)

comment:25 by fatherbushido, 6 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.

Last edited 6 years ago by fatherbushido (previous) (diff)

comment:26 by fatherbushido, 6 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

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 ?

Last edited 6 years ago by fatherbushido (previous) (diff)

comment:27 by fatherbushido, 6 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).

Last edited 6 years ago by fatherbushido (previous) (diff)

comment:28 by elexis, 6 years ago

Milestone: Alpha 22Work In Progress

Moving to the new WIP milestone.

by fatherbushido, 6 years ago

Attachment: fireoutofrange.mkv added

comment:30 by fatherbushido, 6 years ago

Keywords: review removed

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

comment:31 by fatherbushido, 6 years ago

refs r19136

comment:32 by fatherbushido, 6 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 fatherbushido, 6 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 fatherbushido, 6 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 fatherbushido, 6 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)

by fatherbushido, 6 years ago

Attachment: unitai_bug.txt added

elexis replay

comment:36 by fatherbushido, 6 years ago

refs r19366

comment:37 by fatherbushido, 6 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:38 by fatherbushido, 6 years ago

refs r19432

comment:39 by fatherbushido, 6 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

Last edited 6 years ago by fatherbushido (previous) (diff)

comment:40 by fatherbushido, 6 years ago

(Refs r19791 for the mirage issue)

comment:41 by fatherbushido, 5 years ago

Milestone: Work In ProgressAlpha 23
Resolution: fixed
Status: newclosed

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

Note: See TracTickets for help on using tickets.