Page MenuHomeWildfire Games

Reset attack timer if repeat time has changed (Cleopatra aura bug)
Needs ReviewPublic

Authored by temple on Aug 21 2017, 2:20 AM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Trac Tickets
#4733
Summary

When units are attacking something and Cleo walks so that they come into her aura, their actual attack rate doesn't get updated even though the gui does. Similarly, if they start attacking while in her aura range and then she moves away, they'll continue to attack at the higher rate until they move or stop attacking.

This patch checks if the attack repeat rate has changed after every attack on value modification, and if so it resets the timer to the new rate.

Test Plan

I haven't messed with timers before so make sure the code is okay.
(You can set Cleo's multiplier to 0.2 for example to make it easier to see when it's in effect.)

We need to check BuildingAI for the siege tower. The timer is more complicated there. We have two options, to either add in a lastAttacked variable to figure out the exact timing, or just use some approximation.
We can't use the prepare time because the siege tower normally shoots continuously (even when moving) and this would introduce a 1.2s delay every time it entered or left Cleo's aura.
In this patch I used an offset of zero. But thinking about it more, I guess on average we're somewhere in the middle of the timer interval so maybe attackTimers.repeat / roundCount / 2 would be better.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

temple created this revision.Aug 21 2017, 2:20 AM
elexis added a subscriber: elexis.Aug 21 2017, 3:12 AM

Could this be abused or unintented side effects? Like moving in and out of aura range making that catapult shoot never or too often?

fatherbushido added a subscriber: fatherbushido.EditedAug 21 2017, 4:55 AM

I won't look at that, but hint:
listen to MT_ValueModification and ask unitAI to do something when msg.to == "Attack/"+type+/"RepeatTime"

(also it's a perhaps expected that it finish its order at the expected rate? So that's elexis question which is relevant.)

I won't look at that, but hint:
listen to MT_ValueModification and ask unitAI to do something when msg.to == "Attack/"+type+/"RepeatTime"

Thanks for the hint. I think I got something working, I'll update in the morning.

temple updated this revision to Diff 3267.Aug 21 2017, 9:24 PM
temple edited the summary of this revision. (Show Details)
temple edited the test plan for this revision. (Show Details)
In D818#32277, @elexis wrote:

Could this be abused or unintented side effects? Like moving in and out of aura range making that catapult shoot never or too often?

For the catapult, I don't think so, because we scale the interval. But for the siege tower (see the test plan), it's a possibility. I don't know if that's worth adding a lastAttacked variable (to keep track of the last timer time) or not.

A couple other things I remembered.
The first is that there may or may not be problems if ever prepare time was larger than repeat rate. But that's actually not the case for any unit. (Maybe it would mess up the animation too, I didn't test.)
The second is that healers are probably susceptible to the same bug. However, currently Cleopatra is the only aura/tech that modifies the repeat time, and she doesn't affect healers, so it's not an issue at the moment. It's probably the same fix so I think I should include it in this patch.

In D818#32474, @temple wrote:

if ever prepare time was larger than repeat rate ... Maybe it would mess up the animation

Would, refs D258

The second is that healers are probably susceptible to the same bug... same fix ... include it in this patch.

(At least it should be reported if not fixed in the same go)

temple updated this revision to Diff 3287.Aug 22 2017, 5:32 PM

Added the healing fix. (Add "Healer" and { "value": "Heal/Rate", "multiply": 0.2 } to Cleo's aura to test.)
Used half the repeat rate for the offset in BuildingAI. Note that here actually prepare time > repeat time, since it's 1.2s versus 2.0/10 = 0.2s. So if we tried to figure out the correct offset by storing the attack timers and last attacked time (every 0.2s), then scaling the interval, it might be more complicated. Well, let me figure out if it is more complicated or not...

temple updated this revision to Diff 3288.Aug 22 2017, 6:52 PM

Fixed test.

The important thing with BuildingAI is that the repeat time is updated to the correct value. So I'm okay with the estimated offset.

bb added a subscriber: bb.Aug 30 2017, 1:45 PM

It would be nice if we add an unitAI test for this in the test file. (din't look how hard that would be, maybe its just making a unit attack and then value-modify it and check its attackTimers). When we need an extra unitAI function for this, we might just leave the tests as is.

binaries/data/mods/public/simulation/components/BuildingAI.js
208

perhaps rather than offset name wasAttacking and use it as an optional bool with default false

binaries/data/mods/public/simulation/components/UnitAI.js
1949

(cost me some time but then considered correct)

2087

no need to check since we are attacking => ok

2092

cuz of round players could cheat with 0.5 ms => meh but it is no crime to use non integer values, is it? (as the value modification could change to that anyway)

4154

bbLint was warning, but then saw the lines above....

(that should come after buildingAI fix, but that's only my opinion)

temple added inline comments.Sep 4 2017, 5:42 AM
binaries/data/mods/public/simulation/components/UnitAI.js
1949

Yeah, maybe I should've added a comment there. For anyone else reading the code: It's possible for the attack rate to be modified before we make the first attack, i.e. during the prepare phase. The easiest way to handle that is to treat this as if we were in the repeat phase, by figuring out what would have been the lastAttacked time (although of course we didn't actually attack then). Then the math in the ValueModification section will be correct.

2092

I just saw things like xx.000001 and was worried maybe something bad could happen if some timer interval was 0.000001. Not sure exactly what that bad thing would be though, so I'll remove the rounding.

temple updated this revision to Diff 3477.Sep 4 2017, 5:53 AM

Added test.

temple marked an inline comment as done.Sep 4 2017, 5:54 AM