HomeWildfire Games

Rome Testudo & Anti-Cavalry formations animations:
AuditedrP22081

Description

Rome Testudo & Anti-Cavalry formations animations:

Reviewed by Stan

Details

Auditors
Silier
Committed
AlexandermbFeb 6 2019, 3:57 PM
Parents
rP22080: Fix a typo in last commit
Branches
Unknown
Tags
Unknown
Build Status
Buildable 6897
Build 11309: Post-Commit Build (macOS)Jenkins

Event Timeline

elexis added a subscriber: elexis.Feb 6 2019, 6:12 PM

Reviewed by Stan

If there's a forum link, would be useful to add it to the commit message.

/ps/trunk/binaries/data/mods/public/simulation/data/civs/rome.json
11

Was this intended? It's the only civ that used this song if Im not mistaken

Silier awarded a token.Feb 6 2019, 6:13 PM
Stan added a subscriber: Stan.Feb 6 2019, 7:49 PM
Stan added inline comments.
/ps/trunk/binaries/data/mods/public/simulation/data/civs/rome.json
11

Arf missed that. Should have run a diff.

Discussion was in a pm

fatherbushido added a subscriber: fatherbushido.EditedFeb 18 2019, 11:17 AM

I'd first salute the artwork.
So I hesitated to post something here and I wondered if I was the only one to notice some weird visual.
Is there something wrong in the animation (or in the code?), I have the anti_cavalry formation walking then sometimes putting knees on the ground and standing up doing some kind of squat.

edit: pinging the author @Alexandermb

I will assume I am not the only one to have test it so it's probably my computer which does something weird.

Stan added a comment.Feb 25 2019, 9:05 PM

@Alexandermb is working on quite some stuff right now so he probably missed it :) You know how fast things go around here :)

I'd first salute the artwork.
Is there something wrong in the animation (or in the code?), I have the anti_cavalry formation walking then sometimes putting knees on the ground and standing up doing some kind of squat.

edit: pinging the author @Alexandermb

The formation should remain with the animation when idle but due to code in formations they don't remain with the formation animation idle_variant for example see also the testudo wich should remain as a turtle or the macedonian Syntagma.

Formations code needs to replace the default_variant with formation_variant until the formation is disbanded.

fatherbushido added a comment.EditedFeb 25 2019, 9:48 PM

Thanks!
@Alexandermb: that's exactly what I wanted to know!

(again nice artwork!)

elexis raised a concern with this commit.Jun 22 2019, 1:46 PM
elexis added a subscriber: Nescio.

I wanted to see the squat, but I didn't test the repository of this commit, perhaps I missed 0ad dank meme material.
Testing with r22393 revealed some bugs, but they are probably not related to this commit (a segfault and units often not attacking anymore).

/ps/trunk/binaries/data/mods/public/simulation/templates/special/formations/anti_cavalry.xml
6

Concern:
For roman swordsmen the formation button is present but disabled, with the tooltip stating that one needs "16 melee infantry units", but the user may have selected 16 roman sweordsmen already that don't havet formation.

So "Requires 16 Spear Infantry Soldiers." or similar would be more precise.
The Wedge formation says "At least 6 cavalry units required.".
I suppose that misses capitalization, perhaps there is already a patch somewhere (Perhaps @Nescio knows?)

This commit now has outstanding concerns.Jun 22 2019, 1:46 PM
elexis added inline comments.Jun 22 2019, 1:48 PM
/ps/trunk/binaries/data/mods/public/simulation/templates/special/formations/anti_cavalry.xml
6

Actually you added the animation to swordsmen too, so the template is missing?

Nescio added inline comments.
/ps/trunk/binaries/data/mods/public/simulation/templates/special/formations/anti_cavalry.xml
6

Actually there is an old one waiting to be committed: D1614. However, this commit is later, so this line is not included there.
I suppose I could write a new patch standardizing formation tooltips, if someone is willing to review it. I haven't seen @Gallaecio in months.

Gallaecio added inline comments.Jun 24 2019, 9:29 AM
/ps/trunk/binaries/data/mods/public/simulation/templates/special/formations/anti_cavalry.xml
6

I’ll probably not resume 0 A.D. contributions before August, you might want to take that into account.

As noted on IRC, this triggers an infinite loop in JS (so contrary to what said above, the segfault is related), because of some unknown (so far) issue with attacking as formation (which was removed by D1220 otherwise).

I don't think we can sanely guarantee that attack-as-formation works right now, even if this particular case may be fixable - so it should probably be changed.

As noted on IRC, this triggers an infinite loop in JS (so contrary to what said above, the segfault is related), because of some unknown (so far) issue with attacking as formation (which was removed by D1220 otherwise).

I don't think we can sanely guarantee that attack-as-formation works right now, even if this particular case may be fixable - so it should probably be changed.

Ah, that explains why there was no formation which could attack as formation whilst testing D1971 ;)

Silier raised a concern with this commit.Jan 8 2020, 10:18 AM
Silier added a subscriber: Silier.
Silier added inline comments.
/ps/trunk/binaries/data/mods/public/simulation/templates/special/formations/testudo.xml
33
Stan added a subscriber: temple.Jan 8 2020, 10:24 AM
/ps/trunk/binaries/data/mods/public/simulation/templates/special/formations/testudo.xml
33
In D1360#55872, @temple wrote:

The reasoning is we shouldn't have these (unadvertised) bonuses/penalties until we have also implemented trade-offs like reduced/increased armor. (See D1218.)

Silier accepted this commit.Jan 8 2020, 6:08 PM

Speed multiplication removed in rP23345.

Freagarach added inline comments.Mar 17 2020, 9:29 PM
/ps/trunk/binaries/data/mods/public/simulation/templates/special/formations/anti_cavalry.xml
6

(See also rP23450.)

Stan removed an auditor: elexis.Jan 10 2021, 7:28 PM
All concerns with this commit have now been addressed.Jan 10 2021, 7:28 PM