Page MenuHomeWildfire Games

Fix prepare time across templates
ClosedPublic

Authored by wraitii on Mar 24 2017, 7:18 PM.

Details

Summary

Prepare time is a bit of a misunderstood feature.
As of rP14449, any attack that is started (either melee or ranged) will fire at least once, even if in between the start of the attack and the effective firing moment, the unit has moved out of range. This was to enable chasing to actually do something.

Thus, the prepare time is just a feature to sync the animation and the simulation's "firing point". The first attack happens after prepare-time, and the animation is adapted (we skip to a different part).
It turns out that for some reason, most of our ranged unit have a prepare time equal to our repeat time. This makes them look weird and annoying, since they often fire 1+ second after the order has been given, which is a really, really long time, and it also makes them animate oddly (just try it, it's hard to describe).
The following patch change that to about half the repeat time (which fits more or less well with the actor "event"), which looks more natural and is less annoying. It could be lowered still, give me your feedback.

It also turns that melee units do not have a prepare time, it's effectively 0. This has a small bug, though: when entering "Combat.attacking", we switch immediately to the attack animation, and since prepare time is 0, we switch to exactly the "event", i.e. the moment the attack is at its fullest. BUT, since the first "perform attack" happens in Timer, we have a delay of at least one turn. This isn't too obvious in SP, but in MP it's really egregious: the damage is obviously dealt at the wrong time.

The easiest fix for this is to add a prepare time for melee attacks, which this patch does.

Since rP12542, which seems to work, we have a check against attacks repeating too often, so there should be no issues here.

Test Plan

Fire up a game and report on prepare time and animations and feel.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

wraitii created this revision.Mar 24 2017, 7:18 PM
Owners added a subscriber: Restricted Owners Package.Mar 24 2017, 7:18 PM
wraitii added reviewers: Restricted Owners Package, Restricted Owners Package.Mar 24 2017, 7:19 PM
Vulcan added a subscriber: Vulcan.Mar 24 2017, 8:03 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/590/ for more details.

First in game tests show an improvement.
gaia/fauna_bear.xml
gaia/fauna_boar.xml
gaia/fauna_crocodile.xml
gaia/fauna_lion.xml
gaia/fauna_lioness.xml
gaia/fauna_rhino.xml
gaia/fauna_tiger.xml
gaia/fauna_walrus.xml
gaia/fauna_wolf_snow.xml
gaia/fauna_wolf.xml
are perhaps missing (even if we get the things by inheritance)
and some others
infantry_archer
hero_cavalry_archer

fatherbushido requested changes to this revision.Mar 28 2017, 11:46 AM
This revision now requires changes to proceed.Mar 28 2017, 11:46 AM
wraitii updated this revision to Diff 1297.Apr 17 2017, 9:45 AM
wraitii edited edge metadata.

Updated to make sure all relevant templates have a correct prepare time. The inheritance for animals is fine imo. Validated using D302.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/794/ for more details.

I didn't test all units.

wraitii removed reviewers: Restricted Owners Package, Restricted Owners Package.Apr 18 2017, 4:42 PM
This revision is now accepted and ready to land.Apr 18 2017, 4:42 PM
This revision was automatically updated to reflect the committed changes.