Paths

Table of Contentst

Differential D1108

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

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

Branch

/ps/trunk

Lint

Lint OK

Unit

No Unit Test Coverage

Build Status

Buildable 4031
Build 7070: Vulcan Build (Windows)	Jenkins
Build 7069: Vulcan Build	Jenkins
Build 7068: arc lint + arc unit

Event Timeline

elexis created this revision.Dec 5 2017, 3:19 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.

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

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

Harbormaster completed remote builds in B3959: Diff 4553.Dec 5 2017, 5:56 AM

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 mentioned this in D1119: Allow AI to read JSON files and Resources globalscript, don't copy Resources each turn in GetSimulationState.Dec 6 2017, 3:34 PM

elexis mentioned this in D1130: Remove useless cache and serialization of technologies TechnologyManager.Dec 8 2017, 5:17 PM

elexis mentioned this in rP20610: Remove unused serialized cache of technology templates in TechnologyManager..Dec 8 2017, 7:35 PM

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

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

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.

Harbormaster completed remote builds in B4031: Diff 4664.Dec 9 2017, 3:16 AM

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	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	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	Nevermind, I forgot the OS cache (#4072)

elexis mentioned this in D587: Remove VFS cache.Dec 9 2017, 3:37 PM

elexis added inline comments.Dec 10 2017, 6:40 PM

binaries/data/mods/public/globalscripts/ModificationTemplates.js
51	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.

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	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	Any reason not to inline this ?
binaries/data/mods/public/simulation/components/Auras.js
63	Can it be inlined ?
binaries/data/mods/public/simulation/components/tests/test_TechnologyManager.js
5	Why the parenthesis ?

elexis abandoned this revision.Dec 15 2017, 5:17 PM

elexis mentioned this in D1107: Remove redundant JS VFS functions from gui/common/functions_utility.js and simplify related code.Dec 23 2017, 11:32 PM

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	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	indeed nicer to inline (I've kept the changes minimal)
binaries/data/mods/public/simulation/components/Auras.js
63	ternary possibly
binaries/data/mods/public/simulation/components/tests/test_TechnologyManager.js
5	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).

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.

Harbormaster completed remote builds in B4195: Diff 4967.Dec 27 2017, 12:41 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)............................................................................................................................................................................................................................
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

Harbormaster completed remote builds in B4195: Diff 4967.Dec 27 2017, 2:24 AM

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
15–17	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	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
93–94	whitespaces
138	getting the same value twice in the function, maybe store it
binaries/data/mods/public/simulation/components/ProductionQueue.js
188	(oos, but should be for .. of)
binaries/data/mods/public/simulation/components/tests/test_ProductionQueue.js
35–37	?

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`?

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
15–17	Indeed, I didn't notice it can be further simplified following D1107, thanks. Loses a bit of symmetry, but thats no problem
29	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
93–94	roger
138	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	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	(failed merge conflict resolution)

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.

Closed by commit rP20737: Replace DataTemplateManager simulation component with a globalscript, refs… (authored by elexis). · Explain WhyDec 31 2017, 2:03 AM

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

elexis mentioned this in rP20756: JS GUI session cleanup..Jan 2 2018, 10:51 PM

elexis mentioned this in D1192: Remove structree production queue parsing quadruplication.Jan 3 2018, 1:48 AM

elexis mentioned this in D1201: AuraManager shouldn't serialize the affected classes array directly.Jan 6 2018, 3:23 AM

elexis mentioned this in rP21014: Fix AuraManager OOS on rejoin following rP20737 / D1108..Jan 25 2018, 5:46 PM