Page MenuHomeWildfire Games

Move {civ}_*.xml → {civ}/*.xml in templates.
ClosedPublic

Authored by Freagarach on Aug 14 2020, 7:10 PM.

Details

Summary

This moves the templates from the civs to a folder called as the civ.
E.g.

  • units
    • athen
      • <the templates>
    • (...)

Open questions for a later diff:

  • What to do with units/
    • viking_*
    • theb(es)_*
    • thespian_*
    • noldor_*
    • samnite_*
    • merc
  • What to do with structures/
    • palisades_*

(FYI: 2218 files changed.)

Test Plan

Run P225 in the "public" folder and apply this patch on top.
Verify that:

  • Random maps load fine.
  • Checkrefs passes.
  • Playtest some maps.
  • Verify wall_demo loads.

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

Freagarach created this revision.Aug 14 2020, 7:10 PM
Owners added a subscriber: Restricted Owners Package.Aug 14 2020, 7:10 PM

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

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2985/display/redirect

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

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/1339/display/redirect

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/2438/display/redirect

Freagarach requested review of this revision.Aug 14 2020, 7:14 PM

Template folders after the script and this patch:

Units
athen
brit
cart
gaul
iber
kush
mace
maur
merc
noldor_ship_bireme.xml
pers
plane.xml
ptol
rome
samnite_skirmisher.xml
samnite_spearman.xml
samnite_swordsman.xml
sele
spart
thebes_sacred_band_hoplitai.xml
theb_siege_fireraiser.xml
thespian_melanochitones.xml
viking_longship.xml
Structures
athen
brit
cart
gaul
iber
kush
mace
maur
mausoleum.xml
merc
palisades_curve.xml
palisades_end.xml
palisades_fort.xml
palisades_gate.xml
palisades_long.xml
palisades_medium.xml
palisades_outpost.xml
palisades_short.xml
palisades_spike_angle.xml
palisades_spikes_small.xml
palisades_spikes_tall.xml
palisades_straight.xml
palisades_tower.xml
palisades_watchtower.xml
pers
ptol
rome
sele
spart
stonehenge.xml
wallset_palisade.xml
binaries/data/mods/public/maps/random/rmgen-common/wall_builder.js
30 ↗(On Diff #13190)

Needs some thinking.

Freagarach edited the test plan for this revision. (Show Details)Aug 14 2020, 7:25 PM

The script works, is, as far as I currently know, correct and complete.

In P225#1774, @Nescio wrote:

Also, merc (line 5) is not a civ. It's probably best to keep the non-civ (i.e. gaia) entities (e.g. palisades, plane) in the units/ and structures/ folders without moving them to subfolders, at least for now. What to do with them can be discussed later.

Sure, no problem.

Freagarach updated this revision to Diff 13191.Aug 14 2020, 8:13 PM

Fix mapgen.

Freagarach edited the summary of this revision. (Show Details)Aug 14 2020, 8:15 PM

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

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/1340/display/redirect

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

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2986/display/redirect

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/2439/display/redirect

Nescio requested changes to this revision.Aug 14 2020, 9:04 PM

Sure, no problem.

(Also, there are just three merc* units and only one merc* structure.)

Run P225 and apply this patch on top.

To make it work I had to insert a line at the beginning:

cd binaries/data/mods/public/

and another at the end:

cd ../../../../

I suppose the test plan meant to say to run it inside the public/ folder?

The script works, is, as far as I currently know, correct and complete.

Neither technology nor aura lines in templates should be altered:

ERROR: File 'simulation/data/auras/structures/cart/super_dock_repair.json' does not exist

ERROR: JavaScript error: globalscripts/Templates.js line 240 TypeError: aura is undefined GetTemplateDataHelper@globalscripts/Templates.js:240:5 getEntity@gui/reference/common/TemplateParser.js:39:16 draw@gui/reference/structree/Boxes/EntityBox.js:25:19 draw@gui/reference/structree/Boxes/StructureBox.js:17:3 draw@gui/reference/structree/Sections/Tree/TreeSection.js:35:1 selectCiv@gui/reference/structree/StructreePage.js:54:3 onSelectionChange@gui/reference/common/Dropdowns/CivSelectDropdown.js:26:4 CivSelectDropdown/this.civSelection.onSelectionChange@gui/reference/common/Dropdowns/CivSelectDropdown.js:18:47

ERROR: Errors executing script event "SelectionChange"

ERROR: File 'simulation/data/auras/structures/iber/monument.json' does not exist

ERROR: JavaScript error: globalscripts/Templates.js line 240 TypeError: aura is undefined GetTemplateDataHelper@globalscripts/Templates.js:240:5 getEntity@gui/reference/common/TemplateParser.js:39:16 draw@gui/reference/structree/Boxes/EntityBox.js:25:19 draw@gui/reference/structree/Boxes/StructureBox.js:17:3 draw@gui/reference/structree/Sections/Tree/TreeSection.js:35:1 selectCiv@gui/reference/structree/StructreePage.js:54:3 onSelectionChange@gui/reference/common/Dropdowns/CivSelectDropdown.js:26:4 CivSelectDropdown/this.civSelection.onSelectionChange@gui/reference/common/Dropdowns/CivSelectDropdown.js:18:47

ERROR: Errors executing script event "SelectionChange"

ERROR: File 'simulation/data/auras/structures/kush/pyramids_economic.json' does not exist

ERROR: File 'simulation/data/auras/structures/kush/pyramids_military.json' does not exist

ERROR: File 'simulation/data/auras/structures/kush/temple_amun.json' does not exist

ERROR: JavaScript error: globalscripts/Templates.js line 240 TypeError: aura is undefined GetTemplateDataHelper@globalscripts/Templates.js:240:5 getEntity@gui/reference/common/TemplateParser.js:39:16 draw@gui/reference/structree/Boxes/EntityBox.js:25:19 draw@gui/reference/structree/Boxes/StructureBox.js:17:3 draw@gui/reference/structree/Sections/Tree/TreeSection.js:35:1 selectCiv@gui/reference/structree/StructreePage.js:54:3 onSelectionChange@gui/reference/common/Dropdowns/CivSelectDropdown.js:26:4 CivSelectDropdown/this.civSelection.onSelectionChange@gui/reference/common/Dropdowns/CivSelectDropdown.js:18:47

ERROR: Errors executing script event "SelectionChange"

ERROR: File 'simulation/data/auras/structures/maur/pillar.json' does not exist

ERROR: JavaScript error: globalscripts/Templates.js line 240 TypeError: aura is undefined GetTemplateDataHelper@globalscripts/Templates.js:240:5 getEntity@gui/reference/common/TemplateParser.js:39:16 draw@gui/reference/structree/Boxes/EntityBox.js:25:19 draw@gui/reference/structree/Boxes/StructureBox.js:17:3 draw@gui/reference/structree/Sections/Tree/TreeSection.js:35:1 selectCiv@gui/reference/structree/StructreePage.js:54:3 onSelectionChange@gui/reference/common/Dropdowns/CivSelectDropdown.js:26:4 CivSelectDropdown/this.civSelection.onSelectionChange@gui/reference/common/Dropdowns/CivSelectDropdown.js:18:47

ERROR: Errors executing script event "SelectionChange"
binaries/data/mods/public/maps/random/rmgen-common/wall_builder.js
151 ↗(On Diff #13190)

Why the [1]?

binaries/data/mods/public/simulation/components/tests/test_UpgradeModification.js
145 ↗(On Diff #13190)

+ + ?

This revision now requires changes to proceed.Aug 14 2020, 9:04 PM
Freagarach edited the test plan for this revision. (Show Details)Aug 14 2020, 9:07 PM
Freagarach marked an inline comment as done.Aug 14 2020, 9:11 PM
Freagarach added inline comments.
binaries/data/mods/public/maps/random/rmgen-common/wall_builder.js
151 ↗(On Diff #13190)

Because it formerly split at the _-character giving the civ as the first (i.e. position 0) entry of the array, now it splits at / and the second element is needed. (See line 136.)

Nescio added inline comments.Aug 14 2020, 9:39 PM
binaries/data/mods/public/maps/random/rmgen-common/wall_builder.js
151 ↗(On Diff #13190)

Well, I don't quite understand, but I suppose you know what you're doing here.
I guess it has something to do with the changes you made in lines 26 to 31?

389 ↗(On Diff #13191)

This change seems wrong: it's supposed to be {civ}/wallset_stone, isn't it?

Freagarach marked an inline comment as done.Aug 15 2020, 7:23 AM
Freagarach added a subscriber: FeXoR.

Pinging @FeXoR over wall_builder.js.

Neither technology nor aura lines in templates should be altered:

Interesting that I didn't encounter that when playtesting ^^' (A closer inspection suggests techs need no change.)
Any suggestion how the auras can be handled in the script? To me it seems they have to be fixed manually. (One could, of course, put the aura files also in a civ-dir ^^ )

binaries/data/mods/public/maps/random/rmgen-common/wall_builder.js
389 ↗(On Diff #13191)

That is the filename indeed, but not the style. While I rarely touch rmgen it seems to me that the style is (sometimes, the file is not very consistent) a combination of the civ and something like "stone". Some other times style is merely "stone".

Palisades could go into a structures/common folder, at least until palisade models can be created on a per civ basis (if ever; largely a palisade is a palisade, not much culturally happening there, so I imagine the Palisades objects being common amonfg multiple civs for quite a while longer).

Interesting that I didn't encounter that when playtesting ^^' (A closer inspection suggests techs need no change.)
Any suggestion how the auras can be handled in the script? To me it seems they have to be fixed manually. (One could, of course, put the aura files also in a civ-dir ^^ )

There are not that many, just six in the public mod, so I suppose you could restore them manually. Luckily hero aura lines are unaffected, I guess because they have an additional folder step, e.g. units/heroes/athen_hero_pericles_1. However, this is something to be aware of when fixing other mods.

Palisades could go into a structures/common folder, at least until palisade models can be created on a per civ basis (if ever; largely a palisade is a palisade, not much culturally happening there, so I imagine the Palisades objects being common amonfg multiple civs for quite a while longer).

It's possible to introduce structures/gaia/ and units/gaia/ subfolders for all entities belonging to <Civ>gaia</Civ> (e.g. common, merc, other, shared, special are less suitable). For palisades, it's not impossible someone at some point decides to introduce {civ} palisade files, which are identical to and inherit from the current palisades, except that they have a different <Civ> and <SpecificName>.

binaries/data/mods/public/maps/random/rmgen-common/wall_builder.js
389 ↗(On Diff #13191)

Well, I can't say I fully understand this particular script, so it would certainly be helpful if someone more skilled could verify the file changes.

Freagarach updated this revision to Diff 13239.EditedAug 18 2020, 10:36 AM
Freagarach edited the summary of this revision. (Show Details)
  • Updated script.
  • Revert technologies.

Sorry about the missing context, not sure why it is gone,,,

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

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/3022/display/redirect

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/2469/display/redirect

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

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/1370/display/redirect

After running P225 and applying D2592, I tried starting a “Fortress” random map, which failed, here is the error:

ERROR: CCacheLoader failed to find archived or source file for: "simulation/templates/structures/iber_house.xml"

ERROR: Failed to load entity template 'structures/iber_house'

ERROR: Invalid template found for 'structures/iber_house'

ERROR: JavaScript error: maps/random/fortress.js line 112 TypeError: Engine.GetTemplate(...) is undefined @maps/random/fortress.js:112:6

ERROR: CMapGeneratorWorker::Run: Failed to load RMS 'maps/random/fortress.js'

Sorry about the missing context, not sure why it is gone,,,

Probably because the svn version has e.g. structures/cart_super_dock_repair, while this patch changes structures/cart/super_dock_repair to structures/cart_super_dock_repair, i.e. it's not compatible with the current trunk, one needs to run P225 first.

Freagarach planned changes to this revision.Aug 19 2020, 5:55 PM

After running P225 and applying D2592, I tried starting a “Fortress” random map, which failed, here is the error:

I *love* the hardcoding in maps,,,

Probably because the svn version has e.g. structures/cart_super_dock_repair, while this patch changes structures/cart/super_dock_repair to structures/cart_super_dock_repair, i.e. it's not compatible with the current trunk, one needs to run P225 first.

That I figured, but I meant the lack of context in e.g. Templates.js.

Freagarach updated this revision to Diff 13248.Aug 19 2020, 6:43 PM

Updated script and diff to include (some?) random map hardcoding.

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

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/3026/display/redirect

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/2473/display/redirect

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

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/1374/display/redirect

Freagarach edited the summary of this revision. (Show Details)Aug 20 2020, 9:18 AM
Freagarach marked an inline comment as done.Oct 8 2020, 8:04 PM

Why didn't we move the icons here again?

Because it's art?

@Stan can we move the portraits as well?
@Nescio anything left to do here?

Stan added a comment.Nov 13 2020, 12:05 PM

You can move the icons, if you want. If you plan to change all the template files at once though, I'd suggest two commits, or one commit per civ.

binaries/data/mods/public/simulation/templates/structures/maur/pillar_ashoka.xml
4 ↗(On Diff #13248)

Sorry if that has been answered before, but why did we move this?

anything left to do here?

The last time I checked there was an error with one or two maps. If that's fixed, I suppose I could try again.

You can move the icons, if you want. If you plan to change all the template files at once though, I'd suggest two commits, or one commit per civ.

The actual file moves are done by script (P225). I believe committing it in parts is actually more complicated than doing it all at once.

binaries/data/mods/public/simulation/templates/structures/maur/pillar_ashoka.xml
4 ↗(On Diff #13248)

The aura file is not actually moved. However, running P225 replaces the _ with a /, hence why this patch (D2952) restores it.

Stan added a comment.Nov 13 2020, 12:14 PM

The actual file moves are done by script (P225). I believe committing it in parts is actually more complicated than doing it all at once.

It might be yes. but I like to avoid commits like rP19095 (I still don't know all what that patch changed) when possible :) makes it easier to find problems in the future.

rP19095 is a very bad commit indeed, both because it includes various unrelated changes, and because its commit message and title do not reflect everything that's done.
By contrast, while moving all templates would result in a large commit, the change itself is simple and straightforward, and can be described in a single sentence. Spreading it out over several smaller commits probably means more work, not just to perform, but also to review or audit.
That is at least my understanding of it. @Freagarach might see things differently, of course.

Indeed. Although this commit wil be _huge_ it will actually encompass one change: moving templates from folder/{civcode}_* to folder/{civcode}/*. But nothing unrelated. E.g. renaming aura's to a similar schema is unrelated and can be done in a separate commit (if ever).

Nescio accepted this revision.EditedNov 13 2020, 1:26 PM

So I tried again, and did the following:

  • ran the newest version of P225 (uploaded 2020-11-13 at 08.04)
  • applied P2952
  • ran the checkrefs.pl --validate-templates script
  • had a look at the simulation/templates/structures/ and ../units/ folders
  • launched Atlas
  • launched 0 A.D. and viewed all civs in the structure tree
  • started a Fortress random map, then each of the five Trigger maps

I did not encounter any errors or warnings this time. Although I can't say I fully understand the map scripts, everything seems to work as it is supposed to. Moreover, the change as a whole is a great improvement: having a few dozen folders with a few dozen files each is much more manageable than one with hundreds of files.

@Stan can we move the portraits as well?

While I fully agree that should be done, I strongly recommend doing it in a separate patch.

This revision is now accepted and ready to land.Nov 13 2020, 1:26 PM

Given that it requires numerous patches to be rebased, I'd also suggested waiting with committing this for a bit, to allow some other revisions (e.g. D2995, D3018, D3098) to be committed before.

@Stan can we move the portraits as well?

While I fully agree that should be done, I strongly recommend doing it in a separate patch.

Moreover, I think icons ought to organized similarily to actors (see art/actors/units/), hence full civ names (e.g. britons, celts, gauls), instead of civ codes (e.g. brit, celt, gaul).

This revision was landed with ongoing or failed builds.Nov 19 2020, 11:51 AM
This revision was automatically updated to reflect the committed changes.
Owners added subscribers: Restricted Owners Package, Restricted Owners Package.Nov 19 2020, 11:51 AM