petra: revisit the serialization of functions, to fix rP16016
ClosedPublic
Actions

Authored by mimo on Jun 9 2017, 8:19 PM.

Details

Reviewers

elexis

Group Reviewers

Restricted Owners Package

(Owns No Changed Paths)

Commits

rP19794: petra: revisit the serialization of functions, to fix rP16016

Summary

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.

Test Plan

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

Repository

rP 0 A.D. Public Repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

mimo created this revision.Jun 9 2017, 8:19 PM

mimo retitled this revision from petra: revisit the serialization of functions, to fix to petra: revisit the serialization of functions, to fix rP16016.

elexis awarded a token.Jun 9 2017, 8:22 PM

mimo edited the summary of this revision. (Show Details)Jun 9 2017, 8:28 PM

mimo edited the test plan for this revision. (Show Details)

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.

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.

Harbormaster completed remote builds in B2226: Diff 2493.Jun 9 2017, 11:24 PM

I tested this and the OOS I was having does seem to be resolved.

Closed by commit rP19794: petra: revisit the serialization of functions, to fix rP16016 (authored by mimo). · Explain WhyJun 17 2017, 10:42 AM

This revision was automatically updated to reflect the committed changes.

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!

(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.)

see rP19795 and rP19796