Page MenuHomeWildfire Games

Unify simulation Engine.ReadJSON, simulation Engine.ReadCivJSONFile with the unified gui rmgen Engine.ReadJSON
ClosedPublic

Authored by elexis on Nov 29 2017, 7:44 PM.

Details

Summary

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.

Test Plan

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.

Event Timeline

elexis created this revision.Nov 29 2017, 7:44 PM
elexis added inline comments.Nov 29 2017, 7:54 PM
binaries/data/mods/public/globalscripts/Resources.js
22

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.
The DataTemplateManager could cache not only two but all three kinds of JSON files read from in the simulation and call that function.
Seems nicer to do in a separate diff to keep this one closer to its essence.

source/ps/scripting/JSInterface_VFS.cpp
196

If would have been nicer to abstract these functions.
The next patch after this one should unfiy the FileExist and ListFiles functions and those will need one wrapper each as well.

One wrapper per context per function is about a dozen of lines each.
That seems cheap and preferable over not having unifiable globalscripts/ code.

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.

[AI will] thus not require a copy of the simstate each turn.

Don't really see what that has to do with this?

[AI will] thus not require a copy of the simstate each turn.

Don't really see what that has to do with this?

[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

leper requested changes to this revision.Nov 29 2017, 9:57 PM
leper edited reviewers, added: Itms; removed: leper.

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.

Vulcan added a subscriber: Vulcan.Nov 29 2017, 11:00 PM
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.

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

@wraitii, what do you think about this? I see you gave input on D1062 so you are probably more in the loop than I am. The idea of unifying the functions with a check compartmentalizing the parts of the code looks good to me, what is your opinion on the maintainability issue leper brought up?

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

This should definitely be a starts-with of some kind.

238

The above is one of those cases where C++ is pretty ugly…
This might be a really terrible idea, but how about we call a JS function to warn users that it's not allowed?

elexis planned changes to this revision.Nov 30 2017, 12:50 PM

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

I won't be using boost

238

Why?

268

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.

wraitii added inline comments.Nov 30 2017, 1:40 PM
source/ps/scripting/JSInterface_VFS.cpp
227

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

cause then it's like

allowedPaths.map(x=> '"' + x + '"').join(",")
elexis updated this revision to Diff 4459.Nov 30 2017, 2:32 PM

Use a macro so that every script function needs to be defined only once instead of repeating it once per context.

elexis added inline comments.Nov 30 2017, 2:33 PM
source/ps/scripting/JSInterface_VFS.cpp
238

A string concatenation in JS is nicer than in C++.
But doing so here comes with additional costs of introducing a globalscripts function that doesnt fit anywhere and only contains this line and hardcoding JS function name in C++.
If we wouldn't have to allocate a new JS object and receive a less intuitive string, I'd have offered JSON stringify.
In rP19491 we used a stringstream, but that seems less readable (performance doesn't matter here since the program aborts after these microseconds)

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...
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.
wraitii added inline comments.Nov 30 2017, 3:15 PM
source/ps/scripting/JSInterface_VFS.cpp
31

Think you should put this above the function definition rather than at the top. Also constexpr perhaps? As we're going to drop VS13 soon

238

True. Well it was just a dumb suggestion anyways.

wraitii added inline comments.Nov 30 2017, 3:20 PM
source/ps/scripting/JSInterface_VFS.cpp
227

Had misread this as != end() initially, so never mind on the above, we're good to go.

elexis added inline comments.Nov 30 2017, 3:25 PM
source/ps/scripting/JSInterface_VFS.cpp
31

Shouldn't nonlocal variables always be mentioned at the top of the file?
I'm not familiar with VS2013 restrictions, can you explain me what you mean? Afaics constexpr means const and that the variable can be inlined, but const vectors and maps won't be dropped, will they?

wraitii added inline comments.Nov 30 2017, 3:47 PM
source/ps/scripting/JSInterface_VFS.cpp
31

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.

elexis marked an inline comment as done.Nov 30 2017, 6:10 PM

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

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?
And indeed the type ‘const std::map<CStr8, std::vector<CStrW> >’ of constexpr variable ‘pathRestriction’ is not literal

Correct, static was missing.

elexis updated this revision to Diff 4470.Nov 30 2017, 6:47 PM

static pathRestriction, remove two header function definitions and idling

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...
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.
elexis added inline comments.Dec 1 2017, 1:09 PM
source/ps/scripting/JSInterface_VFS.cpp
33

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.

wraitii accepted this revision.Dec 3 2017, 10:56 AM

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

Is that file just always supposed to be present?

source/gui/scripting/ScriptFunctions.cpp
70

Think this should be deleted?

source/ps/scripting/JSInterface_VFS.cpp
31

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?

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.

224–247

This function doesn't need to be part of JSI_VFS, move it above ReadJSonFile and it can exist only in this cpp file.

Thanks for your review wraitii, I hope I can make it up.

binaries/data/mods/public/simulation/helpers/Player.js
19

Certainly it cant be removed at this point and is relied upon by many places.
if you ask me it should be read from gamesetup, autostart-gamesetup and atlas but nothing else. The things starting a game should make sure they provide all things that must have a value IMO

source/ps/scripting/JSInterface_VFS.cpp
31

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.

224–247

doable

227

np

Itms added a comment.Dec 3 2017, 2:50 PM

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.

elexis added a comment.Dec 3 2017, 2:57 PM

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

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

elexis added a comment.Dec 3 2017, 3:07 PM

Not sure I get your second sentence though.

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.

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.

elexis updated this revision to Diff 4507.Dec 3 2017, 3:47 PM

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.

wraitii accepted this revision.Dec 3 2017, 4:25 PM

Fair enough, I do prefer it this way. I suppose the bigger cleanup could be done when your idea above gets implemented.

Vulcan added a comment.Dec 3 2017, 6:42 PM
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.
Vulcan added a comment.Dec 3 2017, 6:52 PM

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...
Itms removed a reviewer: Itms.Dec 3 2017, 7:47 PM

Thank you very much for the work on this!

This revision is now accepted and ready to land.Dec 3 2017, 7:47 PM
This revision was automatically updated to reflect the committed changes.
Owners added subscribers: Restricted Owners Package, Restricted Owners Package.Dec 4 2017, 12:02 AM