Page MenuHomeWildfire Games

Convert simulation DataTemplateManager to globalscripts, remove json template serialization in simulation and AI
ClosedPublic

Authored by elexis on Dec 5 2017, 3:19 AM.

Details

Reviewers
mimo
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP20737: Replace DataTemplateManager simulation component with a globalscript, refs…
Trac Tickets
#4239
#4868
Summary

rP11584 introduced technologies and the simulation component TechnologyTemplateManager.js which was renamed to DataTemplateManager in rP18100.
rP13225 introduced the interface for this simulation component so that the AI could access it too.

Loading JSON files from the disk is a task that not only the simulation, but also the GUI and AI thread need to perform, possibly more.
So we need a globalscript if we don't want to reinvent the wheel.
We don't need any C++ code to do these things.
That will also allow the AI to load Aura files, parse team bonuses, judge the most valuable hero or relic and so forth.
Every context should be able to read JSON files from JS without any hassle. This should now be the case (#4868).

Nor should templates be serialized to begin with.

Test Plan

rP18100 had disabled serialization. But that caused an incredibly painful OOS documented in #4239.
rP18752 enabled serialization of JSON file data again, but it wasn't the ultimate solution.
As sanderd17 pointed out in the ticket, the other places, if no component ever serializes JSON file objects, then we won't run into that OOS.

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

elexis created this revision.Dec 5 2017, 3:19 AM
elexis planned changes to this revision.Dec 5 2017, 3:23 AM

The patch seems to works, except that I didn't make sure that rP18752 isn't reintroduced. As sanderd17 said in 4239#comment:7, the AuraManager and TechnologyManager should not serialize their templates either, which I didn't test for yet.

Vulcan added a subscriber: Vulcan.Dec 5 2017, 4:14 AM
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/globalscripts/VFS.js
|  19| »   [this.techNames,·this.allTechs]·=·loadTemplates(this.technologiesPath);
|    | [MAJOR] JSHintBear:
|    | Expected ']' to match '[' from line 19 and instead saw ','.

binaries/data/mods/public/globalscripts/VFS.js
|  19| »   [this.techNames,·this.allTechs]·=·loadTemplates(this.technologiesPath);
|    | [MAJOR] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/globalscripts/VFS.js
|  19| »   [this.techNames,·this.allTechs]·=·loadTemplates(this.technologiesPath);
|    | [NORMAL] JSHintBear:
|    | Expected an assignment or function call and instead saw an expression.

binaries/data/mods/public/globalscripts/VFS.js
|  19| »   [this.techNames,·this.allTechs]·=·loadTemplates(this.technologiesPath);
|    | [MAJOR] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/globalscripts/VFS.js
|  19| »   [this.techNames,·this.allTechs]·=·loadTemplates(this.technologiesPath);
|    | [MAJOR] JSHintBear:
|    | Expected an identifier and instead saw ']'.

binaries/data/mods/public/globalscripts/VFS.js
|  19| »   [this.techNames,·this.allTechs]·=·loadTemplates(this.technologiesPath);
|    | [MAJOR] JSHintBear:
|    | Expected an operator and instead saw '='.

binaries/data/mods/public/globalscripts/VFS.js
|  19| »   [this.techNames,·this.allTechs]·=·loadTemplates(this.technologiesPath);
|    | [NORMAL] JSHintBear:
|    | Expected an assignment or function call and instead saw an expression.

binaries/data/mods/public/globalscripts/VFS.js
|  19| »   [this.techNames,·this.allTechs]·=·loadTemplates(this.technologiesPath);
|    | [MAJOR] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/globalscripts/VFS.js
|  21| };
|    | [NORMAL] JSHintBear:
|    | Unnecessary semicolon.

binaries/data/mods/public/globalscripts/VFS.js
|  65| »   VFS·=·new·VFS();
|    | [NORMAL] JSHintBear:
|    | Reassignment of 'VFS', which is is a function. Use 'var' or 'let' to declare bindings that may change.

binaries/data/mods/public/simulation/helpers/Cheat.js
| 103| »   »   var·cmpTechnologyManager·=·Engine.QueryInterface(playerEnt,·IID_TechnologyManager);
|    | [NORMAL] JSHintBear:
|    | 'cmpTechnologyManager' is already defined.

binaries/data/mods/public/simulation/helpers/Cheat.js
| 110| »   »   »   var·cmpProductionQueue·=·Engine.QueryInterface(input.selected[0],·IID_ProductionQueue);
|    | [NORMAL] JSHintBear:
|    | 'cmpProductionQueue' is already defined.

binaries/data/mods/public/simulation/ai/common-api/shared.js
|  55| »   »   this._templates[name]·=·Engine.GetTemplate(name)·||·null
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 200| »   for·(var·i·in·techList)
|    | [NORMAL] JSHintBear:
|    | 'i' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 202| »   »   var·tech·=·techList[i];
|    | [NORMAL] JSHintBear:
|    | 'tech' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 214| »   for·(var·i·=·0;·i·<·techList.length;·i++)
|    | [NORMAL] JSHintBear:
|    | 'i' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 216| »   »   var·tech·=·techList[i];
|    | [NORMAL] JSHintBear:
|    | 'tech' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 252| »   »   »   ||·cmpTechnologyManager.IsTechnologyResearched(template.bottom)·||·cmpTechnologyManager.IsInProgress(template.bottom));
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 387| »   »   »   var·cmpTrigger·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_Trigger);
|    | [NORMAL] JSHintBear:
|    | 'cmpTrigger' is already defined.

binaries/data/mods/public/simulat
Vulcan added a comment.Dec 5 2017, 5:56 AM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
elexis added a comment.Dec 5 2017, 3:25 PM

Perhaps its nicer as globalscripts/Auras.js and globalscripts/Technologies.js, would also fit to Resources.js and we could add Cheats.js and Civs.js possbly in some VFS or eh DataTemplates directory (I hate this ambiguous name).

OT, but I'm not sure anymore that simulation/data/settings/ should be in that place, since it's mostly GUI choices and the simulation should not be restricted by the GUI restriction.

elexis updated this revision to Diff 4664.Dec 9 2017, 1:23 AM

Split into TechnologyTemplateManager and AuraTemplateManager.
Delete more AI technology template serialization.
Replace Auras template cache with unserialized cache.

elexis retitled this revision from Convert simulation DataTemplateManager to globalscripts VFS, allow AI to read auras to Convert simulation DataTemplateManager to globalscripts, remove json template serialization in simulation and AI.Dec 9 2017, 1:26 AM
Vulcan added a comment.Dec 9 2017, 3:14 AM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests).ERROR: JavaScript error: simulation/components/tests/test_Auras.js line 38
ReferenceError: assignment to undeclared variable AuraTemplateManager
  testAuras@simulation/components/tests/test_Auras.js:38:3
  @simulation/components/tests/test_Auras.js:96:1

In TestComponentScripts::test_scripts:
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Test failed: L"Running script simulation/components/tests/test_Auras.js"
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Assertion failed: scriptInterface.LoadScript(pathname, content)
ERROR: JavaScript error: simulation/components/tests/test_ProductionQueue.js line 35
ReferenceError: IID_DataTemplateManager is not defined
  @simulation/components/tests/test_ProductionQueue.js:35:1
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Test failed: L"Running script simulation/components/tests/test_ProductionQueue.js"
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Assertion failed: scriptInterface.LoadScript(pathname, content)
ERROR: JavaScript error: simulation/components/tests/test_TechnologyManager.js line 5
ReferenceError: assignment to undeclared variable TechnologyTemplateManager
  @simulation/components/tests/test_TechnologyManager.js:5:2
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Test failed: L"Running script simulation/components/tests/test_TechnologyManager.js"
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Assertion failed: scriptInterface.LoadScript(pathname, content)
..................................................................................................................................................................................................................................................................................................................
Failed 1 and Skipped 0 of 308 tests
Success rate: 99%
Running debug tests...
Running cxxtest tests (308 tests).ERROR: JavaScript error: simulation/components/tests/test_Auras.js line 38
ReferenceError: assignment to undeclared variable AuraTemplateManager
  testAuras@simulation/components/tests/test_Auras.js:38:3
  @simulation/components/tests/test_Auras.js:96:1

In TestComponentScripts::test_scripts:
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Test failed: L"Running script simulation/components/tests/test_Auras.js"
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Assertion failed: scriptInterface.LoadScript(pathname, content)
ERROR: JavaScript error: simulation/components/tests/test_ProductionQueue.js line 35
ReferenceError: IID_DataTemplateManager is not defined
  @simulation/components/tests/test_ProductionQueue.js:35:1
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Test failed: L"Running script simulation/components/tests/test_ProductionQueue.js"
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Assertion failed: scriptInterface.LoadScript(pathname, content)
ERROR: JavaScript error: simulation/components/tests/test_TechnologyManager.js line 5
ReferenceError: assignment to undeclared variable TechnologyTemplateManager
  @simulation/components/tests/test_TechnologyManager.js:5:2
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Test failed: L"Running script simulation/components/tests/test_TechnologyManager.js"
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2
Vulcan added a comment.Dec 9 2017, 3:16 AM
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/simulation/components/tests/test_TechnologyManager.js
|  12| cmpTechnologyManager.classCounts["Village"]·=·2;
|    | [NORMAL] JSHintBear:
|    | ['Village'] is better written in dot notation.

binaries/data/mods/public/simulation/components/tests/test_TechnologyManager.js
|  16| cmpTechnologyManager.classCounts["Village"]·=·6;
|    | [NORMAL] JSHintBear:
|    | ['Village'] is better written in dot notation.

binaries/data/mods/public/globalscripts/ModificationTemplates.js
|  22| };
|    | [NORMAL] JSHintBear:
|    | Unnecessary semicolon.

binaries/data/mods/public/globalscripts/ModificationTemplates.js
|  50| »   global.TechnologyTemplateManager·=·new·ModificationTemplateManager("simulation/data/technologies/")
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/simulation/ai/common-api/shared.js
|  55| »   »   this._templates[name]·=·Engine.GetTemplate(name)·||·null
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/simulation/helpers/Cheat.js
| 103| »   »   var·cmpTechnologyManager·=·Engine.QueryInterface(playerEnt,·IID_TechnologyManager);
|    | [NORMAL] JSHintBear:
|    | 'cmpTechnologyManager' is already defined.

binaries/data/mods/public/simulation/helpers/Cheat.js
| 110| »   »   »   var·cmpProductionQueue·=·Engine.QueryInterface(input.selected[0],·IID_ProductionQueue);
|    | [NORMAL] JSHintBear:
|    | 'cmpProductionQueue' is already defined.

binaries/data/mods/public/simulation/components/TechnologyManager.js
|  68| »   »   »   ||·(tech.top·&&·(this.IsTechnologyResearched(tech.top)·||·this.IsTechnologyResearched(tech.bottom))))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 229| »   »   »   for·(var·component·in·modifiedComponents)
|    | [NORMAL] JSHintBear:
|    | 'component' is already defined.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 235| »   »   var·cmpTemplateManager·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_TemplateManager);
|    | [NORMAL] JSHintBear:
|    | 'cmpTemplateManager' is already defined.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 236| »   »   var·template·=·cmpTemplateManager.GetCurrentTemplateName(msg.entity);
|    | [NORMAL] JSHintBear:
|    | 'template' is already defined.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 241| »   »   »   var·cmpIdentity·=·Engine.QueryInterface(msg.entity,·IID_Identity);
|    | [NORMAL] JSHintBear:
|    | 'cmpIdentity' is already defined.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 244| »   »   »   »   var·classes·=·cmpIdentity.GetClassesList();
|    | [NORMAL] JSHintBear:
|    | 'classes' is already defined.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 452| »   return·this.researchQueued.get(tech)
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/simulation/ai/common-api/entity.js
| 158| »   »   »   let·w·=·+right["@x"]·+·+right["@width"]/2·-·+left["@x"]·+·+left["@width"]/2;
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.

binaries/data/mods/public/simulation/ai/common-api/entity.js
| 158| »   »   »   let·w·=·+right["@x"]·+·+right["@width"]/2·-·+left["@x"]·+·+left["@width"]/2;
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.

binaries/data/mods/public/simulation/ai/common-api/entity.js
| 159| »   »   »   let·h·=·Math.max(+right["@z"]·+·+right["@depth"]/2,·+left["@z"]·+·+left["@depth"]/2)
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.

binaries/data/mods/public/simulation/ai/common-api/entity.js
| 159| »   »   »   let·h·=·Math.max(+right["@z"]·+·+right["@depth"]/2,·+left["@z"]·+·+left["@depth"]/2)
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.

binaries/data/mods/public/simulation/ai/common-api/entity.js
| 160| »   »   »   ······-·Math.min(+right["@z"]·-·+right["@depth"]/2,·+left["@z"]·-·+left["@depth"]/2);
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '-'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 202| »   for·(var·i·in·techList)
|    | [NORMAL] JSHintBear:
|    | 'i' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 204| »   »   var·tech·=·techList[i];
|    | [NORMAL] JSHintBear:
|    | 'tech' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 216| »   for·(var·i·=·0;·i·<·techList.length;·i++)
|    | [NORMAL] JSHintBear:
|    | 'i' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 218| »   »   var·tech·=·techList[i];
|    | [NORMAL] JSHintBear:
|    | 'tech' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 254| »   »   »   ||·cmpTechnologyManager.IsTechnologyResearched(template.bottom)·||·cmpTechnologyManager.IsInProgress(template.bottom));
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 392| »   »   »   var·cmpTrigger·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_Trigger);
|    | [NORMAL] JSHintBear:
|    | 'cmpTrigger' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 431| »   for·(var·i·=·0;·i·<·this.queue.length;·++i)
|    | [NORMAL] JSHintBear:
|    | 'i' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 797| »   »   »   »   »   var·cmpPlayer·=·QueryOwnerInterface(this.entity);
|    | [NORMAL] JSHintBear:
|    | 'cmpPlayer' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 808| »   »   »   var·cmpTechnologyManager·=·QueryOwnerInterface(this.entity,·IID_TechnologyManager);
|    | [NORMAL] JSHintBear:
|    | 'cmpTechnologyManager' is already defined.
elexis added reviewers: mimo, Restricted Owners Package.Dec 9 2017, 12:44 PM

mimo the majority of the patch consists of AI hunks, so you probably want to a read-through.
I can ask the others for the JS simulation diffs.
The most crucical thing to judge is whether there is any part of simulation/ that still serializes aura or tech templates, but I'm not aware of any (as all the DataTemplateManager references have been flattened).

elexis added inline comments.Dec 9 2017, 12:47 PM
binaries/data/mods/public/simulation/components/Auras.js
22 ↗(On Diff #4664)

I have removed the cache for simplicity, otherwise we would require ugly Serialize and Deserialize functions.
It had only cached references to the objects rather than copied the objects, so it didn't have a big memory footprint.

Now we have function calls and an object property lookup where before we had only an object property lookup.
I'll do a performance test to test if it's significant.

elexis added inline comments.Dec 9 2017, 3:22 PM
binaries/data/mods/public/globalscripts/ModificationTemplates.js
51 ↗(On Diff #4664)

This is also executed each time a message box is opened. So either we

  • don't mind
  • or do the init manually once in sim, once in ai, and in many dialogs (like we do with the g_Settings variables. or g_CivInfo).
  • or rely on the disks cache (assumptious, limited in size)
  • or make it a just-in-time fileloading (can cause the game to wait for a HDD to get out of standby mode for 20 seconds+)
  • or rely on the VFS cache to be removed in D587
  • or ScriptInterface::ReadJSONFile could cache every JSON file, but that sounds redundant with the VFS cache in D587
elexis added inline comments.Dec 9 2017, 3:28 PM
binaries/data/mods/public/globalscripts/ModificationTemplates.js
51 ↗(On Diff #4664)

Nevermind, I forgot the OS cache (#4072)

elexis added inline comments.Dec 10 2017, 6:40 PM
binaries/data/mods/public/globalscripts/ModificationTemplates.js
51 ↗(On Diff #4664)

and I don't know which simulation file should hold the global AuraTemplateManager or call a global init function cleanly.
And it can easily be changed after the rest of the patch is in.

mimo added a comment.Dec 10 2017, 8:24 PM
In D1108#45235, @elexis wrote:

mimo the majority of the patch consists of AI hunks, so you probably want to a read-through.
I can ask the others for the JS simulation diffs.
The most crucical thing to judge is whether there is any part of simulation/ that still serializes aura or tech templates, but I'm not aware of any (as all the DataTemplateManager references have been flattened).

I'm not sure to find the time to review this patch in the near future as it is quite long, so do not hesitate to ask anybody else. I had nonetheless a look at the simulation/ai changes, and do not see anything to complain. If not reviewed before, i think i could do it during the christmas break.

In D1108#45675, @mimo wrote:
In D1108#45235, @elexis wrote:

mimo the majority of the patch consists of AI hunks, so you probably want to a read-through.
I can ask the others for the JS simulation diffs.
The most crucical thing to judge is whether there is any part of simulation/ that still serializes aura or tech templates, but I'm not aware of any (as all the DataTemplateManager references have been flattened).

I'm not sure to find the time to review this patch in the near future as it is quite long, so do not hesitate to ask anybody else. I had nonetheless a look at the simulation/ai changes, and do not see anything to complain. If not reviewed before, i think i could do it during the christmas break.

It's all I wanted to hear for now. (Whether you have a striking disagreement with the AI part on first sight).
The details are juicy and I'll take full responsability of any (OOS) fallout. :-)

Stan added a subscriber: Stan.Dec 10 2017, 9:07 PM
Stan added inline comments.
binaries/data/mods/public/globalscripts/ModificationTemplates.js
51 ↗(On Diff #4664)

I wonder, were those paths to change in the future, how many occurrences are there of those ?

binaries/data/mods/public/simulation/ai/common-api/technology.js
49 ↗(On Diff #4664)

Any reason not to inline this ?

binaries/data/mods/public/simulation/components/Auras.js
63 ↗(On Diff #4664)

Can it be inlined ?

binaries/data/mods/public/simulation/components/tests/test_TechnologyManager.js
5 ↗(On Diff #4664)

Why the parenthesis ?

elexis abandoned this revision.Dec 15 2017, 5:17 PM
elexis reclaimed this revision.Dec 26 2017, 5:05 PM
elexis added a subscriber: sanderd17.

Imaginary Person: But elexis, the patch is too important to abandon it!
elexis: Ok you are right :-)

(not requesting an entire review of mimo, was just assigned for the AI part)

binaries/data/mods/public/globalscripts/ModificationTemplates.js
51 ↗(On Diff #4664)

Few. We might add globals eventually if they are used in more than one place.

binaries/data/mods/public/simulation/ai/common-api/technology.js
49 ↗(On Diff #4664)

indeed nicer to inline (I've kept the changes minimal)

binaries/data/mods/public/simulation/components/Auras.js
63 ↗(On Diff #4664)

ternary possibly

binaries/data/mods/public/simulation/components/tests/test_TechnologyManager.js
5 ↗(On Diff #4664)

With parentheses it's evalued to an empty object and returned.
Without the parentheses it's considered the scope of the function and that has no return statement, hence returns undefined (instead of the intended empty object).

elexis updated this revision to Diff 4967.Dec 27 2017, 12:41 AM

Init only in the simulation and AI for now (rather than anytime a new GUI page (message box) is opened).
Rebase following attack range overlays.
Stans inlines.
Fixed typo.

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)............................................................................................................................................................................................................................
In TestComponentScripts::test_scripts:
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:56: Error: Assertion failed: componentManager->LoadScript(VfsPath(L"simulation/helpers") / pathname)
ERROR: JavaScript error: globalscripts/utility.js line 68
TypeError: Engine.ListDirectoryFiles is not a function
  listFiles@globalscripts/utility.js:68:9
  ModificationTemplates@globalscripts/ModificationTemplates.js:14:19
  LoadModificationTemplates@globalscripts/ModificationTemplates.js:47:25
  @simulation/helpers/ValueModification.js:2:1
  @simulation/components/tests/test_Attack.js:4:1
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Test failed: L"Running script simulation/components/tests/test_Attack.js"
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Assertion failed: scriptInterface.LoadScript(pathname, content)
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:56: Error: Assertion failed: componentManager->LoadScript(VfsPath(L"simulation/helpers") / pathname)
ERROR: JavaScript error: globalscripts/utility.js line 68
TypeError: Engine.ListDirectoryFiles is not a function
  listFiles@globalscripts/utility.js:68:9
  ModificationTemplates@globalscripts/ModificationTemplates.js:14:19
  LoadModificationTemplates@globalscripts/ModificationTemplates.js:47:25
  @simulation/helpers/ValueModification.js:2:1
  @simulation/components/tests/test_Auras.js:2:1
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Test failed: L"Running script simulation/components/tests/test_Auras.js"
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Assertion failed: scriptInterface.LoadScript(pathname, content)
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:56: Error: Assertion failed: componentManager->LoadScript(VfsPath(L"simulation/helpers") / pathname)
ERROR: JavaScript error: globalscripts/utility.js line 68
TypeError: Engine.ListDirectoryFiles is not a function
  listFiles@globalscripts/utility.js:68:9
  ModificationTemplates@globalscripts/ModificationTemplates.js:14:19
  LoadModificationTemplates@globalscripts/ModificationTemplates.js:47:25
  @simulation/helpers/ValueModification.js:2:1
  @simulation/components/tests/test_Builder.js:1:1
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Test failed: L"Running script simulation/components/tests/test_Builder.js"
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Assertion failed: scriptInterface.LoadScript(pathname, content)
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:56: Error: Assertion failed: componentManager->LoadScript(VfsPath(L"simulation/helpers") / pathname)
ERROR: JavaScript error: globalscripts/utility.js line 68
TypeError: Engine.ListDirectoryFiles is not a function
  listFiles@globalscripts/utility.js:68:9
  ModificationTemplates@globalscripts/ModificationTemplates.js:14:19
  LoadModificationTemplates@globalscripts/ModificationTemplates.js:47:25
  @simulation/helpers/ValueModification.js:2:1
  @simulation/components/tests/test_Capturable.js:2:1
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Test failed: L"Running script simulation/components/tests/test_Capturable.js"
/mnt/data/jenkins-phabricator/wor
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/simulation/components/tests/test_TechnologyManager.js
|  12| cmpTechnologyManager.classCounts["Village"]·=·2;
|    | [NORMAL] JSHintBear:
|    | ['Village'] is better written in dot notation.

binaries/data/mods/public/simulation/components/tests/test_TechnologyManager.js
|  16| cmpTechnologyManager.classCounts["Village"]·=·6;
|    | [NORMAL] JSHintBear:
|    | ['Village'] is better written in dot notation.

binaries/data/mods/public/globalscripts/ModificationTemplates.js
|  22| };
|    | [NORMAL] JSHintBear:
|    | Unnecessary semicolon.

binaries/data/mods/public/globalscripts/ModificationTemplates.js
|  48| »   global.TechnologyTemplates·=·new·ModificationTemplates("simulation/data/technologies/")
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/simulation/components/TechnologyManager.js
|  68| »   »   »   ||·(tech.top·&&·(this.IsTechnologyResearched(tech.top)·||·this.IsTechnologyResearched(tech.bottom))))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 229| »   »   »   for·(var·component·in·modifiedComponents)
|    | [NORMAL] JSHintBear:
|    | 'component' is already defined.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 235| »   »   var·cmpTemplateManager·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_TemplateManager);
|    | [NORMAL] JSHintBear:
|    | 'cmpTemplateManager' is already defined.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 236| »   »   var·template·=·cmpTemplateManager.GetCurrentTemplateName(msg.entity);
|    | [NORMAL] JSHintBear:
|    | 'template' is already defined.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 241| »   »   »   var·cmpIdentity·=·Engine.QueryInterface(msg.entity,·IID_Identity);
|    | [NORMAL] JSHintBear:
|    | 'cmpIdentity' is already defined.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 244| »   »   »   »   var·classes·=·cmpIdentity.GetClassesList();
|    | [NORMAL] JSHintBear:
|    | 'classes' is already defined.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 452| »   return·this.researchQueued.get(tech)
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 202| »   for·(var·i·in·techList)
|    | [NORMAL] JSHintBear:
|    | 'i' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 204| »   »   var·tech·=·techList[i];
|    | [NORMAL] JSHintBear:
|    | 'tech' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 216| »   for·(var·i·=·0;·i·<·techList.length;·i++)
|    | [NORMAL] JSHintBear:
|    | 'i' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 218| »   »   var·tech·=·techList[i];
|    | [NORMAL] JSHintBear:
|    | 'tech' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 254| »   »   »   ||·cmpTechnologyManager.IsTechnologyResearched(template.bottom)·||·cmpTechnologyManager.IsInProgress(template.bottom));
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 392| »   »   »   var·cmpTrigger·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_Trigger);
|    | [NORMAL] JSHintBear:
|    | 'cmpTrigger' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 431| »   for·(var·i·=·0;·i·<·this.queue.length;·++i)
|    | [NORMAL] JSHintBear:
|    | 'i' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 797| »   »   »   »   »   var·cmpPlayer·=·QueryOwnerInter
bb added a subscriber: bb.Dec 27 2017, 10:11 PM

Vulcan ^^

open a SP game, type "gift from the gods" and sniff ("back to the future" the same way)

binaries/data/mods/public/globalscripts/ModificationTemplates.js
16 ↗(On Diff #4967)

No idea if it helps on performance (maybe it even doesn't matter), fill (and freeze) this.names before the loop and loop over that, instead of filling it item per item (also leaves shorter code this way)

29 ↗(On Diff #4967)

Was thinking about a set, but meh-ed

binaries/data/mods/public/simulation/ai/common-api/globals.js
4 ↗(On Diff #4967)

misspelled plea for a t?

binaries/data/mods/public/simulation/components/Auras.js
96–98 ↗(On Diff #4967)

whitespaces

146 ↗(On Diff #4967)

getting the same value twice in the function, maybe store it

binaries/data/mods/public/simulation/components/ProductionQueue.js
188 ↗(On Diff #4967)

(oos, but should be for .. of)

binaries/data/mods/public/simulation/components/tests/test_ProductionQueue.js
35–37 ↗(On Diff #4967)

?

elexis added inline comments.Dec 30 2017, 3:58 PM
binaries/data/mods/public/simulation/ai/common-api/globals.js
5 ↗(On Diff #4967)

@mimo is this the ideal file location for globalscript prototype instances?

There could come more JSON template loaders, like for DamageTypes.

Perhaps it would be better in common-api/shared.js?

elexis marked an inline comment as done.Dec 30 2017, 10:57 PM

Thanks for the feedback bb,

In D1108#47654, @bb wrote:

Vulcan ^^
open a SP game, type "gift from the gods" and sniff ("back to the future" the same way)

Both were failed rebases.

binaries/data/mods/public/globalscripts/ModificationTemplates.js
16 ↗(On Diff #4967)

Indeed, I didn't notice it can be further simplified following D1107, thanks. Loses a bit of symmetry, but thats no problem

29 ↗(On Diff #4967)

Thought about it too, but not possible because we want to be able to use array functions on the list of techs.

binaries/data/mods/public/simulation/ai/common-api/globals.js
5 ↗(On Diff #4967)

I kept the resources loader in resources.js for now, add the modification template call to technologies.js and load the simulation context one in a new helper file. It's the best solution I could come up with, despite that it's not nice to create a new simulation/helpers/ file with only one line (yet better than mixing it in somewhere unrelated).

Please tell me if you would prefer this global.js files, so that all other files do start with the var API3 = function(m) line, then I can move those 2 calls over.

The solution of the very first diff was more ugly IMO (loading it always everywhere, as long as Engine.ReadJSONFile exists) and less performant (all JSON files loaded with each new message box and whatnot).

binaries/data/mods/public/simulation/components/Auras.js
96–98 ↗(On Diff #4967)

roger

146 ↗(On Diff #4967)

Worse performance in case there are no requirements, not sure if better performance if there are some, should be cheap in both cases as it's only a getter which can be accessed directly

binaries/data/mods/public/simulation/components/ProductionQueue.js
188 ↗(On Diff #4967)

Probably should find a different acronym, since this one is spicy in the context of this diff :-)

binaries/data/mods/public/simulation/components/tests/test_ProductionQueue.js
35–37 ↗(On Diff #4967)

(failed merge conflict resolution)

elexis marked an inline comment as done.EditedDec 31 2017, 1:55 AM

Read through the entire thing again and confirmed absence of JSON further serialization, tested all cheats, AI match, rejointest of a hashed replay, noticed upcoming GUI removal fun.

If we ever decide to cache the cache in each Auras component, then we need to take special care to not serialize that.

Thanks for all feedback.

This revision was automatically updated to reflect the committed changes.
Owners added subscribers: Restricted Owners Package, Restricted Owners Package, Restricted Owners Package.Dec 31 2017, 2:03 AM