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

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

In D1001#39225, @Vulcan wrote:

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

Interesting ... how can I figure out what I did to cause a build failure?

Please avoid mixing template organization and gameplay changes in the same patch: the first part should be easily reviewed and commited while the second (adding it buildable to other civs) would require more discussion on where we want to go (do we want it, do we then need to remove elephant from fortresses, ...) which should be better dealt in another patch.
And try to base your patches on the latest version, to avoid merge conflict (the ai changes for example conflict with recent commits, but anyway it should be removed in a purely organisational patch).

In D1001#39228, @mimo wrote:

Please avoid mixing template organization and gameplay changes in the same patch: the first part should be easily reviewed and commited while the second (adding it buildable to other civs) would require more discussion on where we want to go (do we want it, do we then need to remove elephant from fortresses, ...) which should be better dealt in another patch.
And try to base your patches on the latest version, to avoid merge conflict (the ai changes for example conflict with recent commits, but anyway it should be removed in a purely organisational patch).

You're probably right, I'll redo this to only contain template organization then.

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

Updating workspaces...
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDAnimation]’:
FCollada/FCDocument/FCDLibrary.cpp:149:30:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
  const T* cptr = ((const FCDLibrary<T>*)l1)->GetEntity(0);
           ^
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDAnimationClip]’:
FCollada/FCDocument/FCDLibrary.cpp:150:34:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDCamera]’:
FCollada/FCDocument/FCDLibrary.cpp:151:27:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDController]’:
FCollada/FCDocument/FCDLibrary.cpp:152:31:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDEffect]’:
FCollada/FCDocument/FCDLibrary.cpp:153:27:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDEmitter]’:
FCollada/FCDocument/FCDLibrary.cpp:154:28:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDForceField]’:
FCollada/FCDocument/FCDLibrary.cpp:155:31:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDGeometry]’:
FCollada/FCDocument/FCDLibrary.cpp:156:29:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDImage]’:
FCollada/FCDocument/FCDLibrary.cpp:157:26:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDLight]’:
FCollada/FCDocument/FCDLibrary.cpp:158:26:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDMaterial]’:
FCollada/FCDocument/FCDLibrary.cpp:159:29:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDSceneNode]’:
FCollada/FCDocument/FCDLibrary.cpp:160:30:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDPhysicsModel]’:
FCollada/FCDocument/FCDLibrary.cpp:161:33:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDPhysicsMaterial]’:
FCollada/FCDocument/FCDLibrary.cpp:162:36:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ s

The patch looks good to me.
My only worry is when looking at the global (future) picture: it seems we are going towards barracks (possibly with a barracks_barracks), barracks_range and barracks_stable, all of them with the build category Barracks, and this new elephant_stables with build category ElephantStables does not really fit the picture. It would seem more logical (if we stick to that picture which was never really discussed) to have a barracks_elephantstable inheriting from barracks.
I've no strong feelings about that, so if other people have opinions, it's time to speak :)

I've no strong feelings about that, so if other people have opinions, it's time to speak :)

I didn't want to be intrusive but as you ask... I agree with you. barracks_barracks could sound weird to some ears but for me it's ok (we could use barracks_infantry but I don't know if it matches with barracks_range).
(Digression: I don't know if all the identity classes are ok. @Nescio It could be also nice to update at https://trac.wildfiregames.com/browser/ps/trunk/binaries/data/mods/public/simulation/components/Identity.js)

would require more discussion on where we want to go

I also agree. Those discussions can be simple (or not :/). My contribution is: I agree with all non messy thing ;-)

In D1001#39287, @mimo wrote:

The patch looks good to me.
My only worry is when looking at the global (future) picture: it seems we are going towards barracks (possibly with a barracks_barracks), barracks_range and barracks_stable, all of them with the build category Barracks, and this new elephant_stables with build category ElephantStables does not really fit the picture. It would seem more logical (if we stick to that picture which was never really discussed) to have a barracks_elephantstable inheriting from barracks.
I've no strong feelings about that, so if other people have opinions, it's time to speak :)

Those recently introduced “_barracks_” templates I simply understood to be placeholders. I assumed “military” is the template category for structures which train military units; having a “_military_barracks_barracks” or a “_military_barracks_elephant_stables” template seems a bit weird to me.

In D1001#39307, @Nescio wrote:

Those recently introduced “_barracks_” templates I simply understood to be placeholders. I assumed “military” is the template category for structures which train military units; having a “_military_barracks_barracks” or a “_military_barracks_elephant_stables” template seems a bit weird to me.

Yep, that's also fine with me if we rather go with military_barracks, military_range, military_stable and military_elephantstable. So the patch is basically ok. Let us just add the new class ElephantStable in the Identy.js doc as noticed by fatherbushido, and i would rather put the same build category for all these structures (let's call it Barracks as it already exists, waiting for a better name).

In D1001#39330, @mimo wrote:

In D1001#39307, @Nescio wrote:

Those recently introduced “_barracks_” templates I simply understood to be placeholders. I assumed “military” is the template category for structures which train military units; having a “_military_barracks_barracks” or a “_military_barracks_elephant_stables” template seems a bit weird to me.

Yep, that's also fine with me if we rather go with military_barracks, military_range, military_stable and military_elephantstable. So the patch is basically ok.

Besides, template names in structures/ typically match the names of the generic template, which is another reason why inserting an additional “_barracks_” layer is weird.

Let us just add the new class ElephantStable in the Identy.js doc as noticed by fatherbushido,

Just that one or are there any other missing classes to be added?

and i would rather put the same build category for all these structures (let's call it Barracks as it already exists, waiting for a better name).

OK but why do they have to have the same build category? E.g. farmsteads have build restriction “Farmstead” and storehouses have build restriction “Storehouse”; nor is the “Barracks” restriction included in the special/player.xml file.

In D1001#39344, @Nescio wrote:

Let us just add the new class ElephantStable in the Identy.js doc as noticed by fatherbushido,

Just that one or are there any other missing classes to be added?

Concerning this patch, that's the only added class. So it should be enough to only add that one. But fatherbushido's remark remind me that i've added the Workshop class in rP20389 without updating the list. So if you could add it at the same occasion. And if you see other's missing, why not.

and i would rather put the same build category for all these structures (let's call it Barracks as it already exists, waiting for a better name).

OK but why do they have to have the same build category? E.g. farmsteads have build restriction “Farmstead” and storehouses have build restriction “Storehouse”; nor is the “Barracks” restriction included in the special/player.xml file.

No real reason as long as it is not used. But 1) as it is not used, i don't see any good argument to add tons of different categories, and 2) if in the future we need to add some restrictions, i presume that it will concern globally all of these structures producing units, without caring of which unit exactly is produced (but i may be wrong, we'll only know when we need it).

In D1001#39350, @mimo wrote:

In D1001#39344, @Nescio wrote:

Let us just add the new class ElephantStable in the Identy.js doc as noticed by fatherbushido,

Just that one or are there any other missing classes to be added?

Concerning this patch, that's the only added class. So it should be enough to only add that one. But fatherbushido's remark remind me that i've added the Workshop class in rP20389 without updating the list. So if you could add it at the same occasion. And if you see other's missing, why not.

“Workshop” or “SiegeWorkshop”? The only one currently in use is the structures/mace_siege_workshop.xml template; besides, we also have a “CivilCentre” class, but not a “Centre”, therefore I think it's better to be a bit more precise. And shall I add “ArcheryRange” and “CavalryStables” as well?

and i would rather put the same build category for all these structures (let's call it Barracks as it already exists, waiting for a better name).

OK but why do they have to have the same build category? E.g. farmsteads have build restriction “Farmstead” and storehouses have build restriction “Storehouse”; nor is the “Barracks” restriction included in the special/player.xml file.

No real reason as long as it is not used. But 1) as it is not used, i don't see any good argument to add tons of different categories, and 2) if in the future we need to add some restrictions, i presume that it will concern globally all of these structures producing units, without caring of which unit exactly is produced (but i may be wrong, we'll only know when we need it).

1. Embassy has restriction “Embassy”, house “House”, market “Market”, temple “Temple”, in other words, up until now the build restriction typically matches the generic structure template; most of those are unused as well.
2. Then wouldn't assigning a “Military” restriction in the template_structure_military.xml file be better?

Again, I don't really care what the restriction is, however, I'd like to avoid introducing unnecessary inconsistencies, hence my asking.

In D1001#39362, @Nescio wrote:

In D1001#39350, @mimo wrote:

In D1001#39344, @Nescio wrote:

Let us just add the new class ElephantStable in the Identy.js doc as noticed by fatherbushido,

Just that one or are there any other missing classes to be added?

Concerning this patch, that's the only added class. So it should be enough to only add that one. But fatherbushido's remark remind me that i've added the Workshop class in rP20389 without updating the list. So if you could add it at the same occasion. And if you see other's missing, why not.

“Workshop” or “SiegeWorkshop”? The only one currently in use is the structures/mace_siege_workshop.xml template; besides, we also have a “CivilCentre” class, but not a “Centre”, therefore I think it's better to be a bit more precise. And shall I add “ArcheryRange” and “CavalryStables” as well?

I tend to think that it will be better to name the templates military_workshop (as done in newer templates) and not military_siege_workshop (as in the old mace one), thus the name of the class. But that will be finalized when someone reorganize the workshop templates.
And do not add classes which are not yet used as the two you proposed.

and i would rather put the same build category for all these structures (let's call it Barracks as it already exists, waiting for a better name).

OK but why do they have to have the same build category? E.g. farmsteads have build restriction “Farmstead” and storehouses have build restriction “Storehouse”; nor is the “Barracks” restriction included in the special/player.xml file.

No real reason as long as it is not used. But 1) as it is not used, i don't see any good argument to add tons of different categories, and 2) if in the future we need to add some restrictions, i presume that it will concern globally all of these structures producing units, without caring of which unit exactly is produced (but i may be wrong, we'll only know when we need it).

1. Embassy has restriction “Embassy”, house “House”, market “Market”, temple “Temple”, in other words, up until now the build restriction typically matches the generic structure template; most of those are unused as well.

Once again, i don't see any use case for so much categories.

2. Then wouldn't assigning a “Military” restriction in the template_structure_military.xml file be better?

And as i said, i don't really care of the exact name (i proposed Barracks as a temporary solution as it already exists) but Military looks to me too general.

Again, I don't really care what the restriction is, however, I'd like to avoid introducing unnecessary inconsistencies, hence my asking.

I think we already discussed quite a lot for something which is not used, and has nearly no chance to be ever used.

In D1001#39363, @mimo wrote:

Once again, i don't see any use case for so much categories.

Nor do I, but they are already there right now.

In D1001#39363, @mimo wrote:

I think we already discussed quite a lot for something which is not used, and has nearly no chance to be ever used.

True; so “barracks” it is?

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...