Page MenuHomeWildfire Games

Fix linear splash damage
ClosedPublic

Authored by temple on Aug 3 2017, 6:13 PM.

Details

Summary

There is a bug with the splash damage of bolt shooters. If you attack from the east-west (1), things seem fine, the target is killed and the units behind him receive some splash damage. But if you attack from the north-south (2), oh sweet Jesus...

The problem is that the calculations assume a 2D vector, but the direction is a 3D vector with the y-coordinate corresponding to height. So instead of sending the north-south direction we send the vertical direction, which is essentially zero. When we attack from the north-south, the x-coordinate is also zero, so the parallel and perpendicular distances (dot and cross products) are almost zero. That means that every unit gets hit with splash damage, and gets hit with a multiplier close to 1.

The calculation is fixed in (3). (The bolt shooter has splash radius 8m; I included Scipio's 10m radius for reference. The units are in Testudo formation.)

Test Plan

Verify that the bug is fixed.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

temple created this revision.Aug 3 2017, 6:13 PM
temple edited the summary of this revision. (Show Details)Aug 3 2017, 6:16 PM
temple updated this revision to Diff 3003.Aug 3 2017, 8:00 PM

Moved dir outside the for loop.

elexis awarded a token.Aug 3 2017, 8:58 PM
elexis added a subscriber: elexis.Aug 3 2017, 10:26 PM

(Indeed I didn't came to the idea to read the prior unused, now used code for accuracy when reviewing D107 or even test multiple angles. Seems we either have to spend seemingly unproportional amounts of time or get bugs. rP19193 did it's intended job of revealing bugs in that linear splash damage code and the balancing hadn't been messed up after all. So despite the impact of the bug on a22 players, it was the right decision to commit it. Thanks. for the report.)

fatherbushido requested changes to this revision.Aug 4 2017, 3:31 PM

rP19193 did it's intended job of revealing bugs in that linear splash damage code

(As we said https://trac.wildfiregames.com/ticket/4328#comment:2)


@temple

  • Thanks for noticing that.
  • The mistake appears when javascript Vector objects were created (In first implementation of splash damage the 2D vector was created as an object with (x,z) coordinates). It's really sad that we don't get an error when trying to compute those product between a 2D vector and a 3D ones (there isn't any instance control of the argument).

Review:

  • What you suggest fix that computation problem.
  • But in fact, (I didn't look at your screenshots) it doesn't fix some other issues of that part of the code.

(I will continue my review later, let's discuss of the first comments before.)

binaries/data/mods/public/simulation/components/Damage.js
170

I don't know if it's better here (for vs if).
'direction' instead of 'dir' is perhaps a better name.
(We could - to check - also just parse the 2D one)

186

The previous comment was not so bad, but why not.

191

parallelDist > -width is not the condition to check
One should in fact check

parallelDist < data.radius

(and applying Math.abs() at L187)

193

To avoid implicit multiplication questions and perhaps simplify the expression, I would perhaps replace by

(1 - parallelDist * parallelDist / (data.radius * data.radius)) * (1 - perpDist * perpDist / (width * width));

This revision now requires changes to proceed.Aug 4 2017, 3:31 PM
temple added a comment.Aug 4 2017, 5:34 PM

Since we're here, I'll mention that I considered taking into account lateness and footprints, like ranged attacks do.
I did some tests attacking moving formations, but I couldn't really see damage being done to the wrong units. I guess the quadratic falloff hides it somewhat.
When attacking a civic center with a catapult, you'll hit it 100% of the time. But splash damage only cares about the center of the object, so the amount of the damage varies, which seems odd (since we hit the building 100% of the time). I guess splash damage is meant more for units than buildings? And again it's quadratic falloff so maybe it doesn't matter too much.

binaries/data/mods/public/simulation/components/Damage.js
170

I changed it because in vector.js I saw the comment "// Note that object creation is slow in JS, so use them only when necessary". We don't use dir with circular damage, but I thought it would be better to create it once every time rather than lots of times in the linear case. (Of course we could move the ifs outside the for loop, i.e. have two for loops, but that would repeat a lot of code.)

191

That's already checked when finding nearEnts.
What parallelDist > -width is saying is only damage units immediately in front of the target, but damage all of them behind him (up to the radius). You can see this in the screenshots (1) and (3).

fatherbushido added inline comments.Aug 4 2017, 5:43 PM
binaries/data/mods/public/simulation/components/Damage.js
170

I had understood.

191

Could you reconsider it (even if we choose to splash only the targets before or behind)?

fatherbushido added inline comments.Aug 4 2017, 5:58 PM
binaries/data/mods/public/simulation/components/Damage.js
191

(I am a bit stupid too:
parallelDist > -width could just be nuked
or replaced by parallelDist > 0 or parallelDist < 0 depending if you want to hit in front or behind - though iirc it was designed first to hit both zone).

temple added a comment.EditedAug 4 2017, 6:35 PM

Here is the current damage zone, which I'm fine with. (Bolt hitting the origin, moving left to right.)


What would you like it to be?

fatherbushido added a comment.EditedAug 4 2017, 10:39 PM
In D767#30160, @temple wrote:

Here is the current damage zone, which I'm fine with. (Bolt hitting the origin, moving left to right.)


What would you like it to be?

(you wasted time with your picture)
I wouldn't like anything, it first ought to be defined by parallelDist < splashRadius and perpDist < width.
(But comments were deleted and so on).
As we need a cutoff for the range query - and the current choice ie the same as for splash radius seems the good one - the first condition is useless (but I don't learn you anything here).
If you want to only keep one side, why not... But the current thing is not expected in any case.
One could also decide to use the width in templates instead of defining it from the radius but it's useless complication, I fine with first implementation.

Footprint (and lateness) are irrelevant (or useless complications) for that as there is no graphical stuff (with the projectile).

fatherbushido added inline comments.Aug 5 2017, 8:26 AM
binaries/data/mods/public/simulation/components/Damage.js
170

Don't forget to renomarlize.

temple added a comment.Aug 5 2017, 8:32 AM

I don't always understand what you're saying, fatherbushido.
My testing didn't work because I playing single player. Now I did a network game with Engine.SetTurnLength(1000) in the javascript console (I think the default is 500ms?), and lateness was definitely noticeable. Here's screenshots:


The missile hit the right target but the splash damage hit the wrong targets. So I think we need to add lateness to splash damage.
Catapults aren't often used against units, so maybe that's why this wasn't noticed before.

fatherbushido added a comment.EditedAug 5 2017, 8:51 AM
In D767#30172, @temple wrote:

So I think we need to add lateness to splash damage.

It's really not worth the complication imo (at least for now - that can be considered latter). We can discuss about that elsewhere if you want details.


(I will provide the end of the review this evening.)

temple updated this revision to Diff 3014.Aug 5 2017, 5:24 PM
temple edited edge metadata.
temple added a comment.Aug 5 2017, 5:29 PM

The direction is already normalized in the x-z plane. From Attack.js:

		let missileDirection = Vector3D.sub(realTargetPosition, selfPosition).div(realHorizDistance);

We can discuss about that elsewhere if you want details.

Yes, please.

In D767#30213, @temple wrote:

The direction is already normalized in the x-z plane. From Attack.js:

		let missileDirection = Vector3D.sub(realTargetPosition, selfPosition).div(realHorizDistance);

Ah what a nice joke :) We should consider after that patch to perhaps only parse the 2D one (or change the misleading js comments). Thx for noticing.

Then we should add that

. (see https://trac.wildfiregames.com/wiki/SubmittingPatches).
And also fix the current Damage test which should fail with your patch.

Thanks for the work/discussion.

fatherbushido added inline comments.Aug 5 2017, 10:03 PM
binaries/data/mods/public/simulation/components/Damage.js
191

I still would change that by nuking the first condition (or using the redundant one)

This is where I don't understand you. You say to nuke the first condition, parallelDist > -width, which would make the damage zone this:


But when I asked what you wanted the damage zone to be, you said you didn't want anything. (I prefer the current way.)

fatherbushido requested changes to this revision.EditedAug 6 2017, 7:56 AM
In D767#30244, @temple wrote:

This is where I don't understand you. You say to nuke the first condition, parallelDist > -width, which would make the damage zone this:
But when I asked what you wanted the damage zone to be, you said you didn't want anything. (I prefer the current way.)

I don't want anything. I will precise again:

  • I simply look at what was wanted (but some comments were deleted and...). I checked again (notice that I opened discussion): it is (it was designed) parallelDist >= 0 where parallelDist is the dot product (without abs - though Dist is confusing here). (My personal feeling was to take the zone before and after impact - but it's not what was intended and thinking again it's wrong).
  • Ideally I would use a larger range query to avoid the curved 'vertical' side but the approximation is good enough in the scope of that patch. (Using a larger query could allow more complex things too).
  • (One could also move the width in templates but it's imo useless complications for now).

So my last word is:
if (parallelDist > -width && perpDist < width) -> if (parallelDist >= 0 && perpDist < width)

This revision now requires changes to proceed.Aug 6 2017, 7:56 AM
temple updated this revision to Diff 3019.Aug 6 2017, 5:48 PM
temple edited edge metadata.

Added and fixed tests.

Thanks for the modifications :-)

binaries/data/mods/public/simulation/components/Damage.js
193

parallelPos > -width -> parallelPos >=0 and I commit it :-)

This comment was removed by fatherbushido.
temple added a comment.Aug 6 2017, 5:55 PM

I think you're abusing english. :)

If we change it to parallelDist >= 0, then half the time the unit that gets hit with the missile won't take any splash damage itself (since we don't take into account footprints). So I prefer to keep it as is. You can change it in commit, if you want.

I tried merging your tests with the damage tests, but it was ridiculous.

(Doing the math, the maximum damage on that curved vertical side turns out to be exactly 1%, so we aren't missing much.)
(Since we aren't looking much behind the missile, to find nearEnts we could use a smaller circle, about 60% of the radius, centered on the rectangle rather than the origin. I don't know enough about performance to say whether that's worth doing or not.)
(I agree length + width or something would be better than radius + radius/5. But it doesn't matter much to me so I never said anything.)

fatherbushido added a comment.EditedAug 6 2017, 10:00 PM
In D767#30266, @temple wrote:

I think you're abusing english. :)

I think that too.

If we change it to parallelDist >= 0, then half the time the unit that gets hit with the missile won't take any splash damage itself (since we don't take into account footprints).

I forgot to consider that. (You should have said it sooner :p)
In practice it will actaully be damage if we let a main damage (as it is - perhaps badly - currently done). But if one want to set 0 to the main ranged damage, it would perhaps be visually bad (in case of a big fleeing target mainly?).

So: to be thought. I am still not convinced by anything, I could let the use of the width as a workaround for now but I will add a commentary when commiting (as it's misdleading).

I tried merging your tests with the damage tests, but it was ridiculous.

Indeed. I will perhaps group both things in two separated functions in the same file. Even if there will be code duplication, we could keep the one file one component logic. Don't bother with that.

(Doing the math, the maximum damage on that curved vertical side turns out to be exactly 1%, so we aren't missing much.)
(Since we aren't looking much behind the missile, to find nearEnts we could use a smaller circle, about 60% of the radius, centered on the rectangle rather than the origin. I don't know enough about performance to say whether that's worth doing or not.)
(I agree length + width or something would be better than radius + radius/5. But it doesn't matter much to me so I never said anything.)

We agree.


Edit (Addendum):

  • The initial comment was " Check that the unit is within the distance splashWidth of the line starting at the missile's landing point which extends in the direction of the missile for length splashRadius". I think we can perhaps add it again.
  • Still don't know if we should bend to parallelDist >= 0 for now or not
fatherbushido added inline comments.Aug 8 2017, 8:55 AM
binaries/data/mods/public/simulation/components/Damage.js
175

I reconsidered interpolating following your remarks.
That's not a big cost and we'll just have to do that here, isn't it?
If you want to do that here, I am ok with that too. Else you can do that in another patch.

temple added a comment.Aug 8 2017, 4:24 PM
  • The initial comment was " Check that the unit is within the distance splashWidth of the line starting at the missile's landing point which extends in the direction of the missile for length splashRadius". I think we can perhaps add it again.

Sure.

  • Still don't know if we should bend to parallelDist >= 0 for now or not

I prefer not, but you've said this lots of times so just do it. :)

binaries/data/mods/public/simulation/components/Damage.js
175

It affects all splash damage so I think it's probably better in a separate patch.

fatherbushido accepted this revision.Aug 10 2017, 10:02 AM

Ok here we go.
Thanks for pointing out the issue, providing a patch and discussing about it.

This revision is now accepted and ready to land.Aug 10 2017, 10:02 AM
This revision was automatically updated to reflect the committed changes.