Details

Reviewers

Commits

rP20459: Rearrange tower templates

Trac Tickets

Summary

The template structure tree currently is:

template_structure_defense_defense_tower.xml
template_structure_defense_outpost.xml
template_structure_defense_sentry_tower.xml

However, the sentry tower has the defense tower as its parent, which is not reflected in its name (cf. template_structure_civic_civil_centre_military_colony.xml)

This patch creates a new generic tower template and changes the situation into:

template_structure_defense_tower.xml
template_structure_defense_tower_outpost.xml
template_structure_defense_tower_sentry.xml
template_structure_defense_tower_stone.xml

All dependent templates are changed accordingly

The advantage of this patch is that if you want to edit the stats (e.g. costs, health, vision range, etc) of all towers, you can simply edit the generic tower file, and all towers will automatically change accordingly; and if you want to change the stats of the defense tower only, you only have to edit the stone tower file.
Without this patch this was not possible. If you wanted to change the defensive tower, you also had to look at the sentry tower template, because the sentry tower had the defensive tower as its parent, something which is easy to overlook. With this patch this is no longer the case.

Furthermore:

Renamed technologies for naming consistency (e.g. tower_armour instead of attack_tower_defense)
added a missing skirmish/structures/default_sentry_tower.xml

Test Plan

Check if nothing is overlooked and if everything works properly.

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

There are a very large number of changes, so older changes are hidden. Show Older Changes

bb added inline comments.Oct 22 2017, 4:31 PM

binaries/data/mods/public/simulation/data/technologies/tower_armour.json
1 ↗	(On Diff #3702)	file rename ok
9 ↗	(On Diff #3702)	liked the old one better
binaries/data/mods/public/simulation/data/technologies/tower_crenellations.json
13 ↗	(On Diff #3702)	its a balance change don't see it making things better So leave it DefenseTower
binaries/data/mods/public/simulation/data/technologies/tower_decay.json
9 ↗	(On Diff #3702)	changed but ok with me
binaries/data/mods/public/simulation/data/technologies/tower_murderholes.json
10–12 ↗	(On Diff #3702)	I don't think we have clear style rules for json's, but this seems sane so ok
13 ↗	(On Diff #3702)	We have already D111 so leave it DefenseTower
binaries/data/mods/public/simulation/data/technologies/tower_range.json
11 ↗	(On Diff #3702)	vision range should stay updated aswell
binaries/data/mods/public/simulation/data/technologies/tower_vision.json
8 ↗	(On Diff #3702)	changed, but ok with me
binaries/data/mods/public/simulation/data/technologies/tower_watch.json
19 ↗	(On Diff #3702)	while at it remove the .0 (notice that doesn't change the simstate)
21 ↗	(On Diff #3702)	leave it DefenseTower
binaries/data/mods/public/simulation/templates/template_structure_defense_tower.xml
5–12 ↗	(On Diff #3702)	The observation that the components should be in alphabetical order is correct, however inside a component we use a "logical" order. Please keep that order, the logical order is (or at least should be) the order in which the schema has them. You can find the schema in the components' .js/.cpp file. So for the attack component that is attack.js (also in the original file the order was correct so please use that) Same goes for other components and other files
22 ↗	(On Diff #3702)	set to DefenseTower, since its used twice that is ok So outpost will change the value
26–28 ↗	(On Diff #3702)	useless since it is already added in template_structure
29–37 ↗	(On Diff #3702)	all childs use different values, so better set them there and don't have this here. Also no need for setting those 0's since the template_structure already does
39 ↗	(On Diff #3702)	all childs use a different value so set it there
47 ↗	(On Diff #3702)	set in childs
52 ↗	(On Diff #3702)	don't set things which are unused
55–61 ↗	(On Diff #3702)	All childs different so set there, also no need for 0's since parents
64 ↗	(On Diff #3702)	D757 will move that to template_structure so ok for now, but probably will need a rebase (notice this adds the sound to the outpost which is good)
64–67 ↗	(On Diff #3702)	(ofc in these list type of things "logical" less good defined, so alphabetical seems ok)
binaries/data/mods/public/simulation/templates/template_structure_defense_tower_outpost.xml
19–20 ↗	(On Diff #3702)	buildingAI: Don't give outpost a default arrow
23 ↗	(On Diff #3702)	mindistance change => is balance so not in this patch (also don't like the change)
45 ↗	(On Diff #3702)	its not set anywhere in parents right? so useless
58–59 ↗	(On Diff #3702)	swap, to hold the old state (visual order in queue)
63–64 ↗	(On Diff #3702)	these 2 are common in all childs of defense_tower, so add them there
binaries/data/mods/public/simulation/templates/template_structure_defense_tower_sentry.xml
20 ↗	(On Diff #3702)	doesn't make things cleaner imo, just set directly
30 ↗	(On Diff #3702)	same
binaries/data/mods/public/simulation/templates/template_structure_defense_tower_stone.xml
50–54 ↗	(On Diff #3702)	iirc this order also defines the order of the elements visible, so don't change it
binaries/data/mods/public/simulation/templates/template_structure_defense_wall_tower.xml
55 ↗	(On Diff #3702)	leave the classes as is

Thank you for you detailed comments. It's been over a month since last time I looked at this patch, although I'm quite sure I reverted any statistical changes (“balance”) weeks ago. However, I vaguely recall changing some classes as part of the new templates (e.g. removing the DefenseTower from the sentry tower template forced me to change technology files to keep them effectively the same). Anyway, I'll have a more careful look at it to address the just concerns you raised. (Or maybe it's cleaner for me to redo this patch from scratch).

Currently it seems classes are distributed as:

Defensive: all defensive structures
DefenseTower: sentry tower, stone tower
GarrisonTower: sentry tower, stone tower 
Outpost: outpost
SentryTower: sentry tower
Tower: sentry tower, stone tower, wall turret

which means “DefenseTower” and “GarrisonTower” are duplicates (as are “Fortress” and “GarrisonFortress”); also, there is no class for the stone tower exclusively.

Right now I think something like the following would be better, ideally:

Defensive: all defensive structures
Fortification [new]: all arrow shooting structures: 
Tower: all structures which inherit the tower template: outpost, sentry tower, stone tower
GarrisonTower: sentry tower, stone tower
Outpost: outpost
SentryTower: sentry tower
StoneTower: stone tower
WallTurret: wall turret
GarrisonFortress: fortress, army camp
Fortress: fortress

However, that probably belongs in a different patch. (And right now I have to figure out what this diff exactly does and then change it.)

Redid this patch from scratch; all of bb's points should now be addressed. However, please check critically.

PS It's a bit later than intended, I apologize for the delay.
PPS This is the first time I use a git clone (instead of svn), but this ought not to be a problem.

PPPS Initially overlooked territory influence, now fixed.

Build is green

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

http://jenkins-master:8080/job/phabricator/2159/ for more details.

Harbormaster completed remote builds in B3477: Diff 3965.Oct 25 2017, 1:17 PM

Build is green

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

http://jenkins-master:8080/job/phabricator/2160/ for more details.

Harbormaster completed remote builds in B3480: Diff 3968.Oct 25 2017, 1:21 PM

Needs a rebase, couldn't test if the sim states are identical due to that, you could test some of that yourself by comparing the output before and after the patch by opening the structree and using this patch:
Index: binaries/data/mods/public/gui/reference/structree/structree.js

===================================================================
--- binaries/data/mods/public/gui/reference/structree/structree.js	(revision 20403)
+++ binaries/data/mods/public/gui/reference/structree/structree.js	(working copy)
@@ -70,7 +70,14 @@
 
 	for (let s of templateLists.structures.keys())
 		if (!g_ParsedData.structures[s])
+		{
 			g_ParsedData.structures[s] = loadStructure(s);
+			if (s=="structures/athen_defense_tower" || s=="structures/athen_sentry_tower" || s=="structures/athen_outpost")
+			{
+				warn(uneval(s));
+				warn(uneval(loadStructure(s)));
+			}
+		}
 
 	// Load technologies
 	g_ParsedData.techs[civCode] = {};

binaries/data/mods/public/simulation/data/technologies/tower_armour.json
9 ↗	(On Diff #3968)	we should use American English so armor.
binaries/data/mods/public/simulation/data/technologies/tower_murderholes.json
9 ↗	(On Diff #3968)	according to my understanding of grammer this is correct.
binaries/data/mods/public/simulation/templates/template_structure_defensive_tower.xml
1 ↗	(On Diff #3968)	The name is missleading now, since we inherit the template from `structure_defense` and then we call it `structure_defensive_tower` I guees we should keep it `structure_defense_tower`.
52 ↗	(On Diff #3968)	don't change the order of elements here

In D914#39534, @bb wrote:

Needs a rebase, couldn't test if the sim states are identical due to that, you could test some of that yourself by comparing the output before and after the patch by opening the structree and using this patch:
Index: binaries/data/mods/public/gui/reference/structree/structree.js

Sorry, I don't think I quite understand. Could you elaborate?

binaries/data/mods/public/simulation/data/technologies/tower_armour.json
9 ↗	(On Diff #3968)	Why? There seems to be no spelling consistency in 0 A.D. Some things are in American English, others are in British English, and a few things are occassionally in one but sometimes in another.
binaries/data/mods/public/simulation/data/technologies/tower_murderholes.json
9 ↗	(On Diff #3968)	grammer [sic]? The tooltip is grammatically correct.
binaries/data/mods/public/simulation/templates/template_structure_defensive_tower.xml
1 ↗	(On Diff #3968)	Good point. I guess I made this mistake because “defense” is not a proper adjective, and “defensive” is. Besides, template names typically match class names (e.g. template_structure_military_fortress), but apparently “defense” is an exception to this rule (its class is called “Defensive”). So basically I see two options, both about the same amount of work: move all tower templates to `defense` (for consistency with the current status quo) move the generic `defense` and all walls templates to `defensive` (for consistency with classes and other template naming)

In D914#39543, @Nescio wrote:

In D914#39534, @bb wrote:

Needs a rebase,

Patch doesn't apply cleanly here, so please fix that.

couldn't test if the sim states are identical due to that,

Basicly I am a lazy and not wanting to change code to make the patch work here :P, will be fixed when uploading a rebase.

you could test some of that yourself by comparing the output before and after the patch by opening the structree and using this patch:
Index: binaries/data/mods/public/gui/reference/structree/structree.js

Sorry, I don't think I quite understand. Could you elaborate?

This revision shouldn't change the simState (simulation state, so all the data about the world, players etc. shouldn't be changed due to this patch), for that the old and new templates should be identical. For testing that you could print out all the template data, that way you don't have to check every parent by hand and stuff to make sure nothing is changed. The diff above will print that data out when opening the structree for the athen buildings (you could take another civ).

binaries/data/mods/public/simulation/data/technologies/tower_armour.json
9 ↗	(On Diff #3968)	That is true , but everything should be in American English, so at least don't change American => British (yes we have a Armour component and not Armor, but that is just in the code and perhaps should change but meh)
binaries/data/mods/public/simulation/data/technologies/tower_murderholes.json
9 ↗	(On Diff #3968)	(That is what I meant above)
binaries/data/mods/public/simulation/templates/template_structure_defensive_tower.xml
1 ↗	(On Diff #3968)	In this patch at least keep the status quo, (a next patch might address the second point)

Updated: defensive to defense

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’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDPhysicsScene]’:
FCollada/FCDocument/FCDLibrary.cpp:163:33:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
In file included from FCollada/FUtils/FUSemaphore.cpp:10:0:
FCollada/FUtils/FUSemaphore.h:36:2: warning: #warning "FUSemaphore: Semaphore not implemented for non Windows" [-Wcpp]
 #warning "FUSemaphore: Semaphore not implemented for non Windows"
  ^
FCollada/FUtils/FUStringConversion.cpp: In function ‘void TrickLinkerFUStringConversion()’:
FCollada/FUtils/FUStringConversion.cpp:281:8: warning: variable ‘f’ set but not used [-Wunused-but-set-variable]
  float f = FUStringConversion::ToFloat(&c);
        ^
FCollada/FUtils/FUStringConversion.cpp:283:7: warning: variable ‘b’ set but not used [-Wunused-but-set-variable]
  bool b = FUStringConversion::ToBoolean(c);
       ^
FCollada/FUtils/FUStringConversion.cpp:285:8: warning: variable ‘i32’ set but not used [-Wunused-but-set-variable]
  int32 i32 = FUStringConversion::ToInt32(&c);
        ^
FCollada/FUtils/FUStringConversion.cpp:287:9: warning: variable ‘u32’ set but not used [-Wunused-but-set-variable]
  uint32 u32 = FUStringConversion::ToUInt32(&c);
         ^
In file included from FCollada/FUtils/FUThread.cpp:10:0:
FCollada/FUtils/FUThread.h:30:2: warning: #warning "Threads not yet implemented for non Windows." [-Wcpp]
 #warning "Threads not yet implemented for non Windows."
  ^
FCollada/FCDocument/FCDAnimationCurve.cpp: In member function ‘float FCDAnimationCurve::Evaluate(float) const’:
FCollada/FCDocument/FCDAnimationCurve.cpp:396:13: warning: ‘inTangent.FMVector2::<anonymous>.FMVector2::<anonymous union>::x’ may be used uninitialized in this function [-Wmaybe-uninitialized]
   FMVector2 inTangent;
             ^
FCollada/FCDocument/FCDAnimationCurve.cpp:396:13: warning: ‘inTangent.FMVector2::<anonymous>.FMVector2::<anonymous union>::y’ may be used uninitialized in this function [-Wmaybe-uninitialized]
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/FC

Harbormaster completed remote builds in B3601: Diff 4085.Nov 3 2017, 11:28 PM

bb added inline comments.Nov 10 2017, 2:29 PM

binaries/data/mods/public/simulation/templates/template_structure_defense_tower_outpost.xml
63–64 ↗	(On Diff #4085)	swap the order so we keep the current state (the order here defines the order in the gui, so alphabetical doesn't have to be correct, and isn't here)
binaries/data/mods/public/simulation/templates/template_structure_defense_tower_sentry.xml
38–40 ↗	(On Diff #4085)	Please keep the order inside the components as they were, the order should be like it is defined in the identity component.
binaries/data/mods/public/simulation/templates/template_structure_defense_tower_stone.xml
32 ↗	(On Diff #4085)	StoneTower added (fine for me though, can leave it)

Thanks again for your constructive criticism!

binaries/data/mods/public/simulation/templates/template_structure_defense_tower_outpost.xml
63–64 ↗	(On Diff #4085)	You're right, I'll correct this. However, both are village phase technologies, so it's not really a game changer.
binaries/data/mods/public/simulation/templates/template_structure_defense_tower_sentry.xml
38–40 ↗	(On Diff #4085)	Actually I did not change the order here; I removed the specific name and required technology (per one of your earlier remarks) and added a classes component.

bb added inline comments.Nov 10 2017, 4:59 PM

binaries/data/mods/public/simulation/templates/template_structure_defense_tower_outpost.xml
63–64 ↗	(On Diff #4085)	indeed true, even if it were tech from different phases there would change nothing but the gui, but as ppl are used to this gui, I rather keep it
binaries/data/mods/public/simulation/templates/template_structure_defense_tower_sentry.xml
38–40 ↗	(On Diff #4085)	ohh i c, still visibleclasses should be directly under classes, that is probably what fooled me :P

Updated per bb's latest remarks.

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

Harbormaster completed remote builds in B3653: Diff 4136.Nov 10 2017, 6:29 PM

default_defense_tower wrong parent, same for default_outpost. (a default_sentry_tower is missing it seems, but unrelated #4849)
other/palisades_rocks_fort.xml wrong parent

binaries/data/mods/public/simulation/templates/template_structure_defense_tower_sentry.xml
54 ↗	(On Diff #4136)	unneeded since its defined in parent

Updated per bb's latest remarks.

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

Harbormaster completed remote builds in B3703: Diff 4187.Nov 14 2017, 10:00 PM

bb added inline comments.Nov 15 2017, 8:05 PM

binaries/data/mods/public/simulation/data/technologies/tower_armour.json
4 ↗	(On Diff #4187)	While ad it you could add whitespaces here (after the `{` and before the `}`), this goes for all the technology jons (only change the added files, we already have a diff for all other files)
5 ↗	(On Diff #4187)	(and obviously the same applies here)
11–13 ↗	(On Diff #4187)	and here
15 ↗	(On Diff #4187)	and NOT here (its another type of brace)
binaries/data/mods/public/simulation/data/technologies/tower_crenellations.json
4–5 ↗	(On Diff #4187)	etc.
binaries/data/mods/public/simulation/templates/template_structure_defense_tower_outpost.xml
61 ↗	(On Diff #4187)	marking the line (see below)
binaries/data/mods/public/simulation/templates/template_structure_defense_tower_stone.xml
44 ↗	(On Diff #4187)	this should be moved to the parent (there are errors when selecting a unit), the marked line should be removed then also
53–55 ↗	(On Diff #4136)	(getting the logic, but didn't rly ask for it, the other instance was really useless, since the exact same value was in the parent, but this is another value, but meh putting in childs seems fine)

bb requested changes to this revision.Nov 15 2017, 8:06 PM

This revision now requires changes to proceed.Nov 15 2017, 8:06 PM

Updated per bb's latest remarks.

Looks ok, complete
Tested as ok, and complete, All buildings validate, techs still work
Couldn't find more duplication

This revision is now accepted and ready to land.Nov 15 2017, 10:11 PM

Closed by commit rP20459: Rearrange tower templates (authored by bb). · Explain WhyNov 15 2017, 10:16 PM

This revision was automatically updated to reflect the committed changes.

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

Harbormaster completed remote builds in B3716: Diff 4204.Nov 15 2017, 10:57 PM

Accepted and commited? Finally! Many thanks for your review :)

tower templatesClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 4209

ps/trunk/binaries/data/mods/public/simulation/data/technologies/attack_tower_crenellations.json

ps/trunk/binaries/data/mods/public/simulation/data/technologies/attack_tower_defense.json

ps/trunk/binaries/data/mods/public/simulation/data/technologies/attack_tower_murderholes.json

ps/trunk/binaries/data/mods/public/simulation/data/technologies/attack_tower_range.json

ps/trunk/binaries/data/mods/public/simulation/data/technologies/attack_tower_watch.json

ps/trunk/binaries/data/mods/public/simulation/data/technologies/decay_outpost.json

ps/trunk/binaries/data/mods/public/simulation/data/technologies/tower_armour.json

ps/trunk/binaries/data/mods/public/simulation/data/technologies/tower_crenellations.json

ps/trunk/binaries/data/mods/public/simulation/data/technologies/tower_decay.json

ps/trunk/binaries/data/mods/public/simulation/data/technologies/tower_murderholes.json

ps/trunk/binaries/data/mods/public/simulation/data/technologies/tower_range.json

ps/trunk/binaries/data/mods/public/simulation/data/technologies/tower_vision.json

ps/trunk/binaries/data/mods/public/simulation/data/technologies/tower_watch.json

ps/trunk/binaries/data/mods/public/simulation/data/technologies/vision_outpost.json

ps/trunk/binaries/data/mods/public/simulation/templates/other/palisades_rocks_fort.xml

ps/trunk/binaries/data/mods/public/simulation/templates/skirmish/structures/default_defense_tower.xml

ps/trunk/binaries/data/mods/public/simulation/templates/skirmish/structures/default_outpost.xml

ps/trunk/binaries/data/mods/public/simulation/templates/skirmish/structures/default_sentry_tower.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/athen_defense_tower.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/athen_outpost.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/athen_sentry_tower.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/baby_defense_tower.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/brit_defense_tower.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/brit_outpost.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/brit_sentry_tower.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/cart_defense_tower.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/cart_outpost.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/cart_sentry_tower.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/gaul_defense_tower.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/gaul_outpost.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/gaul_sentry_tower.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/iber_defense_tower.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/iber_outpost.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/iber_sentry_tower.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/mace_defense_tower.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/mace_outpost.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/mace_sentry_tower.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/maur_defense_tower.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/maur_outpost.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/maur_sentry_tower.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/pers_defense_tower.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/pers_outpost.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/pers_sentry_tower.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/ptol_defense_tower.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/ptol_outpost.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/ptol_sentry_tower.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/rome_defense_tower.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/rome_outpost.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/rome_sentry_tower.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/sele_defense_tower.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/sele_outpost.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/sele_sentry_tower.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/spart_defense_tower.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/spart_outpost.xml

ps/trunk/binaries/data/mods/public/simulation/templates/structures/spart_sentry_tower.xml

ps/trunk/binaries/data/mods/public/simulation/templates/template_structure_defense_defense_tower.xml

ps/trunk/binaries/data/mods/public/simulation/templates/template_structure_defense_outpost.xml

ps/trunk/binaries/data/mods/public/simulation/templates/template_structure_defense_sentry_tower.xml

ps/trunk/binaries/data/mods/public/simulation/templates/template_structure_defense_tower.xml

ps/trunk/binaries/data/mods/public/simulation/templates/template_structure_defense_tower_outpost.xml

ps/trunk/binaries/data/mods/public/simulation/templates/template_structure_defense_tower_sentry.xml

ps/trunk/binaries/data/mods/public/simulation/templates/template_structure_defense_tower_stone.xml

tower templates
ClosedPublic
Actions

Revision Contents
Changeset List