Page MenuHomeWildfire Games
Differential D1439

Match wall tower obstruction width with wall piece length
Needs ReviewPublic
Actions

Authored by temple on Apr 7 2018, 4:26 AM.

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Trac Tickets
#5118
Summary

If structures without the same control group overlap then after rP21597 they'll prevent each other from being built. So we need to take more care with walls so that wall towers don't overlap. In particular this means insuring that the wall piece length is at least the obstruction width. This patch fixes the ones that need fixing.

Test Plan

Check the values are reasonable and I didn't miss any templates.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 5779
Build 9692: Vulcan BuildJenkins
Build 9691: arc lint + arc unit

Event Timeline

temple created this revision.Apr 7 2018, 4:26 AM
Vulcan added a subscriber: Vulcan.Apr 7 2018, 4:28 AM

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

Link to build: https://jenkins.wildfiregames.com/job/differential/352/display/redirect

Harbormaster completed remote builds in B5779: Diff 6332.Apr 7 2018, 4:28 AM
elexis added a subscriber: elexis.Apr 7 2018, 9:31 AM

Oh, and Emperior experienced this in a game and didn't report it? Shame and blame!

Patch looks correct, but there is a general problem here. It is very easy to reintroduce this when updating templates or adding new ones. Also people with mods are very likely to not have this fixed.
It would be much more stable if it would check by itself if all wall templates are correct and complete.
There are multiple options:

  • simulation tests (has the advantage of being run very often, but the downside that they should use their own templates and that their purpose is to verify components, not templates)
  • the template validation tool (has the disadvantage of being run (ran?) very rarely, in particular almost never by mod people)
  • testing within a simulation component or GUI when starting the game, loading the templates or placing a wall (has the advantage of being run most likely immediately after a template change, disadvantage that it doesn't test in rmgen stage). (I had proposed the latter for vision vs attack range too, but there was this artillery counter-example.)
elexis added a comment.Apr 7 2018, 12:18 PM
  • WallPiece.prototype.Schema should describe the need to take the obstruction into consideration
  • Adding a warning in the simulation seems wrong, because there is no way for the JS wallpiece component to retrieve the static obstruction size. IIRC all Obstruction code was moved from JS to C++ once in history due to performance.
  • We can add it to the globalscripts Template parsing, but that would most likely be triggered more than once per match.

checkrefs script:

other/fence_longWall piece length (12) is shorter than obstruction width (13.0) at checkrefs.pl line 116.
other/fence_shortWall piece length (6) is shorter than obstruction width (6.5) at checkrefs.pl line 116.
other/palisades_angle_spikeWall piece length (3) is shorter than obstruction width (3.5) at checkrefs.pl line 116.
other/palisades_rocks_endWall piece length (0.8) is shorter than obstruction width (3.0) at checkrefs.pl line 116.
other/palisades_rocks_fortWall piece length (8.0) is shorter than obstruction width (9.0) at checkrefs.pl line 116.
other/palisades_rocks_mediumWall piece length (9.2) is shorter than obstruction width (10) at checkrefs.pl line 116.
structures/athen_fortressWall piece length (24.0) is shorter than obstruction width (26.0) at checkrefs.pl line 116.
structures/athen_wall_shortWall piece length (12.3) is shorter than obstruction width (13.0) at checkrefs.pl line 116.
structures/brit_fortressWall piece length (28) is shorter than obstruction width (29.0) at checkrefs.pl line 116.
structures/cart_fortressWall piece length (25.0) is shorter than obstruction width (28.0) at checkrefs.pl line 116.
structures/cart_s_wall_longWall piece length (11.0) is shorter than obstruction width (14) at checkrefs.pl line 116.
structures/cart_s_wall_mediumWall piece length (9.2) is shorter than obstruction width (10) at checkrefs.pl line 116.
structures/cart_wall_longWall piece length (37.0) is shorter than obstruction width (46.0) at checkrefs.pl line 116.
structures/cart_wall_mediumWall piece length (24.0) is shorter than obstruction width (30.0) at checkrefs.pl line 116.
structures/cart_wall_shortWall piece length (13.0) is shorter than obstruction width (16.0) at checkrefs.pl line 116.
structures/cart_wall_towerWall piece length (7.5) is shorter than obstruction width (11.0) at checkrefs.pl line 116.
structures/gaul_fortressWall piece length (16.8) is shorter than obstruction width (25.0) at checkrefs.pl line 116.
structures/iber_fortressWall piece length (24.5) is shorter than obstruction width (27.0) at checkrefs.pl line 116.
structures/kush_wall_longWall piece length (34.9) is shorter than obstruction width (38.0) at checkrefs.pl line 116.
structures/kush_wall_mediumWall piece length (22.9) is shorter than obstruction width (26.0) at checkrefs.pl line 116.
structures/kush_wall_shortWall piece length (11) is shorter than obstruction width (14.0) at checkrefs.pl line 116.
structures/kush_wall_towerWall piece length (8.6) is shorter than obstruction width (10) at checkrefs.pl line 116.
structures/mace_fortressWall piece length (24.0) is shorter than obstruction width (26.0) at checkrefs.pl line 116.
structures/mace_wall_shortWall piece length (12.3) is shorter than obstruction width (13.0) at checkrefs.pl line 116.
structures/maur_fortressWall piece length (22.0) is shorter than obstruction width (25.0) at checkrefs.pl line 116.
structures/maur_wall_mediumWall piece length (24.1) is shorter than obstruction width (25.0) at checkrefs.pl line 116.
structures/maur_wall_shortWall piece length (12.2) is shorter than obstruction width (13.0) at checkrefs.pl line 116.
structures/maur_wall_towerWall piece length (7.8) is shorter than obstruction width (9.5) at checkrefs.pl line 116.
structures/pers_fortressWall piece length (24.5) is shorter than obstruction width (25.0) at checkrefs.pl line 116.
structures/ptol_fortressWall piece length (24.0) is shorter than obstruction width (26.0) at checkrefs.pl line 116.
structures/ptol_wall_mediumWall piece length (25) is shorter than obstruction width (26.0) at checkrefs.pl line 116.
structures/ptol_wall_towerWall piece length (9.6) is shorter than obstruction width (10) at checkrefs.pl line 116.
structures/rome_army_campWall piece length (29.5) is shorter than obstruction width (36.0) at checkrefs.pl line 116.
structures/rome_siege_wall_longWall piece length (36.0) is shorter than obstruction width (37.0) at checkrefs.pl line 116.
structures/rome_siege_wall_mediumWall piece length (24.0) is shorter than obstruction width (25.0) at checkrefs.pl line 116.
structures/rome_siege_wall_shortWall piece length (12.0) is shorter than obstruction width (13.0) at checkrefs.pl line 116.
structures/rome_siege_wall_towerWall piece length (6.0) is shorter than obstruction width (7.0) at checkrefs.pl line 116.
structures/sele_fortressWall piece length (23) is shorter than obstruction width (26.0) at checkrefs.pl line 116.
structures/sele_wall_longWall piece length (33.2) is shorter than obstruction width (34.0) at checkrefs.pl line 116.
structures/sele_wall_mediumWall piece length (21.1) is shorter than obstruction width (24.0) at checkrefs.pl line 116.
structures/sele_wall_shortWall piece length (10.9) is shorter than obstruction width (13.0) at checkrefs.pl line 116.
structures/spart_fortressWall piece length (24.0) is shorter than obstruction width (26.0) at checkrefs.pl line 116.
structures/spart_wall_shortWall piece length (12.3) is shorter than obstruction width (13.0) at checkrefs.pl line 116.

globalscripts (it doesn't know the template name!)

WARNING: Template with parent template_structure_defense_tower_sentry has a wall piece length (8) shorter than obstruction (9)
WARNING: Template with parent template_structure_defense_wall_medium has a wall piece length (9.2) shorter than obstruction (10)
WARNING: Template with parent template_structure_defense_wall has a wall piece length (0.8) shorter than obstruction (3)
WARNING: Template with parent template_structure_military_fortress has a wall piece length (24) shorter than obstruction (26)
WARNING: Template with parent template_structure_defense_wall_short has a wall piece length (12.3) shorter than obstruction (13)
WARNING: Template with parent template_structure_military_fortress has a wall piece length (28) shorter than obstruction (29)
WARNING: Template with parent template_structure_military_fortress has a wall piece length (25) shorter than obstruction (28)
WARNING: Template with parent other/palisades_rocks_long has a wall piece length (11) shorter than obstruction (14)
WARNING: Template with parent other/palisades_rocks_medium has a wall piece length (9.2) shorter than obstruction (10)
WARNING: Template with parent template_structure_defense_wall_tower has a wall piece length (7.5) shorter than obstruction (11)
WARNING: Template with parent template_structure_military_fortress has a wall piece length (25) shorter than obstruction (28)
WARNING: Template with parent template_structure_defense_wall_long has a wall piece length (37) shorter than obstruction (46)
WARNING: Template with parent template_structure_defense_wall_medium has a wall piece length (24) shorter than obstruction (30)
WARNING: Template with parent template_structure_defense_wall_short has a wall piece length (13) shorter than obstruction (16)
WARNING: Template with parent template_structure_military_fortress has a wall piece length (16.8) shorter than obstruction (25)
WARNING: Template with parent template_structure_military_fortress has a wall piece length (24.5) shorter than obstruction (27)
WARNING: Template with parent template_structure_defense_wall_tower has a wall piece length (8.6) shorter than obstruction (10)
WARNING: Template with parent template_structure_defense_wall_long has a wall piece length (34.9) shorter than obstruction (38)
WARNING: Template with parent template_structure_defense_wall_medium has a wall piece length (22.9) shorter than obstruction (26)
WARNING: Template with parent template_structure_defense_wall_short has a wall piece length (11) shorter than obstruction (14)
WARNING: Template with parent template_structure_military_fortress has a wall piece length (24) shorter than obstruction (26)
WARNING: Template with parent template_structure_defense_wall_short has a wall piece length (12.3) shorter than obstruction (13)
WARNING: Template with parent template_structure_defense_wall_tower has a wall piece length (7.8) shorter than obstruction (9.5)
WARNING: Template with parent template_structure_military_fortress has a wall piece length (22) shorter than obstruction (25)
WARNING: Template with parent template_structure_defense_wall_medium has a wall piece length (24.1) shorter than obstruction (25)
WARNING: Template with parent template_structure_defense_wall_short has a wall piece length (12.2) shorter than obstruction (13)
WARNING: Template with parent template_structure_military_fortress has a wall piece length (24.5) shorter than obstruction (25)
WARNING: Template with parent template_structure_defense_wall_tower has a wall piece length (9.6) shorter than obstruction (10)
WARNING: Template with parent template_structure_military_fortress has a wall piece length (24) shorter than obstruction (26)
WARNING: Template with parent template_structure_defense_wall_medium has a wall piece length (25) shorter than obstruction (26)
WARNING: Template with parent template_structure_defense_wall_tower has a wall piece length (6) shorter than obstruction (7)
WARNING: Template with parent template_structure_special has a wall piece length (29.5) shorter than obstruction (36)
WARNING: Template with parent template_structure_defense_wall_long has a wall piece length (36) shorter than obstruction (37)
WARNING: Template with parent template_structure_defense_wall_medium has a wall piece length (24) shorter than obstruction (25)
WARNING: Template with parent template_structure_defense_wall_short has a wall piece length (12) shorter than obstruction (13)
WARNING: Template with parent template_structure_military_fortress has a wall piece length (23) shorter than obstruction (26)
WARNING: Template with parent template_structure_defense_wall_long has a wall piece length (33.2) shorter than obstruction (34)
WARNING: Template with parent template_structure_defense_wall_medium has a wall piece length (21.1) shorter than obstruction (24)
WARNING: Template with parent template_structure_defense_wall_short has a wall piece length (10.9) shorter than obstruction (13)
WARNING: Template with parent template_structure_military_fortress has a wall piece length (24) shorter than obstruction (26)
WARNING: Template with parent template_structure_defense_wall_short has a wall piece length (12.3) shorter than obstruction (13)

(Since I committed the obstruction thing, it should be my duty to update the templates.)

I wonder if fixing this also helps with the wall demo map.

elexis edited the summary of this revision. (Show Details)Apr 7 2018, 12:21 PM
elexis edited the test plan for this revision. (Show Details)
elexis mentioned this in D1440: Warn if the Obstruction size exceeds the Wallpiece length.Apr 7 2018, 12:23 PM
elexis added a comment.Apr 7 2018, 12:44 PM

I've used this to increase the WallPiece size to match the obstruction. That is good for the athen fortress, but for the great carthaginian walls, the obstruction size should be reduced instead:

Index: binaries/data/mods/public/globalscripts/Templates.js
===================================================================
--- binaries/data/mods/public/globalscripts/Templates.js	(revision 21666)
+++ binaries/data/mods/public/globalscripts/Templates.js	(working copy)
@@ -457,16 +457,22 @@ function GetTemplateDataHelper(template,
 		if (template.WallSet.Templates.WallCurves)
 			ret.wallSet.templates.curves = template.WallSet.Templates.WallCurves.split(" ");
 	}
 
 	if (template.WallPiece)
+	{
+		let length = +template.WallPiece.Length;
+		if (ret.obstruction && ret.obstruction.shape.type == "static")
+			length = Math.max(length, ret.obstruction.shape.width, ret.obstruction.shape.depth);
+
 		ret.wallPiece = {
-			"length": +template.WallPiece.Length,
+			"length": length,
 			"angle": +(template.WallPiece.Orientation || 1) * Math.PI,
 			"indent": +(template.WallPiece.Indent || 0),
 			"bend": +(template.WallPiece.Bend || 0) * Math.PI
 		};
+	}
 
 	return ret;
 }
 
 /**


temple mentioned this in D1445: Make structures not block construction.Apr 9 2018, 9:10 PM
elexis updated the Trac tickets for this revision.Apr 11 2018, 5:19 PM
temple added a comment.Apr 12 2018, 3:38 AM
In D1439#58690, @elexis wrote:

I've used this to increase the WallPiece size to match the obstruction. That is good for the athen fortress, but for the great carthaginian walls, the obstruction size should be reduced instead:

The carthage obstruction probably shouldn't be any smaller though? Or definitely not going from 11 to 7.5 anyway.

Since D1445 solves the issue for the moment, I guess we can wait/abandon this patch. (I need to read the wall-building code in any case.)

wraitii added a reviewer: Restricted Owners Package.May 14 2018, 12:02 PM

Revision Contents
Changeset List

PathSize
binaries/
data/
mods/
public/
simulation/
templates/
structures/
2 lines
2 lines
2 lines
2 lines
4 lines

Diff 6332

View Options

binaries/data/mods/public/simulation/templates/structures/cart_wall_tower.xml

Loading...
View Options

binaries/data/mods/public/simulation/templates/structures/kush_wall_tower.xml

Loading...
View Options

binaries/data/mods/public/simulation/templates/structures/maur_wall_tower.xml

Loading...
View Options

binaries/data/mods/public/simulation/templates/structures/ptol_wall_tower.xml

Loading...
View Options

binaries/data/mods/public/simulation/templates/structures/rome_siege_wall_tower.xml

Loading...
Wildfire Games · Phabricator