Page MenuHomeWildfire Games

Destroy sheep corpses when constructing buildings
ClosedPublic

Authored by elexis on Dec 29 2016, 3:49 PM.

Details

Summary

Ported from #4268 (as is so far, it needs some work cf itms comment)

Test Plan

Review behaviour and code.

Diff Detail

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

Event Timeline

wraitii updated this revision to Diff 71.Dec 29 2016, 3:49 PM
wraitii retitled this revision from to Destroy sheep corpses when constructing buildings.
wraitii updated this object.
wraitii edited the test plan for this revision. (Show Details)
wraitii set the repository for this revision to rP 0 A.D. Public Repository.
Vulcan added a subscriber: Vulcan.Dec 29 2016, 4:33 PM

Build has FAILED

Updating workspaces.
Build (release)...
../../../source/simulation2/components/CCmpObstructionManager.cpp:1056:161: warning: unused parameter ‘strict’ [-Wunused-parameter]
 void CCmpObstructionManager::GetStaticsOnObstruction(const ObstructionSquare& square, std::vector<entity_id_t>& out, const IObstructionTestFilter& filter, bool strict)
                                                                                                                                                                 ^
../../../source/gui/CChart.cpp:38:41: warning: unused parameter ‘Message’ [-Wunused-parameter]
 void CChart::HandleMessage(SGUIMessage& Message)
                                         ^
../../../source/lib/tex/tex_png.cpp: In member function ‘virtual Status TexCodecPng::encode(Tex*, DynArray*) const’:
../../../source/lib/tex/tex_png.cpp:309:9: warning: variable ‘ret’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Wclobbered]
  Status ret = ERR::FAIL;
         ^
Build (debug)...
../../../source/simulation2/components/CCmpObstructionManager.cpp:1056:161: warning: unused parameter ‘strict’ [-Wunused-parameter]
 void CCmpObstructionManager::GetStaticsOnObstruction(const ObstructionSquare& square, std::vector<entity_id_t>& out, const IObstructionTestFilter& filter, bool strict)
                                                                                                                                                                 ^
../../../source/gui/CChart.cpp:38:41: warning: unused parameter ‘Message’ [-Wunused-parameter]
 void CChart::HandleMessage(SGUIMessage& Message)
                                         ^
Running debug tests...
Running cxxtest tests (302 tests).ERROR: JavaScript error: simulation/components/Foundation.js line 182
TypeError: cmpObstruction.GetCollisions is not a function
  Foundation.prototype.Build@simulation/components/Foundation.js:182:20
  testFoundation@simulation/components/tests/test_Foundation.js:151:2
  @simulation/components/tests/test_Foundation.js:166:1

In TestComponentScripts::test_scripts:
/mnt/data/jenkins/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Test failed: L"Running script simulation/components/tests/test_Foundation.js"
/mnt/data/jenkins/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Assertion failed: scriptInterface.LoadScript(pathname, content)
............................................................................................................................................................................................................................................................................................................
Failed 1 and Skipped 0 of 302 tests
Success rate: 99%

Link to build: http://jw:8080/job/phabricator/42/
See console output for more information: http://jw:8080/job/phabricator/42/console

Itms requested changes to this revision.Feb 19 2017, 4:59 PM
Itms added a subscriber: Itms.

Putting that out of the queue since it didn't build (maybe it was because of my attempts at the time), and my comments on the ticket have not been addressed yet.

This revision now requires changes to proceed.Feb 19 2017, 4:59 PM
elexis changed the visibility from "All Users" to "Public (No Login Required)".Apr 22 2017, 2:26 PM
elexis added a subscriber: elexis.Mar 19 2018, 2:37 PM

Putting that out of the queue since it didn't build (maybe it was because of my attempts at the time), and my comments on the ticket have not been addressed yet.

https://trac.wildfiregames.com/ticket/4268#comment:11

So basically hundreds of thousands of players can enjoy this bug because of some comments and variable names that could be improved?
That sounds like an easy task to finish then for one of the worst pathfinding bugs we have in the game.

elexis commandeered this revision.Mar 19 2018, 5:04 PM
elexis planned changes to this revision.
elexis added a reviewer: wraitii.
elexis updated this revision to Diff 6222.Mar 19 2018, 5:41 PM

Rebased patch.
Doesn't implement Itms remarks yet.
Changes some auto to a map iterator.

elexis planned changes to this revision.Mar 19 2018, 5:41 PM

Responding to Itms review in https://trac.wildfiregames.com/ticket/4268#comment:11,

Style fixes:

 the PROFILE call has a typo in the name
 the split line in GetStaticsOnObstruction has bad indentation (coming from copy-paste), see the other function and copy that indentation.
remove the comment in GetAllOnObstruction
 you didn't replace all occurrences in the public mod of GetUnitCollisions that you removed from the API

and

explain in the first comment that living animals have the flag while dead ones are static obstructions without the flag
Then I would add a comment in the Foundation.js code to explain that this code cleans everything under the foundation (corpses and other non foundation-blocking obstructions).

are done in the next upload.

But about:

For me the big problem of this patch is this boolean passed to GetCollisions which has an obfuscated name. It should be clear in the API what this boolean does (i.e. without having to understand the whole code in the Obstruction component).
I suggest calling the boolean onlyConstructionBlocking (and probably making it optional, with a default value of true). Then comments should be detailed more in the function
Add a remark that if the boolean is false, the filter filters nothing and is equivalent to NullObstructionFilter.

I agree that it should become easier for the reader of the JS simulation code (imagine triggerscript authors that aren't used to coding).
But even better than a function passing a weird boolean were two functions that state what they do in the name.

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

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (operator-linebreak):
|    | '&&' should be placed at the end of the line.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Foundation.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Foundation.js
| 211| 211| 					// If obstructing fauna is gaia or our own but doesn't have UnitAI, just destroy it
| 212| 212| 					var cmpOwnership = Engine.QueryInterface(ent, IID_Ownership);
| 213| 213| 					var cmpIdentity = Engine.QueryInterface(ent, IID_Identity);
| 214|    |-					if (cmpOwnership && cmpIdentity && cmpIdentity.HasClass("Animal")
| 215|    |-						&& (cmpOwnership.GetOwner() == 0 || cmpFoundationOwnership && cmpOwnership.GetOwner() == cmpFoundationOwnership.GetOwner()))
|    | 214|+					if (cmpOwnership && cmpIdentity && cmpIdentity.HasClass("Animal") &&
|    | 215|+						(cmpOwnership.GetOwner() == 0 || cmpFoundationOwnership && cmpOwnership.GetOwner() == cmpFoundationOwnership.GetOwner()))
| 216| 216| 						Engine.DestroyEntity(ent);
| 217| 217| 				}
| 218| 218| 

binaries/data/mods/public/simulation/components/Foundation.js
| 361| »   »   »   let·cmpOwnership·=·Engine.QueryInterface(this.entity,·IID_Ownership);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'cmpOwnership' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/Foundation.js
| 215| »   »   »   »   »   »   &&·(cmpOwnership.GetOwner()·==·0·||·cmpFoundationOwnership·&&·cmpOwnership.GetOwner()·==·cmpFoundationOwnership.GetOwner()))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/Foundation.js
| 256| »   »   »   var·cmpFoundationOwnership·=·Engine.QueryInterface(this.entity,·IID_Ownership);
|    | [NORMAL] JSHintBear:
|    | 'cmpFoundationOwnership' is already defined.

binaries/data/mods/public/simulation/components/Foundation.js
| 330| »   »   var·pos·=·cmpPosition.GetPosition2D();
|    | [NORMAL] JSHintBear:
|    | 'pos' is already defined.

binaries/data/mods/public/simulation/components/Foundation.js
| 332| »   »   var·rot·=·cmpPosition.GetRotation();
|    | [NORMAL] JSHintBear:
|    | 'rot' is already defined.

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

elexis updated this revision to Diff 6224.Mar 19 2018, 6:59 PM

Add Itms remarks, but add a new function instead of describing the odd boolean argument.
Inline the two lines of GetObstructionsOnObstruction that are used only there.

I'm dubious about using the NullObstructionFilter in GetUnitCollisions. This could be done in a separate revision or if someone more experienced with the PF claims this to be safe.

Add a remark that if the boolean is false, the filter filters nothing and is equivalent to NullObstructionFilter.

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

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (operator-linebreak):
|    | '&&' should be placed at the end of the line.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Foundation.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Foundation.js
| 212| 212| 					// If obstructing fauna is gaia or our own but doesn't have UnitAI, just destroy it
| 213| 213| 					var cmpOwnership = Engine.QueryInterface(ent, IID_Ownership);
| 214| 214| 					var cmpIdentity = Engine.QueryInterface(ent, IID_Identity);
| 215|    |-					if (cmpOwnership && cmpIdentity && cmpIdentity.HasClass("Animal")
| 216|    |-						&& (cmpOwnership.GetOwner() == 0 || cmpFoundationOwnership && cmpOwnership.GetOwner() == cmpFoundationOwnership.GetOwner()))
|    | 215|+					if (cmpOwnership && cmpIdentity && cmpIdentity.HasClass("Animal") &&
|    | 216|+						(cmpOwnership.GetOwner() == 0 || cmpFoundationOwnership && cmpOwnership.GetOwner() == cmpFoundationOwnership.GetOwner()))
| 217| 217| 						Engine.DestroyEntity(ent);
| 218| 218| 				}
| 219| 219| 

binaries/data/mods/public/simulation/components/Foundation.js
| 362| »   »   »   let·cmpOwnership·=·Engine.QueryInterface(this.entity,·IID_Ownership);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'cmpOwnership' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/Foundation.js
| 216| »   »   »   »   »   »   &&·(cmpOwnership.GetOwner()·==·0·||·cmpFoundationOwnership·&&·cmpOwnership.GetOwner()·==·cmpFoundationOwnership.GetOwner()))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/Foundation.js
| 257| »   »   »   var·cmpFoundationOwnership·=·Engine.QueryInterface(this.entity,·IID_Ownership);
|    | [NORMAL] JSHintBear:
|    | 'cmpFoundationOwnership' is already defined.

binaries/data/mods/public/simulation/components/Foundation.js
| 331| »   »   var·pos·=·cmpPosition.GetPosition2D();
|    | [NORMAL] JSHintBear:
|    | 'pos' is already defined.

binaries/data/mods/public/simulation/components/Foundation.js
| 333| »   »   var·rot·=·cmpPosition.GetRotation();
|    | [NORMAL] JSHintBear:
|    | 'rot' is already defined.

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

Stan added a subscriber: Stan.Mar 19 2018, 7:18 PM
Stan added inline comments.
binaries/data/mods/public/simulation/components/Foundation.js
200

Let and maybe early return ?

Seems to work and it's a good change.

(In general, we should have a way for units to give up their unit motion attempt if they're not making progress.)

Whatever's done in Foundation should be done in Transform too, I assume.

source/simulation2/components/CCmpObstructionManager.cpp
1056

(Btw, I don't know why this rasterizes rather than uses Geometry. There's a weird thing with formations where you place a foundation where the unit currently is and they will move to the outside to start building, but then once there the foundation makes them move away again! So why didn't they move to the correct distance the first time? So either a problem with this function or a problem with inverted goals, or both. And then #5048 too.

I guess it's used in UnitMotion with long paths where it might make sense (although I'm not a fan of that section of code). So maybe we need two versions of the function? But anyway..)

1098

don't think this is needed (can't hurt, but still)

1101

unused

1113

missing a couple spaces

elexis marked 4 inline comments as done.Mar 20 2018, 11:54 AM

In general, we should have a way for units to give up their unit motion attempt if they're not making progress.

Entirely true for the player. These extremely annoying units on the other hand have the advantage that they allow us to notice these bugs much more easily.
(Wondering if there still won't be a neverending loop of units trying to gather the impassable resource, giving up, trying again, anyway out of scope)

Whatever's done in Foundation should be done in Transform too, I assume.

Agree, an entity may not be upgraded until all unit and static obstructions are removed in the affected area.

The only other GetUnitCollisions call is at Gate.CloseGate. Sheep corpses should be ignored and static obstructions of buildings can't be on the obstruction of the gate (otherwise how would the gate have been placed there).

Also fixed test.

binaries/data/mods/public/simulation/components/Foundation.js
200

(Early return would be bad)

source/simulation2/components/CCmpObstructionManager.cpp
1056
1098

Because static obstructions don't have a clearance, yes. Tested briefly.

elexis updated this revision to Diff 6235.Mar 20 2018, 11:55 AM
elexis marked 2 inline comments as done.

Transform.js uses it too, whitespace, fixed test, remove unused.

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

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/helpers/Transform.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/helpers/Transform.js
| 109| 109| 	var cmpPosition = Engine.QueryInterface(ent, IID_Position);
| 110| 110| 	var unitAI = Engine.QueryInterface(ent, IID_UnitAI);
| 111| 111| 	if (cmpPosition && !cmpPosition.IsInWorld() && unitAI && unitAI.IsGarrisoned())
| 112|    |-	{
|    | 112|+	
| 113| 113| 		// We're a garrisoned unit, assume impossibility as I've been unable to find a way to get the holder ID.
| 114| 114| 		// TODO: change this if that ever becomes possibles
| 115| 115| 		return false;
| 116|    |-	}
|    | 116|+	
| 117| 117| 	return true;
| 118| 118| }
| 119| 119| 
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/helpers/Transform.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/helpers/Transform.js
| 169| 169| 
| 170| 170| 	// Check if units are blocking our template change
| 171| 171| 	if (template.Obstruction && newTemplate.Obstruction)
| 172|    |-	{
|    | 172|+	
| 173| 173| 		// This only needs to be done if the new template is strictly bigger than the old one
| 174| 174| 		// "Obstructions" are annoying to test so just check.
| 175| 175| 		if (newTemplate.Obstruction.Obstructions ||
| 195| 195| 					return DeleteEntityAndReturn(previewEntity, cmpPosition, pos, angle, cmpNewPosition, true);
| 196| 196| 			}
| 197| 197| 		}
| 198|    |-	}
|    | 198|+	
| 199| 199| 
| 200| 200| 	return DeleteEntityAndReturn(previewEntity, cmpPosition, pos, angle, cmpNewPosition, false);
| 201| 201| }

binaries/data/mods/public/simulation/components/tests/test_Foundation.js
|  47| »   let·pos·=·new·Vector2D(4,·5);
|    | [NORMAL] JSHintBear:
|    | 'pos' was used before it was defined.

binaries/data/mods/public/simulation/components/tests/test_Foundation.js
| 121| »   let·cmpFoundation·=·ConstructComponent(foundationEnt,·"Foundation",·{});
|    | [NORMAL] JSHintBear:
|    | 'cmpFoundation' was used before it was defined.
|    | [NORMAL] ESLintBear (operator-linebreak):
|    | '&&' should be placed at the end of the line.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Foundation.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Foundation.js
| 212| 212| 					// If obstructing fauna is gaia or our own but doesn't have UnitAI, just destroy it
| 213| 213| 					var cmpOwnership = Engine.QueryInterface(ent, IID_Ownership);
| 214| 214| 					var cmpIdentity = Engine.QueryInterface(ent, IID_Identity);
| 215|    |-					if (cmpOwnership && cmpIdentity && cmpIdentity.HasClass("Animal")
| 216|    |-						&& (cmpOwnership.GetOwner() == 0 || cmpFoundationOwnership && cmpOwnership.GetOwner() == cmpFoundationOwnership.GetOwner()))
|    | 215|+					if (cmpOwnership && cmpIdentity && cmpIdentity.HasClass("Animal") &&
|    | 216|+						(cmpOwnership.GetOwner() == 0 || cmpFoundationOwnership && cmpOwnership.GetOwner() == cmpFoundationOwnership.GetOwner()))
| 217| 217| 						Engine.DestroyEntity(ent);
| 218| 218| 				}
| 219| 219| 

binaries/data/mods/public/simulation/components/Foundation.js
| 362| »   »   »   let·cmpOwnership·=·Engine.QueryInterface(this.entity,·IID_Ownership);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'cmpOwnership' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/Foundation.js
| 216| »   »   »   »   »   »   &&·(cmpOwnership.GetOwner()·==·0·||·cmpFoundationOwnership·&&·cmpOwnership.GetOwner()·==·cmpFoundationOwnership.GetOwner()))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/Foundation.js
| 257| »   »   »   var·cmpFoundationOwnership·=·Engine.QueryInterface(this.entity,·IID_Ownership);
|    | [NORMAL] JSHintBear:
|    | 'cmpFoundationOwnership' is already defined.

binaries/data/mods/public/simulation/components/Foundation.js
| 331| »   »   var·pos·=·cmpPosition.GetPosition2D();
|    | [NORMAL] JSHintBear:
|    | 'pos' is already defined.

binaries/data/mods/public/simulation/components/Foundation.js
| 333| »   »   var·rot·=·cmpPosition.GetRotation();
|    | [NORMAL] JSHintBear:
|    | 'rot' is already defined.

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

temple accepted this revision.Mar 20 2018, 6:42 PM
In D21#57664, @elexis wrote:

Whatever's done in Foundation should be done in Transform too, I assume.

Agree, an entity may not be upgraded until all unit and static obstructions are removed in the affected area.

I was thinking it would be similar to foundation where it would delete carcasses and tell units to leave the area, but I guess this is fine (and tests okay).

source/simulation2/components/CCmpObstructionManager.cpp
1096

maybe bbHalfSize since it's not expanded anymore

Itms resigned from this revision.Mar 20 2018, 10:21 PM
This revision is now accepted and ready to land.Mar 20 2018, 10:21 PM
Itms added a comment.Mar 20 2018, 10:22 PM

Thanks for updating this patch ? I'm afraid it has been a bit too much time since I took a look at this, so I'd rather leave others review it ?

This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Mar 21 2018, 2:44 AM

Thanks for the patch wraitii and the reviews Itms and temple! Good to never see this bug again!

In this 1v1 between borg- and nani with r21604, nani couldn't build a storehouse even after placing the foundation in the field of view.


(Probably not, but maybe it was some kind of mirage leftover?)

In D21#57917, @elexis wrote:

In this 1v1 between borg- and nani with r21604, nani couldn't build a storehouse even after placing the foundation in the field of view.


(Probably not, but maybe it was some kind of mirage leftover?)

It was borg's mirage. Dirty trick he played :)