Differential D1031
Workshop templates reorganization Authored by mimo on Nov 13 2017, 5:57 PM.
Details
Unify the old (mace) and new workshop templates in a military_workshop one (no need to inherit from barracks). As introduced by Lordgood, i've kept the possibility to garrison siege units in workshop, but limit its number to 2. And not done in this patch, we could also add an Aura to repair garrisoned siege (as done for ships in shipyard). Check that everything works, and templates validate.
Diff Detail
Event TimelineComment Actions Executing section Default... Executing section Source... Executing section JS... binaries/data/mods/public/simulation/ai/petra/headquarters.js | 399| » » » let·ratioMax·=·0.70·+·randFloat(0.,·0.1); | | [NORMAL] JSHintBear: | | A trailing decimal point can be confused with a dot: '0.'. binaries/data/mods/public/simulation/ai/petra/headquarters.js | 404| » » » » ratioMax·=·0.85·+·randFloat(0.,·0.1); | | [NORMAL] JSHintBear: | | A trailing decimal point can be confused with a dot: '0.'. Comment Actions patch reuploaded without the two removing of files which will have to be done when commiting, as this seems to be the origin of the build failure (mace_siege_workshop and military_barracks_workshop) Comment Actions Executing section Default... Executing section Source... Executing section JS... binaries/data/mods/public/simulation/ai/petra/headquarters.js | 399| » » » let·ratioMax·=·0.70·+·randFloat(0.,·0.1); | | [NORMAL] JSHintBear: | | A trailing decimal point can be confused with a dot: '0.'. binaries/data/mods/public/simulation/ai/petra/headquarters.js | 404| » » » » ratioMax·=·0.85·+·randFloat(0.,·0.1); | | [NORMAL] JSHintBear: | | A trailing decimal point can be confused with a dot: '0.'. Comment Actions Try again with the stupid phabricator, now removing the map file because of blanks in the name. Comment Actions Thanks for removing the dupes! I didn't review every detail nor tested for completeness (the XML map files should be checked, there is no random map with them), but I definitely agree with and accept with the approach.
Comment Actions Successful build - Chance fights ever on the side of the prudent. Updating workspaces... Build (release)... Build (debug)... Running release tests... Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK! Checking XML files... Relax-NG validity error : Extra element props in interleave /public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element variant failed to validate content /public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element group has extra content: variant Relax-NG validity error : Extra element group in interleave /public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element actor failed to validate content Comment Actions Executing section Default... Executing section Source... Executing section JS... binaries/data/mods/public/simulation/ai/petra/headquarters.js | 399| » » » let·ratioMax·=·0.70·+·randFloat(0.,·0.1); | | [NORMAL] JSHintBear: | | A trailing decimal point can be confused with a dot: '0.'. binaries/data/mods/public/simulation/ai/petra/headquarters.js | 404| » » » » ratioMax·=·0.85·+·randFloat(0.,·0.1); | | [NORMAL] JSHintBear: | | A trailing decimal point can be confused with a dot: '0.'. Comment Actions Update following elexis comments. The only map with workshop is the mace demo one, which should be updated when committing because of phabricator issues Comment Actions Successful build - Chance fights ever on the side of the prudent. Updating workspaces... Build (release)... Build (debug)... Running release tests... Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK! Checking XML files... Relax-NG validity error : Extra element props in interleave /public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element variant failed to validate content /public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element group has extra content: variant Relax-NG validity error : Extra element group in interleave /public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element actor failed to validate content Comment Actions Executing section Default... Executing section Source... Executing section JS... binaries/data/mods/public/simulation/ai/petra/headquarters.js | 399| » » » let·ratioMax·=·0.70·+·randFloat(0.,·0.1); | | [NORMAL] JSHintBear: | | A trailing decimal point can be confused with a dot: '0.'. binaries/data/mods/public/simulation/ai/petra/headquarters.js | 404| » » » » ratioMax·=·0.85·+·randFloat(0.,·0.1); | | [NORMAL] JSHintBear: | | A trailing decimal point can be confused with a dot: '0.'. Comment Actions Successful build - Chance fights ever on the side of the prudent. Updating workspaces... Build (release)... Build (debug)... Running release tests... Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK! Checking XML files... Relax-NG validity error : Extra element props in interleave /public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element variant failed to validate content /public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element group has extra content: variant Relax-NG validity error : Extra element group in interleave /public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element actor failed to validate content Comment Actions Executing section Default... Executing section Source... Executing section JS... binaries/data/mods/public/simulation/ai/petra/headquarters.js | 399| » » » let·ratioMax·=·0.70·+·randFloat(0.,·0.1); | | [NORMAL] JSHintBear: | | A trailing decimal point can be confused with a dot: '0.'. binaries/data/mods/public/simulation/ai/petra/headquarters.js | 404| » » » » ratioMax·=·0.85·+·randFloat(0.,·0.1); | | [NORMAL] JSHintBear: | | A trailing decimal point can be confused with a dot: '0.'. Comment Actions Question: why “workshop” and not the more precise “siege workshop”? I associate workshop with a place where furniture, pottery, sculpture, etc. are produced, not necessarily with siege engines. Besides, the only template currently being used, the Macedonian, is called “*_siege_workshop.xml”, therefore it would make sense to adjust those new ones to the existing, instead of the other way around. Comment Actions You are mixing two things: in the templates directory, the names indicates inheritance "civil_centre_military_colony.xml" because the colony has the civil_centre as parent. Here, the workshop does not inherits from siege. And we already have military_workshop, so we don't expect to make potery there. And anyway, i think siege is too restrictive as mods could produce other units. Comment Actions (I can review it in detail if you want, but I wouldn't complain if you commit this if you convinced yourself that this works as you wish.) Comment Actions ok i'll double check it and commit later. Only remaining point is the naming of the civ specific workshop in the structures directory: {civ}_workshop.xml or {civ}_military_workshop.xml or anything else? I'll wait a bit to see if some nice propositions. Comment Actions Some art is still named siege_workshop don't know if you also wan't to remove that
Comment Actions Yes the icon and some iber stuff. I'd rather leave it for another patch, but it's important to settle quickly the naming of that to avoid to further diverge. (same thing for the stable vs stables templates which will have to be standardized in a following patch also).
Comment Actions I would have chosen {civ}_siege_workshop.xml intuitively too. I suspect mods might want to use a new template. Comment Actions Making the barracks a parent for the workshop seems to be a bad idea to me; keep it directly as a military structure. And the naming in the structures/ folder typically follows the naming of the generic template. The exact naming is not that important; what matters is that's done consistently. | |||||||||||||||||||||||||||||||||||||