Page MenuHomeWildfire Games

[gameplay] add visible garrison points to rome siege wall tower
Needs ReviewPublic

Authored by Nescio on Sep 4 2020, 9:26 PM.

Details

Reviewers
Freagarach
Group Reviewers
Balancing
Summary

This patch is split off from D2769 and a follow-up to D2760/rP24020 and D2986/rP24021. It gives rome siege wall towers four visible garrison slots (i.e. turret points), instead of the attack with up to two arrows other wall towers have. That rome siege wall towers are treated differently from those of normal city walls is justifiable, since they can be built in neutral and enemy territory and be used offensively.
How it looks in game:


[EDIT] Also gives all siege wall segments the same cost-to-health ratio.

Test Plan

Check for mistakes and omissions. Agree with the proposed changes.

Unit TestsFailed

TimeTest
0 msJenkins > cxxtest_debug.xml::[failed-to-read]
Failed to read test report file E:\Jenkins\workspace\vs2015-differential\cxxtest_debug.xml org.dom4j.DocumentException: Error on line 346 of document : Content is not allowed in trailing section. at org.dom4j.io.SAXReader.read(SAXReader.java:511)
0 msJenkins > TestAllocators::Debug Build & Tests / test_da
0 msJenkins > TestAllocators::Release Build & Tests / test_da
0 msJenkins > TestAllocators::test_da
0 msJenkins > TestAllocators::test_da
View Full Test Results (1 Failed · 1,694 Passed)

Event Timeline

Nescio created this revision.Sep 4 2020, 9:26 PM
Owners added subscribers: Restricted Owners Package, Restricted Owners Package.Sep 4 2020, 9:26 PM
Nescio requested review of this revision.Sep 4 2020, 9:33 PM
Nescio updated this revision to Diff 13408.Sep 4 2020, 9:44 PM
Nescio edited the summary of this revision. (Show Details)
  • tweak costs
Freagarach added inline comments.Sep 5 2020, 9:20 AM
binaries/data/mods/public/simulation/templates/structures/rome_siege_wall_tower.xml
4 ↗(On Diff #13408)

This (BuildingAI) is not needed at all, right?

Nescio added inline comments.Sep 5 2020, 10:09 AM
binaries/data/mods/public/simulation/templates/structures/rome_siege_wall_tower.xml
4 ↗(On Diff #13408)

Well, I'm not too sure; template_structure.xml has it, and it's not disabled in brit_wall_tower.xml or gaul_wall_tower.xml either.

Freagarach added inline comments.Sep 9 2020, 6:04 PM
binaries/data/mods/public/simulation/templates/structures/rome_siege_wall_tower.xml
4 ↗(On Diff #13408)

It is only used for structures that fire projectiles.

Nescio updated this revision to Diff 13466.Sep 10 2020, 11:21 AM
Nescio edited the summary of this revision. (Show Details)
  • clean up <BuildingAI> to define it only in structure templates with an <Attack>, per @Freagarach

@Stan opinion on the flag removal? (I like the flags, but I'm no artist.)

@Stan opinion on the flag removal? (I like the flags, but I'm no artist.)

(Other siege wall actors don't have garrison flags either.)

Stan added a comment.Sep 10 2020, 12:15 PM

(That's a tower)

The top of the tower looks very crowded though. I assume it's not possible to make it

2
2
-

And keep the flag?

The garrison flag is not terribly important and could be kept, if you think that's better.
However, I believe the purpose of the garrison flag is to indicate an entity has a garrison inside. Actors of structures with visible garrison slots (e.g. long wall segments) don't have garrison flags, presumably because, well, their garrison is visible. Since this patch makes the garrsion visible, there no longer is a need for the garrison flag, and it's consistent to remove them.
Then there is also the fact that the emblem on the flag is the actor's civilization, not the owner's, but that's a separate issue.

it's consistent to remove them.

True that.

Anything that needs changing?

I guess we need only some opinions from the balancing department.
Unless this:

In D2993#131488, @Stan wrote:

The top of the tower looks very crowded though.

is sufficient reason to hold this off?

(I'd vote for both levels with four turretpoints. Or three if four looks too crowded.)

To be able to use the lower floor (also for siege wall gate) the actor needs to be edited to raise the platform, because currently the garrison heads go through the roof: https://wildfiregames.com/forum/topic/22779-0abc-mod/page/17/?tab=comments#comment-391719

Could you please split these off?

[EDIT] Also gives all siege wall segments the same cost-to-health ratio.
And clean up <BuildingAI> to define it only in structure templates with an <Attack>.

At least the latter, for I can easily review and commit that whilst this patch and the former need some opionion about balancing.

Also gives all siege wall segments the same cost-to-health ratio.

This is intimately linked with this patch. Currently wall towers are disproportionally more expensive than other wall segments because they have an <Attack>. If that's removed, then their cost ought to be proportional.

And clean up <BuildingAI> to define it only in structure templates with an <Attack>.

Done in D3016.

Nescio updated this revision to Diff 13538.Sep 25 2020, 1:15 PM
Nescio edited the summary of this revision. (Show Details)
  • updated because of D3016
Nescio updated this revision to Diff 14066.Thu, Nov 19, 1:59 PM
  • rebased