Page MenuHomeWildfire Games

Remove template_unit_mechanical.xml
ClosedPublic

Authored by wraitii on Jan 24 2019, 3:57 PM.

Details

Summary

Currently the template tree includes a template_unit_mechanical.xml, which has only two children, *_ship.xml and *_siege.xml. However:

  • it does very little
  • it implies ships and siege engines are closer related than, say, cavalry and infantry
  • there are no *_organic_* or *_soldier_* templates either

Therefore it makes sense to simplify the templates a bit by deprecating the mechanical one (similar to D1734).

This patch:

  • deletes template_unit_mechanical.xml and inserts its relevant values into its descendants
  • renames template_unit_mechanical_ship* to template_unit_ship*
  • renames template_unit_mechanical_siege.xml to template_unit_siege.xml
    • renames template_unit_mechanical_siege_ballista.xml to template_unit_siege_boltshooter.xml
    • renames template_unit_mechanical_siege_onager.xml to template_unit_siege_stonethrower.xml
    • renames template_unit_mechanical_siege_ram.xml to template_unit_siege_ram.xml
    • renames template_unit_mechanical_siege_tower.xml to template_unit_siege_tower.xml
  • adjusts all entity parent paths

The ballista→boltshooter and onager→stonethrower moves are to avoid unnecessary misunderstandings: ballist(r)a, catapult, and onager are words that could be applied to various torsion engines, both dart and rock weapons. E.g. 0 A.D.'s cart and rome “ballistas” have the “onager”, not the “ballista”, as their parent. Contrary to the current AoE terminology, “boltshooter” and “stonethrower” make immediately clear what kind of artillery these templates represent.

Test Plan

Probably unproblematic, but check nevertheless.

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

Nescio created this revision.Jan 24 2019, 3:57 PM
Vulcan added a subscriber: Vulcan.Jan 24 2019, 3:59 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/995/

elexis added a subscriber: elexis.Jan 24 2019, 6:14 PM

(refs D490 for ships)

From the D490 discussion:

Relatedly, the siege templates could use some renaming too. The "Ballista" parent template is a bolt shooter, but ballistae in the ancient period were rock throwers!!! This naming convention I think sounds like a holdover ffrom the AOE2 mod days. So:

template_unit_mechanical_siege_boltshooter
template_unit_mechanical_siege_stonethrower

Is more logical to me. Thoughts on this guy?

Yes. It's planned.

And done in this patch.

wraitii added a subscriber: wraitii.

Will ensure this is complete and commit

wraitii commandeered this revision.Sat, Apr 20, 11:02 AM
wraitii edited reviewers, added: Nescio; removed: wraitii.

Commandeering as I couldn't apply the patch quite neatly on git and I had some amendments to make (namely the maps/ files were missed)

wraitii updated this revision to Diff 7784.Sat, Apr 20, 11:09 AM

Otherwise I believe this is complete and neat-o, so I still intend to commit this forthcomingly. Might be tricky to get the renames right on svn annoyingly...

Git branch: https://github.com/wraitii/0ad/tree/D1760_mechanical

Owners added a subscriber: Restricted Owners Package.Sat, Apr 20, 11:09 AM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/differential/1240/display/redirect

@Nescio Feel free to commandeer back, I'll commit it mentioning you regardless.

wraitii added a subscriber: Itms.EditedSat, Apr 20, 11:25 AM

@Itms Weird build error... Patching seems to have failed on files with a space in the name.

Itms added a comment.Sat, Apr 20, 11:37 AM

I think the patch doesn't apply because of the spaces in file names. Nescio is working on a fix for the latter (D1042), if you have some time to steal the review from me that would be a perfect "thank you" to Nescio for reviewing this one 😉

Thank you for commandeering and updating this patch, I appreciate it!
It looks good here on phabricator, however, when I try implementing it (arc patch D1760), there are some warnings and it doesn't succeed properly, so I can't check for completeness or mistakes, unfortunately.

This revision was not accepted when it landed; it landed in state Needs Review.Sun, Apr 21, 7:04 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.