Opened 22 months ago

Closed 22 months ago

Last modified 21 months ago

#4868 closed defect (fixed)

Unify VFS script functions

Reported by: elexis Owned by:
Priority: Should Have Milestone: Alpha 23
Component: Core engine Keywords:
Cc: Patch:

Description

ReadJSONFile is called by simulation/, globalscripts/ and maps/ of the public mod. ai/` might want to use it eventually too.

This and other JSON functions are registered by the following files:

MapGenerator.cpp 
source/ps/scripting/JSInterface_VFS.cpp
simulation2/system/ComponentManager.cpp

They all proxies to ScriptInterface.ReadJSONFile.

JSInterface_VFS.cpp has recently been split from the gui ScriptFunctions.cpp. Since it contains all JSON relevant functions, it could just be included from the other c++ files and the incomplete, differently named proxy copies can be deleted.

This way we can add the missing JSON functions to rmgen interface by removing code. Unfortunately the functions have naming differences, so there is also some JS modification needed.

Additionally, GetCivData seems pointless if JS can load arbitrary JSON files. source/ should become as agnostic of 0ad vocabulary as possible without removing useful features.

The naming unification will solve multiple old resources.js and possibly ai/ problems too:

/**
 * Since the AI context can't access JSON functions, it gets passed an object
 * containing the information from `GuiInterface.js::GetSimulationState()`.
 */
function Resources()
{
	let jsonFiles = [];
	// Simulation context
	if (Engine.FindJSONFiles)
	{
		jsonFiles = Engine.FindJSONFiles("resources", false);
		for (let file in jsonFiles)
			jsonFiles[file] = "resources/" + jsonFiles[file] + ".json";
	}
	// GUI context
	else if (Engine.BuildDirEntList)
		jsonFiles = Engine.BuildDirEntList("simulation/data/resources/", "*.json", false);
	else
	{
		error("Resources: JSON functions are not available");
		return;
	}

Change History (20)

comment:1 Changed 22 months ago by elexis

Milestone: BacklogAlpha 23
Owner: set to elexis

Phab:D900 will change the Civ loading, so the patch might appear a bit nicer if the civ proxy is removed here while at it.

The gui ScriptFunctions.cpp was split in #4772.

r20129 implemented the biome JSON files and copied without modifying the ReadJSONFile proxy from JSInterface_VFS.cpp, so that these two could be unified directly and the GetCivData file can be replaced by a call to ReadJSONFile.

Also if we don't want rmgen code to be able to call WriteJSONFile, we could implement two different Register functions.

comment:2 Changed 22 months ago by elexis

We can also eventually get rid of the DataTemplateManager simulation system component (including the entire unwanted serialization, #3834, #4239) and do all template loading (including aura, tech and civ bonus loading) in globalscripts/Templates.js as mentioned in the last sentence of comment:13:ticket:4239.

comment:3 Changed 22 months ago by elexis

In r20507:

Unify two rmgen and GUI JSON file loading script functions, refs #4868.

Revert the introduction of the exact copies of FileExists, ReadJSONFile functions in MapGenerator.cpp introduced by r20129.
Include JSInterface_VFS.cpp from MapGenerator.cpp to remove that code while gaining the other missing VFS file loading functions in rmgen/.
Split RegisterScriptFunctions of JSInterface_VFS.cpp from r20133 into RegisterReadOnlyScriptFunctions and RegisterWriteScriptFunctions to prevent unintentional write access.

comment:4 Changed 22 months ago by leper

The simulation has all these specific prefix functions to load files since it should not depend on anything outside the simulation. Some parts (globalscripts for templates) have to do some work because the same (or similar name) loads files from different places.

So the best way to handle that would be to possibly rename the simulation function to include "simulation" and also expose those to the other context(s).

I don't really see a huge benefit in removing the civ part from the engine. If a mod does not want to use that then it shouldn't, if it does well it isn't limited to 0ad. source/ is still an rts engine (even if we remove most of the 0ad "specific" pieces), and if someone makes an rts that does not have any sort of factions or civs then they can just not call that function.

comment:5 Changed 22 months ago by elexis

these specific prefix functions
rename to include "simulation" and also expose those to the other context(s).

If you mean that source/simulation2/system/ComponentManager.cpp should not include source/ps/scripting/JSInterface_VFS.cpp but have that moved to source/simulation2/scripting/ and included from source/gui/scripting/ScriptFunctions.cpp then I'll be happy to serve your request.

benefit in removing the civ part from the engine

Mostly that load civ json file being pointlessly redundant with load json file.

comment:6 Changed 22 months ago by leper

If you mean that source/simulation2/system/ComponentManager.cpp should not include source/ps/scripting/JSInterface_VFS.cpp but have that moved to source/simulation2/scripting/ and included from source/gui/scripting/ScriptFunctions.cpp then I'll be happy to serve your request.

It does not include that, unless I missed something.

Mostly that load civ json file being pointlessly redundant with load json file.

Except the point is that the simulation doesn't have a generic load json file function at all. Though given that civ files are within simulation/data/ having one function for all of those seems ok.

comment:7 Changed 22 months ago by elexis

It does not include that, unless I missed something.

Removing the duplicate JSON script loading functions is the thing intended in this ticket.

Except the point is that the simulation doesn't have a generic load json file function at all.

I share the observation that ReadJSONFile and FindJSONFiles of ComponentManager.cpp exposed to the JS simulation code are restricted to simulation/data/ while the gui/ JSON loading code can load gui/ files too. I agree that it's good for many reasons to prevent any simulation code from loading GUI or other non-simulation JSON files. Keeping this restriction while removing the redundancy shouldn't be an issue, because we can pass the directory restrictions in the registration function.

It would be equally relevant if the CCmpAIManager.cpp JSON loading code would be moved from C++ to JS and enable loading of tech/aura/civ files from the public mod AI code.

The rmgen/ code currently wants to load JSON files from maps/random/rmbiome/ and simulation/data/civs/. So a restriction in MapGenerator.cpp would require a whitelist of at least two directores (which could still be passed as a vector).

Amongst the benefit of removing duplication, it also has the benefit of reducing unneeded variability, for instance having a function named BuildDirEntList in the GUI but FindJSONFiles in the simulation. Another example of unbenefitial variability is that some JSON loading functions pass the parsed JSON while other functions require the text to JS object parsing in JS (GetCivData in rmgen/). These things only complicate things. Code should be as simple as possible, as versatile as possible, as restricted as needed.

comment:8 Changed 22 months ago by leper

Differently named functions seem better.

I don't really see a need for changing the AIManager. Allowing the AI to load other JSON files, well yes if that seems useful.

rmgen/ does not really need an explicit whitelist.

They also have different names because they can access different files.

comment:9 Changed 22 months ago by elexis

In 20528:

Delete GetCivData? from MapGeneratorWorker?.cpp which is redundant with the ReadJSONFile from rP20129.
Unify civ file loading from gui/common/functions_civinfo.js and rmgen/library.js in globalscripts/Templates.js.
Delete the two forgotton headers in rP20507.

Refs #4868, #4804, D900.
Differential Revision: https://code.wildfiregames.com/D1062
Discussed with: leper

comment:10 Changed 22 months ago by elexis

Patch: Phab:D1085

comment:11 Changed 22 months ago by elexis

Milestone: Alpha 23Backlog
Owner: elexis deleted

comment:12 Changed 22 months ago by elexis

In 20576:

Expose the same ReadJSONFile function to simulation, GUI and rmgen.

This way globalscripts/ can use the same code in all contexts, all paths are absolute and only one function name is needed.
Remove ReadCivJSONFile which is redundant with this function as well.
Restrict read access of rmgen/ to rmgen/ and simulation/, refs rP20507, rP20528.

Refs #4868
Differential Revision: https://code.wildfiregames.com/D1085
Reviewed By: wraitii
Partial agreement with leper

comment:13 Changed 22 months ago by elexis

In 20586:

Expose the same file listing function to JS GUI, simulation and rmgen, refs #4868.

Removes the workaround and limitations of globalscripts Resources (rP18964) and loadCivFiles (rP20528), allowing them to actually become actually global.
Rename to ListDirectoryFiles? as proposed by wraitii.

Differential Revision: https://code.wildfiregames.com/D1103
Reviewed By: s0600204
Proofread by: echotangoecho
Comments by: wraitii
Relevant discussions with leper in #4868, D1062

comment:14 Changed 22 months ago by elexis

In 20588:

Expose the same FileExists? to JS GUI, simulation and rmgen.

Thus restrict rmgen FileExists? from r20129 to rmgen/ and simulation/.

Refs #4868, rP20551
Differential Revision: https://code.wildfiregames.com/D1104
Same rap as in rP20576, rP20586.

comment:15 Changed 22 months ago by elexis

Milestone: BacklogAlpha 23
Patch: Phab:D1085

Unification of VFS scripts is complete.

The DataTemplateManager should become a globalscripts as described here, but this is out of scope of the ticket (Phab:D1108). The AI can now benefit from loading JSON files, but also out of scope.

Would be nice to make the JSInterface_VFS.cpp agnostic of the simulation, GUI, maps contexts, but I don't see how to achieve that without introducing 3 or 4 copies of the wrappers to these contexts. (Also doesn't seem too bad to see the registrations in one place)

comment:16 Changed 22 months ago by elexis

Resolution: fixed
Status: newclosed

comment:17 Changed 22 months ago by elexis

In 20600:

Allow the AI to read JSON simulation files and use the Resources prototype from rP18964 for the AI directly.
Removes the workaround copy of the resources JSON each turn in GetSimulationState?.

Refs #3934, #4868
Differential Revision: https://code.wildfiregames.com/D1119
Reviewed By: mimo

comment:18 Changed 22 months ago by elexis

In 20610:

Remove unused serialized cache of technology templates in TechnologyManager?.researchedTechs.

Refs #3834, #3909, #4239, #4868, D1108
Differential Revision: https://code.wildfiregames.com/D1130
Reviewed By: mimo

comment:19 Changed 21 months ago by elexis

In 20692:

JS file listing cleanup, refs #4868.

Add global filelisting helper.
Remove getXMLFileList and getJSONFileList from the unsorted gui/common/functions_utility.js.
Make hidden maps prefix more transparent.
Remove hardcoded magic numbers that are string lengths.
Shorten loading screen code and separate pathnames from code.

Differential Revision: https://code.wildfiregames.com/D1107
Reviewed By: bb

comment:20 Changed 21 months ago by elexis

In 20737:

Replace DataTemplateManager? simulation component with a globalscript, refs #4868.

Removes the serialization of JSON files, shrinking savegame files and rejoin states sent across the network, refs #3834, #4239, #3909, rP18100.
Removes the AI C++ code to read JSON files from rP13225 since the AI can now use the globalscript.
Allows the AI to read Aura templates and removal of GUIInterface code to improve performance.
Serialization of the JSON objects in other simulation components was removed in r20606 / D1109, r20610 / D1130.

Serialization removal planned by sanderd17
AI part proofread by mimo
Simulation part proofread by bb
Discussed with Itms on irc

Differential Revision: https://code.wildfiregames.com/D1108

Note: See TracTickets for help on using tickets.