Page MenuHomeWildfire Games

-ConquestCritical clear-up

Authored by Nescio on Sep 13 2018, 5:23 PM.



Currently the "ConquestCritical" class is added to the basic template_structure.xml file and subsequently removed in the majority of cases. This is quite ugly. A much neater way is to add it only to the cases that actually have it (Civic, Military, Wonder, and several special structures), but not to the basic parent template.

Test Plan

Effectively nothing ought to be changed.

Diff Detail

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

Event Timeline

Nescio created this revision.Sep 13 2018, 5:23 PM
Vulcan added a subscriber: Vulcan.Sep 13 2018, 5:29 PM

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

Link to build:

bb added a subscriber: bb.Dec 26 2018, 5:42 PM

15 removed cases, 10 added ones (if counting holds, the 15 goes down to 13 given the structure_defense_* comment)

Not convinced either way about this patch

46 ↗(On Diff #6885)

With the current design the structure_defense_* could be moved into the parent

Currently template_structure.xml adds ConquestCritical, but:

  • template_structure_defense_*.xml
  • template_structure_economic.xml
  • template_structure_resource.xml
  • template_structure_special_*.xml

remove it. Instead of adding it everywhere and subsequently removing it often, I think it's neater to simply add it only to the cases that actually use it:

  • template_structure_civic.xml
  • template_structure_military.xml
  • template_structure_wonder.xml
  • several structures/ files.
bb accepted this revision.Apr 12 2019, 5:04 PM

Reading it again, the actual question is how to treat special (the rest looks good)

in special 10 removed, 10 added, so I guess if someone adds a structure one is as likely to add a conquestcritical one or not, so @coin and take the positive way.

One could argue: if in doubt it should be conquestcritical, so rather add to much than to less, but seeing the number of benches/fenches/tables etc. that are conquestcritical by this (those tags are remove by this patch) , I don't think that argument really holds.

However rome_arch, pers_tacara and pers_ishtar_gate should have the conquestcritical tag imo

(defense template got renamed so rebasing that)

This revision is now accepted and ready to land.Apr 12 2019, 5:04 PM
This revision was automatically updated to reflect the committed changes.