War Ship template inheritance
AbandonedPublic
Actions

Authored by elexis on May 13 2017, 6:49 PM.

Details

Reviewers: None

Summary

Rename the "Medium Warship" to "War Barge" since "War Ship" isn't a too common name and for consistency with the loading screentooltip as proposed by fatherbushido
Remove template duplication by introducing a new parent template for the "War Barge" inheriting the "Trireme" template:

The "Medium Warship" available to Mauryans, Gauls, Britons and Iberians inherits the trireme template.
But since it is the logically an own class, some values are duplicated to each of the "trireme" templates of that civ, f.e. Max Health, Max Garrison, WalkSpeed and Name.
It is easy to not keep them in sync, for example rP18402 removed an unexplained drastic mauryan ship health bonus from rP12007.

Remove template duplication by letting the "Trireme" template inherit the "Bireme" template.

Test Plan

Open the structure tree and see that every civ can build their trireme/war barge. Compare the parent templates for bireme, trireme and war barge and the 3 war barge templates and see that only duplication was removed.
If you're on unix, you can download the patch and execute the "splitpatch" command to get a diff of every file. So they can be compared on a character by character basis quickly with alt+tab or the like.

Diff Detail

Repository

rP 0 A.D. Public Repository

Branch

/ps/trunk

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 1702
Build 2691: Vulcan Build	Jenkins
Build 2690: arc lint + arc unit

Event Timeline

elexis created this revision.May 13 2017, 6:49 PM

Build is green

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

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

Harbormaster completed remote builds in B1702: Diff 1902.May 13 2017, 9:08 PM

Also removes the War Barge tooltip and lets it inherit the trireme one:

<Tooltip>Transport many soldiers across the sea.</Tooltip>
<Tooltip>Garrison units for transport and to increase firepower.</Tooltip>

and removes the melee class for the mauryan war bage from rP14648 / rP16874:

<VisibleClasses datatype="tokens">Melee</VisibleClasses>

Before committing I should add the war barge to the ProductionQueue list of cart_super_dock.xml and brit_crannog.xml.

(Last time I checked patches that applied svn mv didn't apply as intended)

perhaps
template_unit_mechanical_ship_warship
-> bireme
-> trireme
-> barge thing
Instead of that serial inheritance would be better?
(and quinqureme)

Inheriting war barge from trireme, trireme from bireme means it is harder to look at the final numbers but easier to guarantee that their values are greater consecutively. So changing the bireme means the other templates will be changed consecutively.
But it seems easier to read with only one parent template indeed and it might not be wanted to force the inheritance that way. We can continue this after the release if we want, got bigger fish to fry now.

May I suggest a different approach? Renaming the parent templates entirely, to be more logical?

template_unit_mechanical_ship_war
template_unit_mechanical_ship_war_fire
template_unit_mechanical_ship_war_light
template_unit_mechanical_ship_war_medium
template_unit_mechanical_ship_war_heavy

And just make the Celtic "War Barge" a slightly modified medium class ship and be done with it. IMHO nothing needs to be consistent with the wording of the loading tips, you should edit the loading tips to be consistent with the game. ;)

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?

In D490#21508, @wowgetoffyourcellphone wrote:

May I suggest a different approach? Renaming the parent templates entirely, to be more logical?

template_unit_mechanical_ship_war
template_unit_mechanical_ship_war_fire
template_unit_mechanical_ship_war_light
template_unit_mechanical_ship_war_medium
template_unit_mechanical_ship_war_heavy

And just make the Celtic "War Barge" a slightly modified medium class ship and be done with it. IMHO nothing needs to be consistent with the wording of the loading tips, you should edit the loading tips to be consistent with the game. ;)

It's indeed the cleanest solution.

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.

There is no reason why this shouldn't follow D507.

elexis abandoned this revision.Dec 12 2017, 8:41 PM

elexis mentioned this in D1760: Remove template_unit_mechanical.xml.Jan 24 2019, 6:14 PM