Current serialization of petra functions leads to oos in mp games when using different systems. See rP16016.
This patch fixes the problem, removing the use of eval/uneval.
Details
Details
- Reviewers
elexis - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) - Commits
- rP19794: petra: revisit the serialization of functions, to fix rP16016
Check that the code do what is expected (no need to understand the guts of the ai, both the current and the fixed codes should produce identical result, except of course when serializing)
Check on sp games that save and reload of games is still ok
Check that the issue reported in rP16016 is fixed
Diff Detail
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 2226 Build 3668: Vulcan Build (Windows) Jenkins Build 3667: Vulcan Build Jenkins Build 3663: arc lint + arc unit
Event Timeline
Comment Actions
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/1519/ for more details.
Comment Actions
Executing section Default...
Executing section Source...
Executing section JS...
| | [NORMAL] ESLintBear (no-multi-spaces):
| | Multiple spaces found before '='.
|----| | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/ai/petra/tradeManager.js
| |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/ai/petra/tradeManager.js
| 406| 406| let candidate = { "gain": 0 };
| 407| 407| let potential = { "gain": 0 };
| 408| 408| let bestIndex = { "gain": 0 };
| 409| |- let bestLand = { "gain": 0 };
| | 409|+ let bestLand = { "gain": 0 };
| 410| 410|
| 411| 411| let traderTemplatesGains = gameState.getTraderTemplatesGains();
| 412| 412|
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/162/ for more details.
Comment Actions
The patch looks correct to me. The functions replaced are the same that were eval/uneval'ed before and the removal of prop property in the serialization functions is good too.
(I got sidetracked trying to figure out why we have var PETRA = function(m), how this could be refactored, then ended up in #4628 when researching JS possibilities)
With the patch applied the code is still the same as before, so it must be correct. Since these were the only two evals we had, the patch is complete too. Good job.
(For style things, I've only noticed that else after return is not needed,
that consecutive early returns can be merged to a single one and
that isGoRequirement sounds like a boolean but contains a string, so might be named goRequirement.)
Thanks for the patch, good to have it fixed!