In #4868 and D1062 we have discussed to unify the JSON reading functions. This is one of the patches required to unify globalscripts/Resources.js and globalscripts/Templates.js parsing.
The AI will be able to read JSON files too by adding very few code and thus not require a copy of the simstate each turn.
Details
Details
Add any Engine.ReadJSON("path/file") call to any of the contexts and see that the user is informed well why some read access is forbidden.
Thus the user doesn't even have to look into the code to find out why there is a restriction or why there are N different readjson functions.
Diff Detail
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| binaries/data/mods/public/globalscripts/Resources.js | ||
|---|---|---|
| 22 ↗ | (On Diff #4445) | Above code will have no path separation anymore, i.e. unified. loadCivFiles can have the comment about only being usable from gui/ and rmgen/ removed. |
| source/ps/scripting/JSInterface_VFS.cpp | ||
| 196 ↗ | (On Diff #4445) | If would have been nicer to abstract these functions. One wrapper per context per function is about a dozen of lines each. Not having to require one wrapper for each function by using some C++ magic (macros? templates? classes?) would yield less repetition, less to read, less to maintain, less surface for inconsistency bugs. But such voodoo can still be done later. Also we don't want another NativeWrapperDecls.h. |
Comment Actions
[AI will] thus not require a copy of the simstate each turn.
Don't really see what that has to do with this?
Comment Actions
[AI will] thus not require a copy of the resources object of the simstate each turn
See first two lines of the context of globalscripts/Resoruces.js
Comment Actions
Does not really seem any cleaner.
I still think having different functions is a lot cleaner, but have fun taking over maintainership for all involved code.
Comment Actions
Executing section Default...
Executing section Source...
Executing section JS...
binaries/data/mods/public/simulation/helpers/Player.js
| 62| » » var·civ·=·getSetting(playerData,·playerDefaults,·i,·"Civ");
| | [NORMAL] JSHintBear:
| | 'civ' is already defined.
binaries/data/mods/public/simulation/helpers/Player.js
| 63| » » var·template·=·cmpTemplateManager.TemplateExists("special/player_"+civ)·?·"special/player_"+civ·:·"special/player";
| | [NORMAL] JSHintBear:
| | 'template' is already defined.
binaries/data/mods/public/simulation/helpers/Player.js
| 64| » » var·entID·=·cmpPlayerManager.GetPlayerByID(i);
| | [NORMAL] JSHintBear:
| | 'entID' is already defined.
binaries/data/mods/public/simulation/helpers/Player.js
| 73| » for·(var·i·=·0;·i·<·numPlayers;·++i)
| | [NORMAL] JSHintBear:
| | 'i' is already defined.
binaries/data/mods/public/simulation/helpers/Player.js
| 135| » » » for·(var·j·=·0;·j·<·numPlayers;·++j)
| | [NORMAL] JSHintBear:
| | 'j' is already defined.
binaries/data/mods/public/simulation/helpers/Player.js
| 157| » » for·(var·i·=·0;·i·<·numPlayers;·++i)
| | [NORMAL] JSHintBear:
| | 'i' is already defined.Comment Actions
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...
Comment Actions
To me, this seems like trading off a little less maintainability on the C++ side for more JS side. Given that most of our spaghetti code (well the sim part anyways) has been JS side, I think that's likely a worthwhile tradeoff.
I'd prefer if there wasn't as much repetition, but haven't thought of a much better way to write this so far.
(not really that much more in the loop though :p )
| source/ps/scripting/JSInterface_VFS.cpp | ||
|---|---|---|
| 227 ↗ | (On Diff #4445) | This should definitely be a starts-with of some kind. |
| 238 ↗ | (On Diff #4445) | The above is one of those cases where C++ is pretty ugly… |
Comment Actions
Having only one copy means one only has to maintain one copy.
The directory and function name difference wasn't / isn't the only difference, but they also executed different code and returned different types.
I have an idea for a Macro to avoid repetition of the wrappers.
| source/ps/scripting/JSInterface_VFS.cpp | ||
|---|---|---|
| 227 ↗ | (On Diff #4445) | I won't be using boost |
| 238 ↗ | (On Diff #4445) | Why? |
| 268 ↗ | (On Diff #4445) | The context dependent functions could be moved to the functions, but the GUI context ScriptFunctions.cpp looks much nicer with only having one line each there. |
| source/ps/scripting/JSInterface_VFS.cpp | ||
|---|---|---|
| 227 ↗ | (On Diff #4445) | Right, I think that's either C++17 or C++20, still it should behave like one. Otherwise you can just name your JSON file "whaterversimulation/.json" and it's going to be loadable from anywhere. |
| 238 ↗ | (On Diff #4445) | cause then it's like allowedPaths.map(x=> '"' + x + '"').join(",") |
Comment Actions
Use a macro so that every script function needs to be defined only once instead of repeating it once per context.
| source/ps/scripting/JSInterface_VFS.cpp | ||
|---|---|---|
| 238 ↗ | (On Diff #4445) | A string concatenation in JS is nicer than in C++. |
Comment Actions
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...
Comment Actions
Executing section Default...
Executing section Source...
Executing section JS...
binaries/data/mods/public/simulation/helpers/Player.js
| 62| » » var·civ·=·getSetting(playerData,·playerDefaults,·i,·"Civ");
| | [NORMAL] JSHintBear:
| | 'civ' is already defined.
binaries/data/mods/public/simulation/helpers/Player.js
| 63| » » var·template·=·cmpTemplateManager.TemplateExists("special/player_"+civ)·?·"special/player_"+civ·:·"special/player";
| | [NORMAL] JSHintBear:
| | 'template' is already defined.
binaries/data/mods/public/simulation/helpers/Player.js
| 64| » » var·entID·=·cmpPlayerManager.GetPlayerByID(i);
| | [NORMAL] JSHintBear:
| | 'entID' is already defined.
binaries/data/mods/public/simulation/helpers/Player.js
| 73| » for·(var·i·=·0;·i·<·numPlayers;·++i)
| | [NORMAL] JSHintBear:
| | 'i' is already defined.
binaries/data/mods/public/simulation/helpers/Player.js
| 135| » » » for·(var·j·=·0;·j·<·numPlayers;·++j)
| | [NORMAL] JSHintBear:
| | 'j' is already defined.
binaries/data/mods/public/simulation/helpers/Player.js
| 157| » » for·(var·i·=·0;·i·<·numPlayers;·++i)
| | [NORMAL] JSHintBear:
| | 'i' is already defined.| source/ps/scripting/JSInterface_VFS.cpp | ||
|---|---|---|
| 227 ↗ | (On Diff #4445) | Had misread this as != end() initially, so never mind on the above, we're good to go. |
| source/ps/scripting/JSInterface_VFS.cpp | ||
|---|---|---|
| 31 ↗ | (On Diff #4459) | Shouldn't nonlocal variables always be mentioned at the top of the file? |
| source/ps/scripting/JSInterface_VFS.cpp | ||
|---|---|---|
| 31 ↗ | (On Diff #4459) | Well we declare globals at the top, but this is eminently local. In fact it should be static const, and put somewhere around the macro imo. "static" would make it unavailable to other compilation units. I don't have a complete mastery of constexpr admittedly, so I'll link http://en.cppreference.com/w/cpp/language/constexpr, but my understanding is that it's supposed to tell the compiler that this can be evaluated at compile-time, which is much stronger than const. Now in this case I'm actually wrong as none of this is a literal type so it wouldn't compile, but it'd still be better if possible, as the compiler could inline/precompute it all. Might be possible come C++20 or so. |
Comment Actions
If this patch is finished and globalscripts/ polished, then we could add a functionRestriction map similar to the pathRestriction map that stores which function will be compiled into that engine compartment.
This would mean that the three RegisterScriptFunctions_FOO could be unified too.
Then the two restriction constants could be moved to the caller of JSI_VFS::RegisterScriptFunctions.
Thus making JSInterface_VFS.cpp agnostic of engine compartments again.
I didn't explore the feasibility of this approach yet.
| source/ps/scripting/JSInterface_VFS.cpp | ||
|---|---|---|
| 31 ↗ | (On Diff #4459) | Constants at the top also help documenting the concepts used by the code that follows and configuration before being confronted with code. Are(n't) C++ compilers we care about are smart enough to evaluate at compile time that a constant map of constant vectors can be inlined? Correct, static was missing. |
Comment Actions
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...
Comment Actions
Executing section Default...
Executing section Source...
Executing section JS...
binaries/data/mods/public/simulation/helpers/Player.js
| 62| » » var·civ·=·getSetting(playerData,·playerDefaults,·i,·"Civ");
| | [NORMAL] JSHintBear:
| | 'civ' is already defined.
binaries/data/mods/public/simulation/helpers/Player.js
| 63| » » var·template·=·cmpTemplateManager.TemplateExists("special/player_"+civ)·?·"special/player_"+civ·:·"special/player";
| | [NORMAL] JSHintBear:
| | 'template' is already defined.
binaries/data/mods/public/simulation/helpers/Player.js
| 64| » » var·entID·=·cmpPlayerManager.GetPlayerByID(i);
| | [NORMAL] JSHintBear:
| | 'entID' is already defined.
binaries/data/mods/public/simulation/helpers/Player.js
| 73| » for·(var·i·=·0;·i·<·numPlayers;·++i)
| | [NORMAL] JSHintBear:
| | 'i' is already defined.
binaries/data/mods/public/simulation/helpers/Player.js
| 135| » » » for·(var·j·=·0;·j·<·numPlayers;·++j)
| | [NORMAL] JSHintBear:
| | 'j' is already defined.
binaries/data/mods/public/simulation/helpers/Player.js
| 157| » » for·(var·i·=·0;·i·<·numPlayers;·++i)
| | [NORMAL] JSHintBear:
| | 'i' is already defined.| source/ps/scripting/JSInterface_VFS.cpp | ||
|---|---|---|
| 33 ↗ | (On Diff #4470) | Could also be simulation/data/. Depends on whether we wish to enforce our will to only use that subdirectory on mods and future vanilla developers. Doesn't really seem needed to manifest it here. |
Comment Actions
I think this is acceptable, see comments before committing. I haven't actually verified that this changes every instant of JSON reading, but it seems to work and the code looks correct.
| binaries/data/mods/public/simulation/helpers/Player.js | ||
|---|---|---|
| 19 ↗ | (On Diff #4470) | Is that file just always supposed to be present? |
| source/gui/scripting/ScriptFunctions.cpp | ||
| 70 ↗ | (On Diff #4470) | Think this should be deleted? |
| source/ps/scripting/JSInterface_VFS.cpp | ||
| 216 ↗ | (On Diff #4470) | This function doesn't need to be part of JSI_VFS, move it above ReadJSonFile and it can exist only in this cpp file. |
| 31 ↗ | (On Diff #4459) |
Inlined, probably, but "completely taken apart and basically meta-programmed in", probably not. I still think it should go above/inside the function where it's used, but it's not a huge thing. |
Comment Actions
Thanks for your review wraitii, I hope I can make it up.
| binaries/data/mods/public/simulation/helpers/Player.js | ||
|---|---|---|
| 19 ↗ | (On Diff #4470) | Certainly it cant be removed at this point and is relied upon by many places. |
| source/ps/scripting/JSInterface_VFS.cpp | ||
| 216 ↗ | (On Diff #4470) | doable |
| 31 ↗ | (On Diff #4459) | The last word wasnt spoken in this file anyway. I'm eager to see if this file can be come agnostic of the scripts afterwards. |
| 227 ↗ | (On Diff #4445) | np |
Comment Actions
Thank you very much for looking at this @wraitii! ? I'd just like to see the last iteration uploaded following your remarks and I'll remove myself as blocking.
Comment Actions
@wraitii We could remove the other functions from the VFS namespace too. The only reason why I didn't make that Script_ReadJSONFile_Foo part of the VFS namespace is because it would require the same macro in the header. Shouldn't we use the namespace preferably?
Comment Actions
@wraitii We could remove the other functions from the VFS namespace too. The only reason why I didn't make that Script_ReadJSONFile_Foo part of the VFS namespace is because it would require the same macro in the header. Shouldn't we use the namespace preferably?
The distinction imo is that PathRestrictionMet is a helper that shouldn't be used outside this cpp file, so it shouldn't be part of the namespace. The registration probably make more sense within it.
Not sure I get your second sentence though.
Comment Actions
One advantage of using namespaces is also to avoid naming conflicts. For instance if there is a global PathRestrictionMet included from some other place that might conflict with the helper.
Not impossible to use PathRestrictionMet externally, but no well defined use case either, so ok for me to remove it from the namespace.
Comment Actions
One advantage of using namespaces is also to avoid naming conflicts. For instance if there is a global PathRestrictionMet included from some other place that might conflict with the helper.
Ah, right. But that other PathRestrictionMet would be in a specific namespace himself (unless somebody was naughty), so we could just call ::PathRestrictionMet here or something.
Comment Actions
Remove empty line, make the path restrictions three #defines instead of a const map.
Not fully convinced yet to make PathRestrictionsMet a local helper function, because then ReadJSONFile should become one too (which can currently still be called globally), and likely the others as well.
The function would also have to be moved above the calls, ie. would be the first function called.
It would be really nice to avoid manifestating the contexts in the VFS interface file.
As long as RegisterFunction doesn't work with templates, we would however not receive the advantage of the MACRO and had to copy the getters to each context.
Comment Actions
Fair enough, I do prefer it this way. I suppose the bigger cleanup could be done when your idea above gets implemented.
Comment Actions
Executing section Default...
Executing section Source...
Executing section JS...
binaries/data/mods/public/simulation/helpers/Player.js
| 62| » » var·civ·=·getSetting(playerData,·playerDefaults,·i,·"Civ");
| | [NORMAL] JSHintBear:
| | 'civ' is already defined.
binaries/data/mods/public/simulation/helpers/Player.js
| 63| » » var·template·=·cmpTemplateManager.TemplateExists("special/player_"+civ)·?·"special/player_"+civ·:·"special/player";
| | [NORMAL] JSHintBear:
| | 'template' is already defined.
binaries/data/mods/public/simulation/helpers/Player.js
| 64| » » var·entID·=·cmpPlayerManager.GetPlayerByID(i);
| | [NORMAL] JSHintBear:
| | 'entID' is already defined.
binaries/data/mods/public/simulation/helpers/Player.js
| 73| » for·(var·i·=·0;·i·<·numPlayers;·++i)
| | [NORMAL] JSHintBear:
| | 'i' is already defined.
binaries/data/mods/public/simulation/helpers/Player.js
| 135| » » » for·(var·j·=·0;·j·<·numPlayers;·++j)
| | [NORMAL] JSHintBear:
| | 'j' is already defined.
binaries/data/mods/public/simulation/helpers/Player.js
| 157| » » for·(var·i·=·0;·i·<·numPlayers;·++i)
| | [NORMAL] JSHintBear:
| | 'i' is already defined.Comment Actions
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...