Ported from #4268 (as is so far, it needs some work cf itms comment)
Details
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 Build Jenkins Build 9480: arc lint + arc unit
Event Timeline
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
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.
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.
Rebased patch.
Doesn't implement Itms remarks yet.
Changes some auto to a map iterator.
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
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
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 |
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. |
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
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 |
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 ?
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?)