#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 by , 6 years ago
Milestone: | Backlog → Alpha 23 |
---|---|
Owner: | set to |
comment:2 by , 6 years ago
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 by , 6 years ago
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 by , 6 years ago
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 by , 6 years ago
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 by , 6 years ago
If you mean that
source/simulation2/system/ComponentManager.cpp
should not includesource/ps/scripting/JSInterface_VFS.cpp
but have that moved tosource/simulation2/scripting/
and included fromsource/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 by , 6 years ago
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 by , 6 years ago
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:10 by , 6 years ago
Patch: | → Phab:D1085 |
---|
comment:11 by , 6 years ago
Milestone: | Alpha 23 → Backlog |
---|---|
Owner: | removed |
comment:15 by , 6 years ago
Milestone: | Backlog → Alpha 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 by , 6 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
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 fromJSInterface_VFS.cpp
, so that these two could be unified directly and theGetCivData
file can be replaced by a call toReadJSONFile
.Also if we don't want rmgen code to be able to call
WriteJSONFile
, we could implement two differentRegister
functions.