Page MenuHomeWildfire Games

petra: fix rP19698 when a unit is promoted the same turn the attack is launched
ClosedPublic

Authored by mimo on Jun 3 2017, 7:44 PM.

Details

Summary

as the title say,

Test Plan

happen when one ai cc is captured and one of its defender is promoted on the same turn.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mimo created this revision.Jun 3 2017, 7:44 PM
mimo added a comment.Jun 3 2017, 7:52 PM


Replay which shows the problem on turn about 14000

Vulcan added a subscriber: Vulcan.Jun 4 2017, 1:28 AM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1462/ for more details.

Vulcan added a comment.Jun 4 2017, 1:30 AM
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/simulation/ai/petra/attackManager.js
| 361| »   »   »   let·relicsCount·=·allRelics.filter(relic·=>·relic.owner()·===·i).length;
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/simulation/ai/petra/headquarters.js
|1329| »   plan.onStart·=·function(gameState)·{·gameState.ai.queueManager.changePriority("economicBuilding",·gameState.ai.Config.priorities.economicBuilding);·};
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'gameState' is already declared in the upper scope.

binaries/data/mods/public/simulation/ai/petra/headquarters.js
|1409| »   »   plan.isGo·=·function·(gameState)·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'gameState' is already declared in the upper scope.

binaries/data/mods/public/simulation/ai/petra/headquarters.js
|1571| »   »   »   »   plan.onStart·=·function(gameState)·{·gameState.ai.queueManager.changePriority("defenseBuilding",·gameState.ai.Config.priorities.defenseBuilding);·};
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'gameState' is already declared in the upper scope.

binaries/data/mods/public/simulation/ai/petra/headquarters.js
|1603| »   »   plan.onStart·=·function(gameState)·{·gameState.ai.queueManager.changePriority("defenseBuilding",·gameState.ai.Config.priorities.defenseBuilding);·};
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'gameState' is already declared in the upper scope.

binaries/data/mods/public/simulation/ai/petra/headquarters.js
|1643| »   »   »   plan.onStart·=·function(gameState)·{·gameState.ai.queueManager.changePriority("militaryBuilding",·gameState.ai.Config.priorities.militaryBuilding);·};
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'gameState' is already declared in the upper scope.

binaries/data/mods/public/simulation/ai/petra/headquarters.js
| 170| »   »   »   »   builders.forEach(function·(worker)·{
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/120/ for more details.

In D601#24623, @mimo wrote:


Replay which shows the problem on turn about 14000

And at which revision exactly? rP19725? At both that revision and rP19728, I could not reproduce any warnings/errors when rerunning the replay non-visually, but I did get these at the start:

ERROR: JavaScript error: Argument must be an array
WARNING: The mod 'mod' wasn't used when creating this replay file, but was passed as an argument!
WARNING: The mod 'public' wasn't used when creating this replay file, but was passed as an argument!
WARNING: CSimulation2::SetInitAttributes: No seed value specified - using 0

When simply using ./pyrogenesis -mod=public -replay=commands.txt
('mod' wasn't passed as an argument.)
I assume these are because the game was a nonvisual autostart? The replay doesn't have the usual "mods":["mod","public"] on the first line.

When it comes to the diff, it seems really hacky to loop on events in a function like this. I wonder if it would be better to move the call to counterAttack in headquarters.js to defenseManager (and then loop on events.OwnershipChanged there), and then call it if necessary after all armies have been updated, it order to avoid this.

binaries/data/mods/public/simulation/ai/petra/attackManager.js
536 ↗(On Diff #2409)

Shouldn't you rather continue; in order to handle any other relevant entities that have also been renamed?

mimo added a comment.Jun 4 2017, 11:47 AM

Yes, that's an autostart and you need -mod=public
The revision which produces the error is 19724 (sorry i forgot to notify it)

Concerning the move from headquarter to defenseManager, you are right that it would remove the check the renamed. I'll upload a new patch.

mimo updated this revision to Diff 2413.Jun 4 2017, 11:51 AM
mimo edited the summary of this revision. (Show Details)

update patch

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1465/ for more details.

Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/simulation/ai/petra/headquarters.js
|1321| »   plan.onStart·=·function(gameState)·{·gameState.ai.queueManager.changePriority("economicBuilding",·gameState.ai.Config.priorities.economicBuilding);·};
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'gameState' is already declared in the upper scope.

binaries/data/mods/public/simulation/ai/petra/headquarters.js
|1401| »   »   plan.isGo·=·function·(gameState)·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'gameState' is already declared in the upper scope.

binaries/data/mods/public/simulation/ai/petra/headquarters.js
|1563| »   »   »   »   plan.onStart·=·function(gameState)·{·gameState.ai.queueManager.changePriority("defenseBuilding",·gameState.ai.Config.priorities.defenseBuilding);·};
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'gameState' is already declared in the upper scope.

binaries/data/mods/public/simulation/ai/petra/headquarters.js
|1595| »   »   plan.onStart·=·function(gameState)·{·gameState.ai.queueManager.changePriority("defenseBuilding",·gameState.ai.Config.priorities.defenseBuilding);·};
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'gameState' is already declared in the upper scope.

binaries/data/mods/public/simulation/ai/petra/headquarters.js
|1635| »   »   »   plan.onStart·=·function(gameState)·{·gameState.ai.queueManager.changePriority("militaryBuilding",·gameState.ai.Config.priorities.militaryBuilding);·};
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'gameState' is already declared in the upper scope.

binaries/data/mods/public/simulation/ai/petra/headquarters.js
| 170| »   »   »   »   builders.forEach(function·(worker)·{
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/123/ for more details.

elexis added a subscriber: elexis.Jun 5 2017, 4:18 PM

Committed in rP19740. Phabricator broken atm.

This revision was automatically updated to reflect the committed changes.
In D601#24680, @mimo wrote:

Yes, that's an autostart and you need -mod=public

I meant I used the exact command ./pyrogenesis -mod=public -replay=commands.txt on the attached commands.txt, but got the errors I posted when doing so. I didn't pass the mod 'mod', but I did pass the mod 'public'. It seems like some sort of bug.

mimo added a comment.Jun 6 2017, 7:55 PM
In D601#24680, @mimo wrote:

Yes, that's an autostart and you need -mod=public

I meant I used the exact command ./pyrogenesis -mod=public -replay=commands.txt on the attached commands.txt, but got the errors I posted when doing so. I didn't pass the mod 'mod', but I did pass the mod 'public'. It seems like some sort of bug.

Yes, parts of these messages (in particular the missing -mod=public) must have been fixed with recent replays and are due to the fact that this one is a very old commands.txt (maybe two years old or more) which I still use for tests, and parts of them (as the CSimulation2::SetInitAttributes: No seed value specified - using 0) are due to variables not setup while running with autoStart.