Page MenuHomeWildfire Games

Deep freeze templates, techs, auras, resources data and entity states
ClosedPublic

Authored by elexis on Aug 24 2017, 12:14 AM.

Details

Summary

As reported in #4257, we use global cache variables to hold entity states and various template data.
The more code uses the cached variables, the more likely it becomes that one part unintentionally write a helper variable to that cache and silently breaks the data contents.

A very nasty example was rP18788, where the consequence was only visible when selecting two temples with a garrisoned mauryan hero healer.
Depends on the order of selection, slightly different tech reseach costs were shown.

The other examples mentioned in the ticket were all Commands.js ones, which were fixed by that C++ commit in rP17673.

The structure tree needs separate investigation.

The simulation components can have their this.entity and this.template property changed (for example fixed by a clone in rP20015),
despite ScriptComponent.cpp rP8865 trying to freeze them. But that must be fixed from the C++ side.

Test Plan

The nasty example bug is still reproducible by removing the clone, but it now throws a loud error instead of sneaking in different numbers.
The correctness for the frozen objects can be seen by changing the objects after calling the getter function.

The deepfreeze function is almost the same as the example on https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/freeze

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.Aug 24 2017, 12:14 AM
elexis retitled this revision from Deep freeze templates, techs, auras, resources data and entity staes to Deep freeze templates, techs, auras, resources data and entity states.Aug 24 2017, 12:16 AM
leper added inline comments.
binaries/data/mods/public/globalscripts/utility.js
5 ↗(On Diff #3298)

Why not instead expose ScriptInterface's FreezeObject?

Vulcan added a subscriber: Vulcan.Aug 24 2017, 1:05 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/1907/ for more details.

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

binaries/data/mods/public/gui/common/tooltips.js
| 606| »   »   '[/color][/font]'·+·"·"·+
|    | [NORMAL] ESLintBear (no-useless-concat):
|    | Unexpected string concatenation of literals.

binaries/data/mods/public/gui/common/tooltips.js
| 439| »   »   let·[rate,·count]·=·types.reduce((sum,·t)·=>·{
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/common/tooltips.js
| 618| »   if·(walk·==·0·&&·run·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/common/tooltips.js
| 618| »   if·(walk·==·0·&&·run·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/selection_panels.js
|  60| »   »   »   switch·(data.item)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/selection_panels.js
|  74| »   »   switch·(data.item)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/selection_panels.js
| 345| »   »   »   tooltip·+=·"\n"·+·"[color=\"red\"]"·+·translate(formationInfo.tooltip)·+·"[/color]";
|    | [NORMAL] ESLintBear (no-useless-concat):
|    | Unexpected string concatenation of literals.

binaries/data/mods/public/gui/session/selection_panels.js
| 782| »   »   »   »   »   »   switch·(entity.check)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/selection_panels.js
|1010| »   »   »   »   "[/font]"·+·"·"·+·getEntityNamesFormatted(template),
|    | [NORMAL] ESLintBear (no-useless-concat):
|    | Unexpected string concatenation of literals.

binaries/data/mods/public/gui/session/selection_panels.js
| 282| »   »   if·(!technologyEnabled·||·limits.canBeAddedCount·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/selection_panels.js
| 443| »   »   »   »   »   "callback":·function·(item)·{·lockGate(item.locked);·}
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/selection_panels.js
| 449| »   »   »   »   »   "callback":·function·(item)·{·lockGate(item.locked);·}
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/selection_panels.js
| 499| »   »   »   if·(state.pack.progress·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/selection_panels.js
| 643| »   »   if·(data.i·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/selection_panels.js
| 686| »   »   »   »   tech·=>·tech·!=·null·&&·!ret.some(
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with 'null'.

binaries/data/mods/public/gui/session/selection_panels.js
| 686| »   »   »   »   tech·=>·tech·!=·null·&&·!ret.some(
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/selection_panels.js
| 699| »   »   »   »   ret·=·ret.concat(filteredTechs.map(tech·=>·({
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/selection_panels.js
| 732| »   »   pair.hidden·=·data.item.tech.pair·==·null;
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with 'null'.

binaries/data/mods/public/gui/session/selection_panels.js
| 767| »   »   »   ].map(func·=>·func(template));
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/selection_panels.js
| 813| »   »   »   button.onPress·=·function·()·{
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/selection_panels.js
| 822| »   »   »   »   button.onMouseEnter·=·function()·{
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/selection_panels.js
| 825| »   »   »   »   button.onMouseLeave·=·function()·{
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/selection_panels.js
|1042| »   »   if·(!technologyEnabled·||·limits.canBeAddedCount·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/selection_panels.js
|1149| »   »   »   if·(progress·||·!technologyEnabled·||·limits.canBeAddedCount·==·0·&&
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '+'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|1655|1655| 
|1656|1656| 		playerStatistics.economyScore += total + ",";
|1657|1657| 		playerStatistics.militaryScore += Math.round((player.sequences.enemyUnitsKilledValue[maxIndex] +
|1658|    |-			player.sequences.enemyBuildingsDestroyedValue[maxIndex]) / 10)  + ",";
|    |1658|+			player.sequences.enemyBuildingsDestroyedValue[maxIndex]) / 10) + ",";
|1659|1659| 		playerStatistics.totalScore += (total + Math.round((player.sequences.enemyUnitsKilledValue[maxIndex] +
|1660|1660| 			player.sequences.enemyBuildingsDestroyedValue[maxIndex]) / 10)) + ",";
|1661|1661| 

binaries/data/mods/public/gui/session/session.js
| 430| »   »   colorizeHotkey("%(hotkey)s"·+·"·",·"selection.idleworker")·+
|    | [NORMAL] ESLintBear (no-useless-concat):
|    | Unexpected string concatenation of literals.

binaries/data/mods/public/gui/session/session.js
| 450| »   »   let·panelEnt·=·g_PanelEntities.find(panelEnt·=>·panelEnt.slot·!==·undefined·&&·panelEnt.slot·==·slot);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'panelEnt' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
| 461| »   »   let·panelEnt·=·g_PanelEntities.find(panelEnt·=>·panelEnt.slot·!==·undefined·&&·panelEnt.slot·==·slot);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'panelEnt' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1007| »   »   let·panelEnt·=·g_PanelEntities.find(panelEnt·=>·ent·==·panelEnt.ent);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'panelEnt' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1036| »   let·getPanelEntNameTooltip·=·panelEntState·=>·"[font=\"sans-bold-16\"]"·+·template.name.specific·+·"[/font]";
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'panelEntState' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1112| »   »   button.onpress·=·(function(i)·{·return·function()·{·performGroup((Engine.HotkeyIsPressed("selection.add")·?·"add"·:·"select"),·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1113| »   »   button.ondoublepress·=·(function(i)·{·return·function()·{·performGroup("snap",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1114| »   »   button.onpressright·=·(function(i)·{·return·function()·{·performGroup("breakUp",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
| 871| »   »   i·==·0·||
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/session.js
| 873| »   »   g_GameAttributes.settings.PlayerData[i].AI·!=·"");
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare

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

elexis added a comment.EditedAug 24 2017, 1:42 AM

A negligible advantage of JS code is that it can be modified easily.
The freezeSelf argument of the proposed JS function is used to semi-freeze the to-be-extended entity state and would have to be added to the C++ code, or we don't freeze the simple entity states at all, or we clone them to remove the locks (costs too much).

A ScriptFunctions.cpp extension alone is not sufficient because that only works for the GUI context, so a dupe would have to be added to the simulation context and that still doesn't work for the AI (CCmpAIManager.cpp):

Index: source/gui/scripting/ScriptFunctions.cpp
===================================================================
--- source/gui/scripting/ScriptFunctions.cpp	(revision 20026)
+++ source/gui/scripting/ScriptFunctions.cpp	(working copy)
@@ -724,10 +724,15 @@ void ForceGC(ScriptInterface::CxPrivate*
 	JS_GC(pCxPrivate->pScriptInterface->GetJSRuntime());
 	time = timer_Time() - time;
 	g_Console->InsertMessage(fmt::sprintf("Garbage collection completed in: %f", time));
 }
 
+void FreezeObject(ScriptInterface::CxPrivate* pCxPrivate, JS::HandleValue objVal)
+{
+	pCxPrivate->pScriptInterface->FreezeObject(objVal, true);
+}
+
 void DumpSimState(ScriptInterface::CxPrivate* UNUSED(pCxPrivate))
 {
 	OsPath path = psLogDir()/"sim_dump.txt";
 	std::ofstream file (OsString(path).c_str(), std::ofstream::out | std::ofstream::trunc);
 	g_Game->GetSimulation2()->DumpDebugState(file);
@@ -1152,10 +1157,11 @@ void GuiScriptingInit(ScriptInterface& s
 	scriptInterface.RegisterFunction<void, int, &SetTurnLength>("SetTurnLength");
 	scriptInterface.RegisterFunction<void, float, float, float, &SetCameraTarget>("SetCameraTarget");
 	scriptInterface.RegisterFunction<int, &Crash>("Crash");
 	scriptInterface.RegisterFunction<void, &DebugWarn>("DebugWarn");
 	scriptInterface.RegisterFunction<void, &ForceGC>("ForceGC");
+	scriptInterface.RegisterFunction<void, JS::HandleValue, &FreezeObject>("FreezeObject");
 	scriptInterface.RegisterFunction<void, &DumpSimState>("DumpSimState");
 	scriptInterface.RegisterFunction<void, &DumpTerrainMipmap>("DumpTerrainMipmap");
 	scriptInterface.RegisterFunction<void, unsigned int, &EnableTimeWarpRecording>("EnableTimeWarpRecording");
 	scriptInterface.RegisterFunction<void, &RewindTimeWarp>("RewindTimeWarp");
 	scriptInterface.RegisterFunction<void, bool, &SetBoundingBoxDebugOverlay>("SetBoundingBoxDebugOverlay");
Index: source/simulation2/system/ComponentManager.cpp
===================================================================
--- source/simulation2/system/ComponentManager.cpp	(revision 20026)
+++ source/simulation2/system/ComponentManager.cpp	(working copy)
@@ -82,10 +82,11 @@ CComponentManager::CComponentManager(CSi
 		m_ScriptInterface.RegisterFunction<int, std::string, CComponentManager::Script_AddEntity> ("AddEntity");
 		m_ScriptInterface.RegisterFunction<int, std::string, CComponentManager::Script_AddLocalEntity> ("AddLocalEntity");
 		m_ScriptInterface.RegisterFunction<void, int, CComponentManager::Script_DestroyEntity> ("DestroyEntity");
 		m_ScriptInterface.RegisterFunction<void, CComponentManager::Script_FlushDestroyedEntities> ("FlushDestroyedEntities");
 		m_ScriptInterface.RegisterFunction<JS::Value, std::wstring, CComponentManager::Script_ReadJSONFile> ("ReadJSONFile");
+		m_ScriptInterface.RegisterFunction<void, JS::HandleValue, CComponentManager::FreezeObject> ("FreezeObject");
 		m_ScriptInterface.RegisterFunction<JS::Value, std::wstring, CComponentManager::Script_ReadCivJSONFile> ("ReadCivJSONFile");
 		m_ScriptInterface.RegisterFunction<std::vector<std::string>, std::wstring, bool, CComponentManager::Script_FindJSONFiles> ("FindJSONFiles");
 	}
 
 	// Define MT_*, IID_* as script globals, and store their names
@@ -1188,10 +1189,15 @@ std::string CComponentManager::GenerateS
 JS::Value CComponentManager::Script_ReadJSONFile(ScriptInterface::CxPrivate* pCxPrivate, const std::wstring& fileName)
 {
 	return ReadJSONFile(pCxPrivate, L"simulation/data", fileName);
 }
 
+void CComponentManager::FreezeObject(ScriptInterface::CxPrivate* pCxPrivate, JS::HandleValue objVal)
+{
+	pCxPrivate->pScriptInterface->FreezeObject(objVal, true);
+}
+
 JS::Value CComponentManager::Script_ReadCivJSONFile(ScriptInterface::CxPrivate* pCxPrivate, const std::wstring& fileName)
 {
 	return ReadJSONFile(pCxPrivate, L"simulation/data/civs", fileName);
 }
 
Index: source/simulation2/system/ComponentManager.h
===================================================================
--- source/simulation2/system/ComponentManager.h	(revision 20026)
+++ source/simulation2/system/ComponentManager.h	(working copy)
@@ -331,10 +331,11 @@ private:
 	static int Script_AddEntity(ScriptInterface::CxPrivate* pCxPrivate, const std::string& templateName);
 	static int Script_AddLocalEntity(ScriptInterface::CxPrivate* pCxPrivate, const std::string& templateName);
 	static void Script_DestroyEntity(ScriptInterface::CxPrivate* pCxPrivate, int ent);
 	static void Script_FlushDestroyedEntities(ScriptInterface::CxPrivate* pCxPrivate);
 	static JS::Value Script_ReadJSONFile(ScriptInterface::CxPrivate* pCxPrivate, const std::wstring& fileName);
+	static void FreezeObject(ScriptInterface::CxPrivate* pCxPrivate, JS::HandleValue objVal);
 	static JS::Value Script_ReadCivJSONFile(ScriptInterface::CxPrivate* pCxPrivate, const std::wstring& fileName);
 	static std::vector<std::string> Script_FindJSONFiles(ScriptInterface::CxPrivate* pCxPrivate, const std::wstring& subPath, bool recursive);
 	static JS::Value ReadJSONFile(ScriptInterface::CxPrivate* pCxPrivate, const std::wstring& filePath, const std::wstring& fileName);
 
 	// callback function to handle recursively finding files in a directory

This would be nicer as it works everywhere

Index: source/scriptinterface/ScriptInterface.cpp
===================================================================
--- source/scriptinterface/ScriptInterface.cpp	(revision 20026)
+++ source/scriptinterface/ScriptInterface.cpp	(working copy)
@@ -221,10 +221,20 @@ bool deepcopy(JSContext* cx, uint argc,
 
 	args.rval().set(ret);
 	return true;
 }
 
+bool deepfreeze(JSContext* cx, uint argc, jsval* vp)
+{
+	JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
+	if (args.length() < 1)
+		return false;
+
+	ScriptInterface::GetScriptInterfaceAndCBData(cx)->pScriptInterface->FreezeObject(args[0], true);
+	return true;
+}
+
 bool ProfileStart(JSContext* cx, uint argc, jsval* vp)
 {
 	const char* name = "(ProfileStart)";
 
 	JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
@@ -373,10 +383,11 @@ ScriptInterface_impl::ScriptInterface_im
 	JS_DefineFunction(m_cx, globalRootedVal, "print", ::print,        0, JSPROP_ENUMERATE | JSPROP_READONLY | JSPROP_PERMANENT);
 	JS_DefineFunction(m_cx, globalRootedVal, "log",   ::logmsg,       1, JSPROP_ENUMERATE | JSPROP_READONLY | JSPROP_PERMANENT);
 	JS_DefineFunction(m_cx, globalRootedVal, "warn",  ::warn,         1, JSPROP_ENUMERATE | JSPROP_READONLY | JSPROP_PERMANENT);
 	JS_DefineFunction(m_cx, globalRootedVal, "error", ::error,        1, JSPROP_ENUMERATE | JSPROP_READONLY | JSPROP_PERMANENT);
 	JS_DefineFunction(m_cx, globalRootedVal, "deepcopy", ::deepcopy,  1, JSPROP_ENUMERATE | JSPROP_READONLY | JSPROP_PERMANENT);
+	JS_DefineFunction(m_cx, globalRootedVal, "deepfreeze", ::deepfreeze, 1, JSPROP_ENUMERATE | JSPROP_READONLY | JSPROP_PERMANENT);
 
 	Register("ProfileStart", ::ProfileStart, 1);
 	Register("ProfileStop", ::ProfileStop, 0);
 	Register("ProfileAttribute", ::ProfileAttribute, 1);

besides that it doesn't work. Potentially because it freezes an object while JS gets a new (unfrozen) object inheriting the frozen object (which is also what seems to happen for the this.entity / this.template properties in simulation components).

Got a preference whether to use the ScriptFunction dupes or the JS function? I'm leaning towards the latter, because we already have a use case for modifying that function and because it also calls official JS spec functions.

In D829#32753, @elexis wrote:

Got a preference whether to use the ScriptFunction dupes or the JS function? I'm leaning towards the latter, because we already have a use case for modifying that function and because it also calls official JS spec functions.

Not really.

Have you tried setting the rval to args[0], as is required for JSNative functions when returning true (well setting it to something is)? Since that is one of the very few differences between the actual implementation of Object.freeze and the code you have above. Check obj_freeze in libraries/source/spidermonkey/mozjs-38.0.0/js/src/builtin/Object.cpp.
If that'd work, that'd be my preferred variant.

implementation of Object.freeze in Object.cpp

Thanks, that's a good source!

Have you tried setting the rval to args[0]
one of the very few differences between the actual implementation of Object.freeze and the code you have above

Compared everything and share that observation.

Apparently I tested incorrectly before - the code actually does work as advertized.

If that'd work, that'd be my preferred variant.

Agree that it's nicer to have it in the ScriptInterface as a native function rather than one of those odd globalscripts.
The extended entity state issue can be solved by freezing all properties without freezing the entire object (as in moving that globalscripts loop to GetEntityState)

as is required for JSNative functions when returning true (well setting it to something is)?

Are you sure it is required? error, which appears to be defined in libraries/source/spidermonkey/mozjs-38.0.0/intl/icu/source/tools/genrb/errmsg.c doesn't do that either and the specs don't say anything about it:
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_DefineFunction
and it works appaerently fine without.

I've also removed the JSAutoRequest call, length check, used JS::Value, inlined everything and fixed the selection panels Upgrade part, which also changed a template (multiplied the cost with the a count).

binaries/data/mods/public/gui/session/selection_panels.js
262 ↗(On Diff #3298)

This changed g_TemplateData for the sole purpose of showing the tooltip. Hence moving that lookup just there.

597 ↗(On Diff #3298)

This changed the entity state. The helper property producingEnt was only in the location below.

elexis updated this revision to Diff 3311.Aug 25 2017, 2:56 AM

Don't use globalscripts but define a JS native deepfreeze.

If that'd work, that'd be my preferred variant.

Agree that it's nicer to have it in the ScriptInterface as a native function rather than one of those odd globalscripts.
The extended entity state issue can be solved by freezing all properties without freezing the entire object (as in moving that globalscripts loop to GetEntityState)

I guess I'll leave those details to someone with more of an idea of what we are actually doing here ;-)

as is required for JSNative functions when returning true (well setting it to something is)?

Are you sure it is required? error, which appears to be defined in libraries/source/spidermonkey/mozjs-38.0.0/intl/icu/source/tools/genrb/errmsg.c doesn't do that either and the specs don't say anything about it:
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_DefineFunction
and it works appaerently fine without.

On success, the callback must set JS::CallArgs::rval() or call JS_SET_RVAL (at least once) and return true.

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JSNative

I'm not really surprised that it does work without, or that the documentation differs from the code (welcome to the world of SpiderMonkey documentation), which is why checking the actual code and comments (js/CallArgs.h IIRC) on what is the case or planned (possibly asserting in case rval isn't set is a good idea.

I've also removed the JSAutoRequest call, length check, used JS::Value, inlined everything and fixed the selection panels Upgrade part, which also changed a template (multiplied the cost with the a count).

You also shouldn't rely on args[0], being valid, there is some .get() that should probably work. Then again I'd write that code in a way that it does handle unexpected input without failing.

Also worth considering would be what all this freezing means for hotloading. (Though we don't really hotload most of those data files, and if we did we should possibly make that more explicit.)

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/1913/ for more details.

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

binaries/data/mods/public/gui/common/tooltips.js
| 606| »   »   '[/color][/font]'·+·"·"·+
|    | [NORMAL] ESLintBear (no-useless-concat):
|    | Unexpected string concatenation of literals.

binaries/data/mods/public/gui/common/tooltips.js
| 439| »   »   let·[rate,·count]·=·types.reduce((sum,·t)·=>·{
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/common/tooltips.js
| 618| »   if·(walk·==·0·&&·run·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/common/tooltips.js
| 618| »   if·(walk·==·0·&&·run·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/selection_panels.js
|  60| »   »   »   switch·(data.item)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/selection_panels.js
|  74| »   »   switch·(data.item)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/selection_panels.js
| 345| »   »   »   tooltip·+=·"\n"·+·"[color=\"red\"]"·+·translate(formationInfo.tooltip)·+·"[/color]";
|    | [NORMAL] ESLintBear (no-useless-concat):
|    | Unexpected string concatenation of literals.

binaries/data/mods/public/gui/session/selection_panels.js
| 782| »   »   »   »   »   »   switch·(entity.check)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/selection_panels.js
|1010| »   »   »   »   "[/font]"·+·"·"·+·getEntityNamesFormatted(template),
|    | [NORMAL] ESLintBear (no-useless-concat):
|    | Unexpected string concatenation of literals.

binaries/data/mods/public/gui/session/selection_panels.js
| 282| »   »   if·(!technologyEnabled·||·limits.canBeAddedCount·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/selection_panels.js
| 443| »   »   »   »   »   "callback":·function·(item)·{·lockGate(item.locked);·}
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/selection_panels.js
| 449| »   »   »   »   »   "callback":·function·(item)·{·lockGate(item.locked);·}
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/selection_panels.js
| 499| »   »   »   if·(state.pack.progress·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/selection_panels.js
| 643| »   »   if·(data.i·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/selection_panels.js
| 686| »   »   »   »   tech·=>·tech·!=·null·&&·!ret.some(
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with 'null'.

binaries/data/mods/public/gui/session/selection_panels.js
| 686| »   »   »   »   tech·=>·tech·!=·null·&&·!ret.some(
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/selection_panels.js
| 699| »   »   »   »   ret·=·ret.concat(filteredTechs.map(tech·=>·({
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/selection_panels.js
| 732| »   »   pair.hidden·=·data.item.tech.pair·==·null;
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with 'null'.

binaries/data/mods/public/gui/session/selection_panels.js
| 767| »   »   »   ].map(func·=>·func(template));
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/selection_panels.js
| 813| »   »   »   button.onPress·=·function·()·{
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/selection_panels.js
| 822| »   »   »   »   button.onMouseEnter·=·function()·{
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/selection_panels.js
| 825| »   »   »   »   button.onMouseLeave·=·function()·{
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/selection_panels.js
|1042| »   »   if·(!technologyEnabled·||·limits.canBeAddedCount·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/selection_panels.js
|1150| »   »   »   if·(progress·||·!technologyEnabled·||·limits.canBeAddedCount·==·0·&&
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '+'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|1648|1648| 
|1649|1649| 		playerStatistics.economyScore += total + ",";
|1650|1650| 		playerStatistics.militaryScore += Math.round((player.sequences.enemyUnitsKilledValue[maxIndex] +
|1651|    |-			player.sequences.enemyBuildingsDestroyedValue[maxIndex]) / 10)  + ",";
|    |1651|+			player.sequences.enemyBuildingsDestroyedValue[maxIndex]) / 10) + ",";
|1652|1652| 		playerStatistics.totalScore += (total + Math.round((player.sequences.enemyUnitsKilledValue[maxIndex] +
|1653|1653| 			player.sequences.enemyBuildingsDestroyedValue[maxIndex]) / 10)) + ",";
|1654|1654| 

binaries/data/mods/public/gui/session/session.js
| 423| »   »   colorizeHotkey("%(hotkey)s"·+·"·",·"selection.idleworker")·+
|    | [NORMAL] ESLintBear (no-useless-concat):
|    | Unexpected string concatenation of literals.

binaries/data/mods/public/gui/session/session.js
| 443| »   »   let·panelEnt·=·g_PanelEntities.find(panelEnt·=>·panelEnt.slot·!==·undefined·&&·panelEnt.slot·==·slot);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'panelEnt' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
| 454| »   »   let·panelEnt·=·g_PanelEntities.find(panelEnt·=>·panelEnt.slot·!==·undefined·&&·panelEnt.slot·==·slot);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'panelEnt' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1000| »   »   let·panelEnt·=·g_PanelEntities.find(panelEnt·=>·ent·==·panelEnt.ent);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'panelEnt' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1029| »   let·getPanelEntNameTooltip·=·panelEntState·=>·"[font=\"sans-bold-16\"]"·+·template.name.specific·+·"[/font]";
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'panelEntState' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1105| »   »   button.onpress·=·(function(i)·{·return·function()·{·performGroup((Engine.HotkeyIsPressed("selection.add")·?·"add"·:·"select"),·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1106| »   »   button.ondoublepress·=·(function(i)·{·return·function()·{·performGroup("snap",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1107| »   »   button.onpressright·=·(function(i)·{·return·function()·{·performGroup("breakUp",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
| 864| »   »   i·==·0·||
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/session.js
| 866| »   »   g_GameAttributes.settings.PlayerData[i].AI·!=·"");
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with ''.

binaries/data/mods/public/gui/session/session.js
| 976| »   if·(direction·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/session.js
|1000| »   »   let·panelEnt·=·g_PanelEntities.find(panelEnt·=>·ent·==·panelEnt.ent);
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/session.js
|1104| »   »   button.hidden·=·g_Groups.groups[i].getTotalCount()·==·0;
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/session.js
|1105| »   »   button.onpress·=·(function(i)·{·return·function()·{·performGroup((Engine.HotkeyIsPressed("selection.add")·?·"add"·:·"select"),·i);·};·})(i);
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/session.js
|1106| »   »   button.ondoublepress·=·(function(i)·{·return·function()·{·performGroup("snap",·i);·};·})(i);
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/session.js
|1107| »   »   button.onpressright·=·(function(i)·{·return·function()·{·performGroup("breakUp",·i);·};·})(i);
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/session.js
|1112| »   »   »   let·icon·=·GetTemplateData(GetEntityState(g_Groups.groups[i].getEntsGrouped().reduce((pre,·cur)·=>·{
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/session.js
|1166| »   »   if·(player·!=·0·&&
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/gui/session/session.js
|1261| »   »   button.onpress·=·(function(e)·{·return·function()·{·selectAndMoveTo(e);·};·})(researchStarted[tech].researcher);
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/session.js
|1458| »   »   if·(+playerID·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.
Executing section XML GUI...
Executing section Python...
Executing section Perl...

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

In D829#32801, @leper wrote:

I guess I'll leave those [GUI] details to someone with more of an idea of what we are actually doing here ;-)

Sure. Code that wrote helper variables to the original template or entity state data was fixed to use an actual helper variable.

The only logic diff in the new JS patch is that we can't deepfreeze the entire entity state in GetEntityState, but only it's properties, because GetExtendedEntityState extends that the cache of the simple entity state.

On success, the callback must set JS::CallArgs::rval() or call JS_SET_RVAL (at least once) and return true.

New patch is complient with the linked specs, including the following sentence:

Otherwise it should either report an error (using e.g. JS_ReportError or JS_ReportOutOfMemory) or raise an exception (using JS_SetPendingException), and the callback must return false.

Setting the return value also has the advantage that we can return the frozen object, so we can inline this call instead of creating a new statement everywhere.
The deepfreeze function now complains if someone passes more or less than one argument that isn't an object.

what all this freezing means for hotloading

Currently we only hotload code. If we ever want to implement hotloading for templates, then the files holding the frozen cache should subscribe to the hotloading event.
They can then just overwrite the variable with the new data that is deeply frozen as well.
It may just not be marked as const.

I can split the C++ change from the JS change if you want.
Also D838 if we want ScriptInterface.cpp to be consistent.

elexis updated this revision to Diff 3327.Aug 26 2017, 11:14 PM

Add assertions to deepfreeze and set the return value.
Remove committed JS GUI fixes.

Stan added a subscriber: Stan.Aug 26 2017, 11:49 PM
Stan added inline comments.
source/scriptinterface/ScriptInterface.cpp
226 ↗(On Diff #3327)

Any reason that function does not have PascalCase ?

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

binaries/data/mods/public/gui/common/gamedescription.js
| 396| »   »   "label":·"[color=\""·+·g_DescriptionHighlight·+·"\"]"·+·title.label·+·":"·+·"[/color]",
|    | [NORMAL] ESLintBear (no-useless-concat):
|    | Unexpected string concatenation of literals.

binaries/data/mods/public/gui/common/gamedescription.js
|  89| »   »   if·(playerData·==·null·||·playerData.Civ·&&·playerData.Civ·==·"gaia")
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with 'null'.

binaries/data/mods/public/gui/common/gamedescription.js
|  94| »   »   let·isAI·=·playerData.AI·&&·playerData.AI·!=·"";
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with ''.

binaries/data/mods/public/gui/common/gamedescription.js
| 311| »   »   »   »   g_GameAttributes.settings.Ceasefire·==·0·?
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/replaymenu/replay_menu.js
| 100| »   »   if·(g_MapNames.indexOf(replay.attribs.settings.Name)·==·-1·&&·replay.attribs.settings.Name·!=·"")
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with ''.

binaries/data/mods/public/gui/replaymenu/replay_menu.js
| 181| »   if·(attribs.settings.PlayerData.length·&&·attribs.settings.PlayerData[0]·==·null)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with 'null'.
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before 'Engine'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/messages.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/messages.js
| 571| 571|  */
| 572| 572| function updateTimeNotifications()
| 573| 573| {
| 574|    |-	let notifications =  Engine.GuiInterfaceCall("GetTimeNotifications", g_ViewedPlayer);
|    | 574|+	let notifications = Engine.GuiInterfaceCall("GetTimeNotifications", g_ViewedPlayer);
| 575| 575| 	let notificationText = "";
| 576| 576| 	for (let n of notifications)
| 577| 577| 	{

binaries/data/mods/public/gui/session/messages.js
| 479| »   let·cheatCode·=·Object.keys(g_Cheats).find(cheatCode·=>·text.indexOf(cheatCode)·==·0);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'cheatCode' is already declared in the upper scope.

binaries/data/mods/public/gui/session/messages.js
| 601| »   while·(true)
|    | [NORMAL] ESLintBear (no-constant-condition):
|    | Unexpected constant condition.

binaries/data/mods/public/gui/session/messages.js
| 479| »   let·cheatCode·=·Object.keys(g_Cheats).find(cheatCode·=>·text.indexOf(cheatCode)·==·0);
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/messages.js
| 861| »   if·(chatAddressee.selected·>·0·&&·(text.indexOf("/")·!=·0·||·text.indexOf("/me·")·==·0))
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/gui/session/messages.js
| 861| »   if·(chatAddressee.selected·>·0·&&·(text.indexOf("/")·!=·0·||·text.indexOf("/me·")·==·0))
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/messages.js
|1075| »   let·isMe·=·msg.text.indexOf("/me·")·==·0;
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/messages.js
|1079| »   isMe·=·msg.text.indexOf("/me·")·==·0;
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/messages.js
|1197| »   »   if·(text.indexOf(pName·+·"·")·==·0·&&·pName.length·>·match.length)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '+'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|1636|1636| 
|1637|1637| 		playerStatistics.economyScore += total + ",";
|1638|1638| 		playerStatistics.militaryScore += Math.round((player.sequences.enemyUnitsKilledValue[maxIndex] +
|1639|    |-			player.sequences.enemyBuildingsDestroyedValue[maxIndex]) / 10)  + ",";
|    |1639|+			player.sequences.enemyBuildingsDestroyedValue[maxIndex]) / 10) + ",";
|1640|1640| 		playerStatistics.totalScore += (total + Math.round((player.sequences.enemyUnitsKilledValue[maxIndex] +
|1641|1641| 			player.sequences.enemyBuildingsDestroyedValue[maxIndex]) / 10)) + ",";
|1642|1642| 

binaries/data/mods/public/gui/session/session.js
| 410| »   »   colorizeHotkey("%(hotkey)s"·+·"·",·"selection.idleworker")·+
|    | [NORMAL] ESLintBear (no-useless-concat):
|    | Unexpected string concatenation of literals.

binaries/data/mods/public/gui/session/session.js
| 430| »   »   let·panelEnt·=·g_PanelEntities.find(panelEnt·=>·panelEnt.slot·!==·undefined·&&·panelEnt.slot·==·slot);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'panelEnt' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
| 441| »   »   let·panelEnt·=·g_PanelEntities.find(panelEnt·=>·panelEnt.slot·!==·undefined·&&·panelEnt.slot·==·slot);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'panelEnt' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
| 988| »   »   let·panelEnt·=·g_PanelEntities.find(panelEnt·=>·ent·==·panelEnt.ent);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'panelEnt' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1017| »   let·getPanelEntNameTooltip·=·panelEntState·=>·"[font=\"sans-bold-16\"]"·+·template.name.specific·+·"[/font]";
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'panelEntState' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1093| »   »   button.onpress·=·(function(i)·{·return·function()·{·performGroup((Engine.HotkeyIsPressed("selection.add")·?·"add"·:·"select"),·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1094| »   »   button.ondoublepress·=·(function(i)·{·return·function()·{·performGroup("snap",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1095| »   »   button.onpressright·=·(function(i)·{·return·function()·{·performGroup("breakUp",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
| 209| »   »   »   »   »   deepfreeze(g_EntityStates[entId][name])
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/gui/session/session.js
| 850| »   »   i·==·0·||
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/session.js
| 852| »   »   g_GameAttributes.settings.PlayerData[i].AI·!=·"");
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with ''.

binaries/data/mods/public/gui/session/session.js
| 962| »   if·(direction·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/session.js
| 988| »   »   let·panelEnt·=·g_PanelEntities.find(panelEnt·=>·ent·==·panelEnt.ent);
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/session.js
|1092| »   »   button.hidden·=·g_Groups.groups[i].getTotalCount()·==·0;
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/session.js
|1093| »   »   button.onpress·=·(function(i)·{·return·function()·{·performGroup((Engine.HotkeyIsPressed("selection.add")·?·"add"·:·"select"),·i);·};·})(i);
|    | [NORMAL] JSHintBear:
|    | Don't make functi

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

Build has FAILED

Link to build: http://jw:8080/job/phabricator/1924/
See console output for more information: http://jw:8080/job/phabricator/1924/console

what all this freezing means for hotloading

If we ever want to implement hotloading for templates, [...]

I might be wrong but I do seem to recall that we at least did do that at one point. (Not changing any previously existing entities, but new ones would use the new template.) Could be that some caching somewhere broke that at some point though.

Non-js part looks sensible apart from that, assuming it does build and vulkan/jenkins/phab is just acting up again.

source/scriptinterface/ScriptInterface.cpp
228 ↗(On Diff #3327)

Seems unneeded.

elexis marked 2 inline comments as done.Sep 3 2017, 2:50 PM
In D829#33671, @leper wrote:

what all this freezing means for hotloading

If we ever want to implement hotloading for templates, [...]

I might be wrong but I do seem to recall that we at least did do that at one point. (Not changing any previously existing entities, but new ones would use the new template.) Could be that some caching somewhere broke that at some point though.

I couldn't find any traces of Hotloading (TM (as in having hooks throughout the code that use that name)) code for templates.
But thinking about it, it sounds like hotloading (common noun) would have been supported by not caching templates but reloading them from the disk each time it is accessed.
In that case the mentioned approach (reusing old templates for old units) would have been (become?) broken by not serializing templates (as the new templates for old entities would be queried all over the place).

Since we don't want to support hotloading for players but could only use that feature for developers, serializing templates is off the table.
But one could try to achieve hotloading for both new and old entities.
JS components would likely complain, but could be fixed by adding Hotloading (TM) hooks.

This commit won't make it impossible to add hotloading, but merely would require a handful lines of code.
If performance is irrelevant, simply all templates could be reloaded if one template is changed (about 3 loc per template loader).
If performance is relevant, we could instead only freeze each template individually and replace it if some hotloading wants to (similar to what this patch does to g_EntityState).

Looking at all this made me discover the mainmenu and session hotloading code.
The gamesetup and session desperately needs to include the player assignments (as those pages claim to only have one player if someone hotloads code).
So thanks for the hotloading hint!

Non-js part looks sensible apart from that, assuming it does build and vulkan/jenkins/phab is just acting up again.

Thanks for the proofreading! I would have (and did) mess up a C++ implementation :P

source/scriptinterface/ScriptInterface.cpp
226 ↗(On Diff #3327)

Since the other ones defined here (see the hunk below) don't use it either.

elexis marked an inline comment as done.Sep 3 2017, 6:05 PM

@leper the other bug described above (this.template and this.entity being modifyable from JS components) was documented at #4759 and I don't see a solution other than making these two properties enumerable and changing the serialization code to account for that. If you have a hint or find a flaw in my research, that would be appreciated!

elexis added a comment.Sep 3 2017, 6:32 PM

Guess I have to close this by posting the Differential Revision tag in the commit message, despite it removing the C++ change from the default page loaded here.

This revision was automatically updated to reflect the committed changes.