Paths

Table of Contentst

Formations: GetClosestMember passes wrong entity to filter function
AcceptedPublic
Actions

Authored by Yves on Jan 27 2019, 4:38 PM.

Details

Reviewers

bb
Silier

Trac Tickets

#5120

Summary

The function is meant to filter out members that don't meet the filter criteria using a function.
It should pass the current member entity to the filter function and not the entity in the function argument list.

There's currently only one place in the code where the function is used with a filter. In UnitAI, it uses the "CanAttack" function to filter out entities that can't be attacked.
It filters out all entities there because it checks if an entity can attack itself rather than checking if the entity can attack the member of the enemy formation.

I've discovered this when I was experimenting with formations in my git branch. I'm unsure what the impact in SVN currently is.
In SVN we disband the formation when attacking and rarely/never attack a formation controller entity. The code might only be used if "CanAttackAsFormation" is true in the FormationAttack component. It's currently set to false for all formations.

Test Plan

The code is obviously wrong, but fixing it might still have some negative side effects.
A testcase should be found where issues can actually be reproduced. On the other hand, it's probably unused at the moment because the associated code is unfinished anyway.

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Lint Skipped

Unit

Unit Tests Skipped

Build Status

Buildable 6850
Build 11256: Vulcan Build	Jenkins

Event Timeline

Yves created this revision.Jan 27 2019, 4:38 PM

Silier awarded a token.Jan 27 2019, 4:45 PM

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required after '{'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
| 126| 126| 	this.formationMembersWithAura = []; // Members with a formation aura
| 127| 127| 	this.width = 0;
| 128| 128| 	this.depth = 0;
| 129|    |-	this.oldOrientation = {"sin": 0, "cos": 0};
|    | 129|+	this.oldOrientation = { "sin": 0, "cos": 0};
| 130| 130| 	this.twinFormations = [];
| 131| 131| 	// distance from which two twin formations will merge into one.
| 132| 132| 	this.formationSeparation = 0;
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required before '}'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
| 126| 126| 	this.formationMembersWithAura = []; // Members with a formation aura
| 127| 127| 	this.width = 0;
| 128| 128| 	this.depth = 0;
| 129|    |-	this.oldOrientation = {"sin": 0, "cos": 0};
|    | 129|+	this.oldOrientation = {"sin": 0, "cos": 0 };
| 130| 130| 	this.twinFormations = [];
| 131| 131| 	// distance from which two twin formations will merge into one.
| 132| 132| 	this.formationSeparation = 0;
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required after '{'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
| 144| 144| 
| 145| 145| Formation.prototype.GetSize = function()
| 146| 146| {
| 147|    |-	return {"width": this.width, "depth": this.depth};
|    | 147|+	return { "width": this.width, "depth": this.depth};
| 148| 148| };
| 149| 149| 
| 150| 150| Formation.prototype.GetSpeedMultiplier = function()
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required before '}'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
| 144| 144| 
| 145| 145| Formation.prototype.GetSize = function()
| 146| 146| {
| 147|    |-	return {"width": this.width, "depth": this.depth};
|    | 147|+	return {"width": this.width, "depth": this.depth };
| 148| 148| };
| 149| 149| 
| 150| 150| Formation.prototype.GetSpeedMultiplier = function()
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required after '{'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
| 563| 563| 			footprints.push(cmpFootprint.GetShape());
| 564| 564| 	}
| 565| 565| 	if (!footprints.length)
| 566|    |-		return {"width":1, "depth": 1};
|    | 566|+		return { "width":1, "depth": 1};
| 567| 567| 
| 568| 568| 	var r = {"width": 0, "depth": 0};
| 569| 569| 	for (var shape of footprints)
|    | [NORMAL] ESLintBear (key-spacing):
|    | Missing space before value for key 'width'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
| 563| 563| 			footprints.push(cmpFootprint.GetShape());
| 564| 564| 	}
| 565| 565| 	if (!footprints.length)
| 566|    |-		return {"width":1, "depth": 1};
|    | 566|+		return {"width": 1, "depth": 1};
| 567| 567| 
| 568| 568| 	var r = {"width": 0, "depth": 0};
| 569| 569| 	for (var shape of footprints)
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required before '}'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
| 563| 563| 			footprints.push(cmpFootprint.GetShape());
| 564| 564| 	}
| 565| 565| 	if (!footprints.length)
| 566|    |-		return {"width":1, "depth": 1};
|    | 566|+		return {"width":1, "depth": 1 };
| 567| 567| 
| 568| 568| 	var r = {"width": 0, "depth": 0};
| 569| 569| 	for (var shape of footprints)
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required after '{'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
| 565| 565| 	if (!footprints.length)
| 566| 566| 		return {"width":1, "depth": 1};
| 567| 567| 
| 568|    |-	var r = {"width": 0, "depth": 0};
|    | 568|+	var r = { "width": 0, "depth": 0};
| 569| 569| 	for (var shape of footprints)
| 570| 570| 	{
| 571| 571| 		if (shape.type == "circle")
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required before '}'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
| 565| 565| 	if (!footprints.length)
| 566| 566| 		return {"width":1, "depth": 1};
| 567| 567| 
| 568|    |-	var r = {"width": 0, "depth": 0};
|    | 568|+	var r = {"width": 0, "depth": 0 };
| 569| 569| 	for (var shape of footprints)
| 570| 570| 	{
| 571| 571| 		if (shape.type == "circle")
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'for-of'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
| 567| 567| 
| 568| 568| 	var r = {"width": 0, "depth": 0};
| 569| 569| 	for (var shape of footprints)
| 570|    |-	{
|    | 570|+	
| 571| 571| 		if (shape.type == "circle")
| 572| 572| 		{
| 573| 573| 			r.width += shape.radius * 2;
| 578| 578| 			r.width += shape.width;
| 579| 579| 			r.depth += shape.depth;
| 580| 580| 		}
| 581|    |-	}
|    | 581|+	
| 582| 582| 	r.width /= footprints.length;
| 583| 583| 	r.depth /= footprints.length;
| 584| 584| 	return r;
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
| 591| 591| 	separation.depth *= this.separationMultiplier.depth;
| 592| 592| 
| 593| 593| 	if (this.columnar)
| 594|    |-		var sortingClasses = ["Cavalry","Infantry"];
|    | 594|+		var sortingClasses = ["Cavalry", "Infantry"];
| 595| 595| 	else
| 596| 596| 		var sortingClasses = this.sortingClasses.slice();
| 597| 597| 	sortingClasses.push("Unknown");
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'for' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
| 608| 608| 		var classes = cmpIdentity.GetClassesList();
| 609| 609| 		var done = false;
| 610| 610| 		for (var c = 0; c < sortingClasses.length; ++c)
| 611|    |-		{
|    | 611|+		
| 612| 612| 			if (classes.indexOf(sortingClasses[c]) > -1)
| 613| 613| 			{
| 614| 614| 				types[sortingClasses[c]].push({"ent": active[i], "pos": positions[i]});
| 615| 615| 				done = true;
| 616| 616| 				break;
| 617| 617| 			}
| 618|    |-		}
|    | 618|+		
| 619| 619| 		if (!done)
| 620| 620| 			types["Unknown"].push({"ent": active[i], "pos": positions[i]});
| 621| 621| 	}
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required after '{'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
| 611| 611| 		{
| 612| 612| 			if (classes.indexOf(sortingClasses[c]) > -1)
| 613| 613| 			{
| 614|    |-				types[sortingClasses[c]].push({"ent": active[i], "pos": positions[i]});
|    | 614|+				types[sortingClasses[c]].push({ "ent": active[i], "pos": positions[i]});
| 615| 615| 				done = true;
| 616| 616| 				break;
| 617| 617| 			}
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required before '}'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
| 611| 611| 		{
| 612| 612| 			if (classes.indexOf(sortingClasses[c]) > -1)
| 613| 613| 			{
| 614|    |-				types[sortingClasses[c]].push({"ent": active[i], "pos": positions[i]});
|    | 614|+				types[sortingClasses[c]].push({"ent": active[i], "pos": positions[i] });
| 615| 615| 				done = true;
| 616| 616| 				break;
| 617| 617| 			}
|    | [NORMAL] ESLintBear (dot-notation):
|    | ["Unknown"] is better written in dot notation.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
| 617| 617| 			}
| 618| 618| 		}
| 619| 619| 		if (!done)
| 620|    |-			types["Unknown"].push({"ent": active[i], "pos": positions[i]});
|    | 620|+			types.Unknown.push({"ent": active[i], "pos": positions[i]});
| 621| 621| 	}
| 622| 622| 
| 623| 623| 	var count = active.length;
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required after '{'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
| 617| 617| 			}
| 618| 618| 		}
| 619| 619| 		if (!done)
| 620|    |-			types["Unknown"].push({"ent": active[i], "pos": positions[i]});
|    | 620|+			types["Unknown"].push({ "ent": active[i], "pos": positions[i]});
| 621| 621| 	}
| 622| 622| 
| 623| 623| 	var count = active.length;
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required before '}'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
| 617| 617| 			}
| 618| 618| 		}
| 619| 619| 		if (!done)
| 620|    |-			types["Unknown"].push({"ent": active[i], "pos": positions[i]});
|    | 620|+			types["Unknown"].push({"ent": active[i], "pos": positions[i] });
| 621| 621| 	}
| 622| 622| 
| 623| 623| 	var count = active.length;
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
| 635| 635| 	if (this.columnar)
| 636| 636| 	{
| 637| 637| 		shape = "square";
| 638|    |-		cols = Math.min(count,3);
|    | 638|+		cols = Math.min(count, 3);
| 639| 639| 		shiftRows = false;
| 640| 640| 		centerGap = 0;
| 641| 641| 		sortingOrder = null;
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
| 689| 689| 					n -= r%2;
| 690| 690| 			}
| 691| 691| 			else if (shape == "triangle")
| 692|    |-			{
|    | 692|+			
| 693| 693| 				if (shiftRows)
| 694| 694| 					var n = r + 1;
| 695| 695| 				else
| 696| 696| 					var n = r * 2 + 1;
| 697|    |-			}
|    | 697|+			
| 698| 698| 			if (!shiftRows && n > left)
| 699| 699| 				n = left;
| 700| 700| 			for (var c = 0; c < n && left > 0; ++c)
|    | [NORMAL] ESLintBear (space-before-function-paren):
|    | Unexpected space before function parentheses.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
| 730| 730| 	// calculating offset distances without a zero average makes no sense, as the formation
| 731| 731| 	// will jump to a different position any time
| 732| 732| 	var avgoffset = Vector2D.average(offsets);
| 733|    |-	offsets.forEach(function (o) {o.sub(avgoffset);});
|    | 733|+	offsets.forEach(function(o) {o.sub(avgoffset);});
| 734| 734| 
| 735| 735| 	// sort the available places in certain ways
| 736| 736| 	// the places first in the list will contain the heaviest units as defined by the order
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required after '{'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
| 789| 789| 			closestOffsetId = i;
| 790| 790| 		}
| 791| 791| 	}
| 792|    |-	this.memberPositions[entPos.ent] = {"row": offsets[closestOffsetId].row, "column":offsets[closestOffsetId].column};
|    | 792|+	this.memberPositions[entPos.ent] = { "row": offsets[closestOffsetId].row, "column":offsets[closestOffsetId].column};
| 793| 793| 	return closestOffsetId;
| 794| 794| };
| 795| 795| 
|    | [NORMAL] ESLintBear (key-spacing):
|    | Missing space before value for key 'column'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
| 789| 789| 			closestOffsetId = i;
| 790| 790| 		}
| 791| 791| 	}
| 792|    |-	this.memberPositions[entPos.ent] = {"row": offsets[closestOffsetId].row, "column":offsets[closestOffsetId].column};
|    | 792|+	this.memberPositions[entPos.ent] = {"row": offsets[closestOffsetId].row, "column": offsets[closestOffsetId].column};
| 793| 793| 	return closestOffsetId;
| 794| 794| };
| 795| 795| 
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required before '}'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
| 789| 789| 			closestOffsetId = i;
| 790| 790| 		}
| 791| 791| 	}
| 792|    |-	this.memberPositions[entPos.ent] = {"row": offsets[closestOffsetId].row, "column":offsets[closestOffsetId].column};
|    | 792|+	this.memberPositions[entPos.ent] = {"row": offsets[closestOffsetId].row, "column":offsets[closestOffsetId].column };
| 793| 793| 	return closestOffsetId;
| 794| 794| };
| 795| 795| 
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required after '{'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
| 799| 799| Formation.prototype.GetRealOffsetPositions = function(offsets, pos)
| 800| 800| {
| 801| 801| 	var offsetPositions = [];
| 802|    |-	var {sin, cos} = this.GetEstimatedOrientation(pos);
|    | 802|+	var { sin, cos} = this.GetEstimatedOrientation(pos);
| 803| 803| 	// calculate the world positions
| 804| 804| 	for (var o of offsets)
| 805| 805| 		offsetPositions.push(new Vector2D(pos.x + o.y * sin + o.x * cos, pos.y + o.y * cos - o.x * sin));
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required before '}'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
| 799| 799| Formation.prototype.GetRealOffsetPositions = function(offsets, pos)
| 800| 800| {
| 801| 801| 	var offsetPositions = [];
| 802|    |-	var {sin, cos} = this.GetEstimatedOrientation(pos);
|    | 802|+	var {sin, cos } = this.GetEstimatedOrientation(pos);
| 803| 803| 	// calculate the world positions
| 804| 804| 	for (var o of offsets)
| 805| 805| 		offsetPositions.push(new Vector2D(pos.x + o.y * sin + o.x * cos, pos.y + o.y * cos - o.x * sin));
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required after '{'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
| 816| 816| Formation.prototype.GetEstimatedOrientation = function(pos)
| 817| 817| {
| 818| 818| 	var cmpUnitAI = Engine.QueryInterface(this.entity, IID_UnitAI);
| 819|    |-	var r = {"sin": 0, "cos": 1};
|    | 819|+	var r = { "sin": 0, "cos": 1};
| 820| 820| 	var unitAIState = cmpUnitAI.GetCurrentState();
| 821| 821| 	if (unitAIState == "FORMATIONCONTROLLER.WALKING" || unitAIState == "FORMATIONCONTROLLER.COMBAT.APPROACHING")
| 822| 822| 	{
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required before '}'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
| 816| 816| Formation.prototype.GetEstimatedOrientation = function(pos)
| 817| 817| {
| 818| 818| 	var cmpUnitAI = Engine.QueryInterface(this.entity, IID_UnitAI);
| 819|    |-	var r = {"sin": 0, "cos": 1};
|    | 819|+	var r = {"sin": 0, "cos": 1 };
| 820| 820| 	var unitAIState = cmpUnitAI.GetCurrentState();
| 821| 821| 	if (unitAIState == "FORMATIONCONTROLLER.WALKING" || unitAIState == "FORMATIONCONTROLLER.COMBAT.APPROACHING")
| 822| 822| 	{
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Formation.js
| 898| 898| 		cmpOtherFormation.RemoveMembers(otherMembers);
| 899| 899| 		this.AddMembers(otherMembers);
| 900| 900| 		Engine.DestroyEntity(this.twinFormations[i]);
| 901|    |-		this.twinFormations.splice(i,1);
|    | 901|+		this.twinFormations.splice(i, 1);
| 902| 902| 	}
| 903| 903| 	// Switch between column and box if necessary
| 904| 904| 	var cmpUnitAI = Engine.QueryInterface(this.entity, IID_UnitAI);

binaries/data/mods/public/simulation/components/Formation.js
| 335| »   for·(var·ent·of·this.formationMembersWithAura)
|    | [NORMAL] JSHintBear:
|    | 'ent' is already defined.

binaries/data/mods/public/simulation/components/Formation.js
| 422| »   for·(var·ent·of·this.formationMembersWithAura)
|    | [NORMAL] JSHintBear:
|    | 'ent' is already defined.

binaries/data/mods/public/simulation/components/Formation.js
| 468| »   var·cmpPosition·=·Engine.QueryInterface(this.entity,·IID_Position);
|    | [NORMAL] JSHintBear:
|    | 'cmpPosition' is already defined.

binaries/data/mods/public/simulation/components/Formation.js
| 509| »   »   var·cmpUnitAI·=·Engine.QueryInterface(offset.ent,·IID_UnitAI);
|    | [NORMAL] JSHintBear:
|    | 'cmpUnitAI' is already defined.

binaries/data/mods/public/simulation/components/Formation.js
| 544| »   var·cmpPosition·=·Engine.QueryInterface(this.entity,·IID_Position);
|    | [NORMAL] JSHintBear:
|    | 'cmpPosition' is already defined.

binaries/data/mods/public/simulation/components/Formation.js
| 596| »   »   var·sortingClasses·=·this.sortingClasses.slice();
|    | [NORMAL] JSHintBear:
|    | 'sortingClasses' is already defined.

binaries/data/mods/public/simulation/components/Formation.js
| 605| »   for·(var·i·in·active)
|    | [NORMAL] JSHintBear:
|    | 'i' is already defined.

binaries/data/mods/public/simulation/components/Formation.js
| 620| »   »   »   types["Unknown"].push({"ent":·active[i],·"pos":·positions[i]});
|    | [NORMAL] JSHintBear:
|    | ['Unknown'] is better written in dot notation.

binaries/data/mods/public/simulation/components/Formation.js
| 660| »   »   for·(var·i·=·0;·i·<·count;·++i)
|    | [NORMAL] JSHintBear:
|    | 'i' is already defined.

binaries/data/mods/public/simulation/components/Formation.js
| 694| »   »   »   »   »   var·n·=·r·+·1;
|    | [NORMAL] JSHintBear:
|    | 'n' is already defined.

binaries/data/mods/public/simulation/components/Formation.js
| 696| »   »   »   »   »   var·n·=·r·*·2·+·1;
|    | [NORMAL] JSHintBear:
|    | 'n' is already defined.

binaries/data/mods/public/simulation/components/Formation.js
| 700| »   »   »   for·(var·c·=·0;·c·<·n·&&·left·>·0;·++c)
|    | [NORMAL] JSHintBear:
|    | 'c' is already defined.

binaries/data/mods/public/simulation/components/Formation.js
| 707| »   »   »   »   »   var·x·=·side·*·Math.ceil(c/2)·*·separation.width;
|    | [NORMAL] JSHintBear:
|    | 'x' is already defined.

binaries/data/mods/public/simulation/components/Formation.js
| 710| »   »   »   »   »   if·(x·==·0)·//·don't·use·the·center·position·with·a·center·gap
|    | [NORMAL] JSHintBear:
|    | 'x' used out of scope.

binaries/data/mods/public/simulation/components/Formation.js
| 712| »   »   »   »   »   x·+=·side·*·centerGap·/·2;
|    | [NORMAL] JSHintBear:
|    | 'x' used out of scope.

binaries/data/mods/public/simulation/components/Formation.js
| 718| »   »   »   »   offsets.push(new·Vector2D(x·+·r1,·z·+·r2));
|    | [NORMAL] JSHintBear:
|    | 'x' used out of scope.

binaries/data/mods/public/simulation/components/Formation.js
| 698| »   »   »   if·(!shiftRows·&&·n·>·left)
|    | [NORMAL] JSHintBear:
|    | 'n' used out of scope.

binaries/data/mods/public/simulation/components/Formation.js
| 699| »   »   »   »   n·=·left;
|    | [NORMAL] JSHintBear:
|    | 'n' used out of scope.

binaries/data/mods/public/simulation/components/Formation.js
| 700| »   »   »   for·(var·c·=·0;·c·<·n·&&·left·>·0;·++c)
|    | [NORMAL] JSHintBear:
|    | 'n' used out of scope.

binaries/data/mods/public/simulation/components/Formation.js
| 704| »   »   »   »   if·(n%2·==·0)
|    | [NORMAL] JSHintBear:
|    | 'n' used out of scope.

binaries/data/mods/public/simulation/components/Formation.js
| 714| »   »   »   »   var·column·=·Math.ceil(n/2)·+·Math.ceil(c/2)·*·side;
|    | [NORMAL] JSHintBear:
|    | 'n' used out of scope.

binaries/data/mods/public/simulation/components/Formation.js
| 724| »   »   »   this.maxColumnsUsed[r]·=·n;
|    | [NORMAL] JSHintBear:
|    | 'n' used out of scope.

binaries/data/mods/public/simulation/components/Formation.js
| 753| »   for·(var·i·=·sortingClasses.length;·i;·--i)
|    | [NORMAL] JSHintBear:
|    | 'i' is already defined.

binaries/data/mods/public/simulation/components/Formation.js
| 597| »   sortingClasses.push("Unknown");
|    | [NORMAL] JSHintBear:
|    | 'sortingClasses' used out of scope.

binaries/data/mods/public/simulation/components/Formation.js
| 602| »   for·(var·i·=·0;·i·<·sortingClasses.length;·++i)
|    | [NORMAL] JSHintBear:
|    | 'sortingClasses' used out of scope.

binaries/data/mods/public/simulation/components/Formation.js
| 603| »   »   types[sortingClasses[i]]·=·[];
|    | [NORMAL] JSHintBear:
|    | 'sortingClasses' used out of scope.

binaries/data/mods/public/simulation/components/Formation.js
| 610| »   »   for·(var·c·=·0;·c·<·sortingClasses.length;·++c)
|    | [NORMAL] JSHintBear:
|    | 'sortingClasses' used out of scope.

binaries/data/mods/public/simulation/components/Formation.js
| 612| »   »   »   if·(classes.indexOf(sortingClasses[c])·>·-1)
|    | [NORMAL] JSHintBear:
|    | 'sortingClasses' used out of scope.

binaries/data/mods/public/simulation/components/Formation.js
| 614| »   »   »   »   types[sortingClasses[c]].push({"ent":·active[i],·"pos":·positions[i]});
|    | [NORMAL] JSHintBear:
|    | 'sortingClasses' used out of scope.

binaries/data/mods/public/simulation/components/Formation.js
| 753| »   for·(var·i·=·sortingClasses.length;·i;·--i)
|    | [NORMAL] JSHintBear:
|    | 'sortingClasses' used out of scope.

binaries/data/mods/public/simulation/components/Formation.js
| 755| »   »   var·t·=·types[sortingClasses[i-1]];
|    | [NORMAL] JSHintBear:
|    | 'sortingClasses' used out of scope.

binaries/data/mods/public/simulation/components/Formation.js
| 860| »   var·cmpUnitMotion·=·Engine.QueryInterface(this.entity,·IID_UnitMotion);
|    | [NORMAL] JSHintBear:
|    | 'cmpUnitMotion' is already defined.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1006/

Harbormaster completed remote builds in B6850: Diff 7404.Jan 27 2019, 5:08 PM

Simple change, which is correct so can be committed.

This revision is now accepted and ready to land.Jan 28 2019, 9:54 AM

The last time someone has worked on formations, it was @temple.

Determining the impact would allow us to determine whether there are formation tickets that we could close.
There are 4 calls to GetClosestMember:

two of them are supposed to get a member and they might receive the formation controller entity. The controller entity is invincible if I recall correctly? So the bug visible to the user would have been units attacking nothingness / invincible enemies.
the other two calls are 'MoveToTargetAttackRange` / CheckTargetAttackRange and would move to the controller entitys, position which is somewhere near the members, but possibly out of attack range (temple had a patch to display it with a flag D1447).

At least all formation tickets I recall were about formation members being stuck, doing backflips and saltos instead of moving or attacking, so maybe there wasn't a related ticket.
Thanks for the patch.

From these 4 calls, only one passes a filter argument to the function. The other 3 can be ignored because of the check "if (filter ...". The condition after that has no impact when filter is undefined.

I've looked at the code a bit more. There actually is a case when this function is called with a filter.
It happens in the "ATTACKING" timer function when a formation is ordered to attack another formation and only if the target formation is already in range.
The code incorrectly assumes that there are no more targets to find and calls FinishOrder() plus FindNewTargets().

In general the formation code is currently a mess because half of it implements the current behaviour and the other half is code to implement some unclear future behaviour. Someone had an idea how it could work in the future, committed parts of it and then stopped working on it.
For example, there are many cases where the code falls back from formation attack to individual attack. If the formation is not in range when ordered to attack, it calls MoveToTargetAttackRange() which resets the target to the member unit rather than the formation controller. This means that the code for picking new targets from the enemy formation doesn't work anymore and the unit picks any target instead. Something similar happens here, too. GetClosestMember() returns INVALID_ENDITY and then by calling FindNewTargets(), we revert to general attack behaviour.

So basically it's just another case where a fallback to individual unit attack behaviour happens and since that's common anyway and hard to notice, it probably wasn't perceived as a bug.

In general, I wouldn't want to start changing the formation code in SVN before we have a working prototype of a new formation system. This case is special because it's a relatively simple change and clearly a bug.
What do you think? Commit or not?

In D1768#71585, @Yves wrote:

In general, I wouldn't want to start changing the formation code in SVN before we have a working prototype of a new formation system.

In general there's the reason I tried to avoid looking into formations :P To determine how to fix one feature one has to determine how 10 related features should be fixed, for that we might have to know about a total 100 indirectly related features, .... If the features are not implemented and not properly predesigned, one has to design these in order to decide on the original fix. If one arrives at that point, one might just as well finish typing it out and compiling it for few months to implement all of that, even though one just wanted to fix one little bug.

This case is special because it's a relatively simple change and clearly a bug.
What do you think? Commit or not?

"Stakeholders":

Devs: Where one has to work with unknown future constraints, making the current behavior more consistent can make it easier to implement the unforeseen constraint in the future.
Author: Might have to revisit the patch months later in case there's a bug discovered.
Players: Making the formation behavior more consistent while fixing bugs sound good to the player in principle. Sometimes simulation changes can also break the gameplay in unexpected ways or trigger unexpected infinite loops (UnitAI). If one did a code walkthrough like you did, one can try to determine the exact consequences to the player.

So I can't speak about the diff in specific. I have insufficient knowledge to decide on the implementation or feature design, you will know better (and if there was a bugreport spammed next release, we will even know more for the release after that. Though usually the formation bugreports never make it into the reproductive debugging stage, at least for the time when temple is not involved).

(TLDR: good faith and empty phrases)

We should realise that current behaviour returns only INVALID ENTITY if filter is passed what is cmpAttack. CanAttack and there is check if target is enemy esle returns false. So clearly unit cannot attack itself as is not enemy of itself.

Uses of Formation.prototype.GetClosestMember
UnitAI.js
Line 184 - called without filter
Line 1920 - called with filter UnitAi.CanAttack(entity)
Line 4303 - called without filter
Line 4405 - called without fiter

Only case which actually matters for this change is on line 1920.
If attacker is formation controller, answer is always true.

Real filter function which makes decision is located in Attack component.
Current implementation with passing attacker entity into filter makes filter return false because of this checks looping attack types of attacker where in current state targetOwner is attackerOwner so player is not enemy and also not capturable so both
conditions force continue therefore final result is return false

if (type != "Capture" && (!cmpEntityPlayer.IsEnemy(targetOwner) || !cmpHealth || !cmpHealth.GetHitpoints()))
continue;

if (type == "Capture" && (!cmpCapturable || !cmpCapturable.CanCapture(entityOwner)))
continue;

which means that current filter usage with !filter(ent) means continue for every one member of this.members therefor return of GetClosestMember is INVALID_ENTITY

with this patch, filter returns closest member of this formation as intended.

Now
What does it mean for ATTACKING.Timer
This is called in case unit gets formation entity as target so target is switched to formation member which can be attacked and because it is valid entity, there is no harm to other parts of code.
(next command makes this canAttack check again anyway so double check on entity, I think it is safe)

But in case GetClosestMember returns INVALID_ENTITY (what is doing right now so no reason to worry because it cannot brake it by returning sometimes value which is returning every time now) is called cmpAttack.CanAttack what means
check against cmpPostion which does not exists on INVALID_ENTITY therefor return false

Well target is set to 0 = INVALID_ENTITY what means code if (target && cmpTargetFormation) which resets this.order.data.target back to formation entity is not executed, that means next order is to find new targets and
do not attack given formation controller again and proceeds as in any other case.

All things considered this patch can be committed without breaking anything.

@elexis could you commit this change?

Once someone starts doing anything with formations, having such a big around can only be annoying, so rather fix it beforehand.

In D1768#71234, @elexis wrote:

The last time someone has worked on formations, it was @temple.

https://code.wildfiregames.com/rP14638

Freagarach mentioned this in D2137: Fix GetClosestMember function of Formation..Jul 31 2019, 10:50 AM

It seems that the problem causing the segfault found in D1423 regarding this function has been fixed, hence this function can be fixed.
I've tested some of my own and could not generate a segfault. The replay attached here also does not generate a segfault (but is quite old therefore is no guarantee).

Freagarach updated the Trac tickets for this revision.Sep 27 2019, 8:05 AM

Since several svn changes in unitAi, I still can see broken behaviour when the same target can be chosen in infinite loop because stance is not taken into account. Think about hold ground and last reachable formation member just died, so even closest formation member exists, it will be refused to chase over and over again, mainly because that part done by state switch and no passing new attack order.

This revision now requires changes to proceed.Jan 27 2020, 6:40 PM

Freagarach resigned from this revision.Jan 27 2020, 7:20 PM

Freagarach removed a reviewer: Freagarach.

@Angen @Freagarach is this still needed ?

yes

I fail to see how the bug you describe relates to this patch directly. I can see that choosing the same formation member over and over again can cause an infinite loop, however this patch won't cause that. In fact with this bug fixed one could fix it: if there are members failing for whatever reason, they can be excluded by the filter.

Silier resigned from this revision.Jul 31 2023, 5:27 PM

This revision is now accepted and ready to land.Jul 31 2023, 5:27 PM

Revision Contents
Changeset List

Path

Size

binaries/

data/

mods/

public/

simulation/

components/

Formation.js

2 lines

Diff 7404

View Options

binaries/data/mods/public/simulation/components/Formation.js

Formations: GetClosestMember passes wrong entity to filter functionAcceptedPublicActions