Whenever looking at mapgen.js we see civ parsing code which is completely unrelated to the map generation. D900 moves it to a different function.
Instead it can be deleted altogether, because rmgen can load arbitrary JSON files as of rP20129.
We already have a JS function to enumerate and load civ JSON files, so we can reuse it instead of carrying a partial copy.
This allows deletion of gui/common/functions_civinfo.js.
Details
- Reviewers
• leper - Commits
- rP20528: Delete GetCivData from MapGeneratorWorker.cpp which is redundant with the…
- Trac Tickets
- #4868
The gamesetup should still have all civs selectable. The fortress map should still place walls of each civ.
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 3799 Build 6588: Vulcan Build (Windows) Jenkins Build 6587: Vulcan Build Jenkins Build 6586: arc lint + arc unit
Event Timeline
Can this be unified with the simulation civ file function? (I know that one just loads a single file, so maybe we should look at how that is used, since in most cases we end up loading more than one anyway.
binaries/data/mods/public/globalscripts/Templates.js | ||
---|---|---|
3 | Maybe explain why, also this should not be the case. | |
12 | Shouldn't that just always be Engine? I'd rather rename a few exposed functions than have this. Also scriptInterface is a bad name, since what you want here is some object, which in some cases is the global object and in some might not be. And if we need to pass something here I'd default to Engine and make the weird context do more work. | |
source/graphics/MapGenerator.cpp | ||
200 | It is still declared in the header. |
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/globalscripts/Templates.js | 94| » » ····················||·(c[0]·!=·"!"·&&·classes.indexOf(c)·!=·-1))) | | [NORMAL] JSHintBear: | | Misleading line break before '||'; readers may interpret this as an expression boundary.
binaries/data/mods/public/globalscripts/Templates.js | ||
---|---|---|
3 | An explanation would be either lengthy or incomplete and as weird as the weirdness it describes. Agree the differences shouldn't be there to begin with, which is why I wrote that ticket. But unifying it with the simulation should be done in a separate diff as that will have separate controversies and we likely don't want to look at GUI and rmgen code when discussing the simulation changes. The most important difference is the directory restriction for the simulation that we don't have for GUI and rmgen. Secondly the different function names. (possibly more differences as I didn't completely investigate the ComponentManager. Also I was surprised from D754 that the ComponentManager was accessible from the GUI.) | |
12 | It always is Engine, just that Engine is called RMS in the rmgen context (and there are about 700 references to that). Optional arguments mean people don't find out what they should pass unless the default doesn't coincidentally work as intended. | |
source/graphics/MapGenerator.cpp | ||
200 | Oups, thanks |
binaries/data/mods/public/globalscripts/Templates.js | ||
---|---|---|
3 | Different patch for merging the simulation sounds good. The directory restriction should be fixed by having a function to read simulation data files (or enumerat them), and expose that under the same name to all the different contexts. I'm quite certain that it isn't. The exported functions are named the same though. | |
12 | And there is a single caller of that in the whole rmgen. So using Engine here seems like the cleaner choice. I don't expect people to have to care about the fact that RMS is used in one place, and Engine everywhere else. Having that as an optional parameter is exactly about hiding something nobody should have to care about (the one call in rmgen is in the library...). Normally it shouldn't even be a parameter, but since the random map code is somewhat strange we should at least make this work as expected for everyone else, not force the "normal" code to change how it does things. |
binaries/data/mods/public/globalscripts/Templates.js | ||
---|---|---|
3 | ReadSimulationJSONFile, possibly. Will have to look in depth. There is only one cpp file that contains "BroadcastMessage" and that is the ComponentManager. We must fix that, it's quite bad to expose so many simulation functions to the GUI revealed by that diplo patch. | |
12 | We only have 2 calls to this function, one RMS and one GUI.
And they don't have to. Hiding values behind defaults when the caller has to care about the correctness of the value counteracts. I can rename from RMS to Engine for consistency when my rmgen branches have been transfered. However it's not inherently bad that variables can have arbitrary names, but a feature of object orientation that was specifically planned when C++ functions exposed to JS were moved from the global scope to objects in https://wildfiregames.com/forum/index.php?/topic/17289-discussion-spidermonkey-upgrade/ |
binaries/data/mods/public/globalscripts/Templates.js | ||
---|---|---|
3 | cmpGuiInterface is a simulation component. It's the place where GuiInterfaceCalls end up, which is the way to get information about the simulation from the GUI (and to some extent help visualize some things (instant feedback for rallypoints), but for those some care needs to be taken to not change the simulation state). | |
12 | And the simulation will not end up using this function after that future patch? The correct value is Engine in all cases apart from the random map context. And that one is non-standard in quite a few other ways, so making that jump through a single hoop to make the code nicer for everyone else is something we should do. Once the random map scripts have been cleaned up the hoop can be removed since it isn't needed anymore and nothing else has to change a lot anymore. The first post in that topic also talks about "Standardizing scripting access" which IMO includes having Engine everywhere and assuming that to be the correct thing. Not requiring code that is doing everything correctly (regarding this issue) to do more than it needs to, push the ugly compatibilty code on the ugly user. |
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/globalscripts/Templates.js | 94| » » ····················||·(c[0]·!=·"!"·&&·classes.indexOf(c)·!=·-1))) | | [NORMAL] JSHintBear: | | Misleading line break before '||'; readers may interpret this as an expression boundary.
binaries/data/mods/public/globalscripts/Templates.js | ||
---|---|---|
3 | Ah right, it goes through the GUIInterface, not directly from the GUI. Then I can sleep better. | |
12 | The simulation would add the third call to this function (SkirmishReplacer.js), but the argument is independent from the quantity of calls. Removing the argument would make the calls nicer to read, hiding the value in a fraction of calls makes it harder to read due to the additional effort to lookup the values of the call in multiple files. If RMS is renamed to Engine we can remove the scriptInterface argument from the 2 to 3 calls. If those were hundreds, I might agree that we could use the bad practice to ease the the future rename. (Just to have it stated, there is also the possibility that other scriptinterfaces are added later by us or mods and that more than one scriptinterface exists simultaneously. But I agree to assume that there is only one and that its called Engine until there is a documented use case). |
binaries/data/mods/public/globalscripts/Templates.js | ||
---|---|---|
12 | Those aren't scriptinterfaces, not really, those are objects that implement those functions. globalscripts is about being usable by most (read all) of the contexts. These functions should just need the minimal amount of parameters to do their thing. Requiring passing Engine (which is the default and the expected name) in cases where one should be able to assume that this is the expected thing (see Resources.js and l10n.js for other uses of Engine) is just wrong. If RMS is at one point renamed to Engine one can just remove the parameter and be done, no changes needed for any other callsites that do what is already expected. The random map scripts decided to be different, so they should have to pay the cost of being like that instead of doing the same thing as all the other code (that this is the case because of historic reasons is not really the point). The possibility that those aren't called Engine just means that someone decided to ignore any advice and do things differently because pointless work is fun. That said you could still just do var Engine = RMS; and be done. But I'll not accept this as long as we don't default to Engine and instead make code that follows what most of the code does jump through hoops just because RMS decided to be different for no reason. |
binaries/data/mods/public/globalscripts/Templates.js | ||
---|---|---|
12 |
Let's go for engine, shall we?
Making it optional doesn't increase the usability.
Not convinced, sorry.
Obfuscating the value makes every caller pay. The readers of the simulation are still confronted with the unwanted variability once they peel off the obfuscation.
Would buy that argument if it were an order of magnitude more calls than 3
So I have the choice to commit something which you consider a defect, something which I consider a defect, or something which multiple people consider a defect. I can offer to add the default and add "Some disagreements by me" to the commit message. Another possibility would be avoiding the argument altogether and setting let engine = Engine || RMS; // RMS is stupid to the globalscripts function.
is a good example where we could mitigate different code paths for each context by reducing the differences in C++
That can solely be used by the GUI of the public and modmod mod. |
Remove the second argument so that we don't have to discuss it being optional or not.
Fail hard if a civ file is empty or doesn't contain the SkirmisherReplacements entries.
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/globalscripts/Templates.js | 15| » » "SkirmishReplacements",·"Structures",·"StartEntities",·"Formations",·"AINames",·"SelectableInGameSetup"] | | [NORMAL] JSHintBear: | | Missing semicolon. binaries/data/mods/public/globalscripts/Templates.js | 96| » » ····················||·(c[0]·!=·"!"·&&·classes.indexOf(c)·!=·-1))) | | [NORMAL] JSHintBear: | | Misleading line break before '||'; readers may interpret this as an expression boundary.
binaries/data/mods/public/globalscripts/Templates.js | ||
---|---|---|
12 |
Still not that nice.
For non-RMS code it does.
The caller shouldn't need to care, so requiring it to care (in the non-RMS case) is making it pay.
Until we fix that part, which unless I missed something was the point of a certain ticket.
So we are going to write shitty code because it is only used twice currently?
Well, or you could change the RMS code to do var Engine = RMS; and have nice code in globalscripts.
See above. I still consider the current workaround somewhat ugly. >> Resources.js
Yes, and that is why we should have a function to load simulation data exposed to everyone. Wasn't unifying that the point of that ticket? The template code also does something similar (or did) since it gets slightly different data based on a few things.
You might have missed the markForTranslation functions there. Yes some care with not calling some things from some places needs to be taken, but having it like this seems nicer than having to do the caching in every gui page and including tons of files. |
binaries/data/mods/public/globalscripts/Templates.js | ||
---|---|---|
12 | The only usability that can be meant when comparing a mandatory argument with a default value from a mandatory argument without default value is perceived readability. No, renaming RMS to Engine was not the intention of the ticket. Deleting more than 50 shitty lines in exchange for, depending on the revision, one or two shitty lines that can be removed afterwards. I rather add one shitty line inside a function or rebase 20 patches than exposing a new proxy to more than 70 files that might, judging from the current usage, refer to it 10 times per file (opposed to loadCivData which is called once per context on init). Maybe. As long as we don't copy the entire function but call a function with the shared code with an argument. Possibly. |
Executing section Default... Executing section Source... Executing section JS... binaries/data/mods/public/globalscripts/Templates.js | 14| » » "SkirmishReplacements",·"Structures",·"StartEntities",·"Formations",·"AINames",·"SelectableInGameSetup"] | | [NORMAL] JSHintBear: | | Missing semicolon. binaries/data/mods/public/globalscripts/Templates.js | 95| » » ····················||·(c[0]·!=·"!"·&&·classes.indexOf(c)·!=·-1))) | | [NORMAL] JSHintBear: | | Misleading line break before '||'; readers may interpret this as an expression boundary.
binaries/data/mods/public/globalscripts/Templates.js | ||
---|---|---|
12 | Defaults: One example where I have used default values is the upcoming starting base unification patch. ReadJSONFile: l10n.js: Thanks for your comments and replies, even if I don't agree with all of them. |
binaries/data/mods/public/globalscripts/Templates.js | ||
---|---|---|
12 | (Besides, if it's preposterous to have more than one ScriptInterface simultaneously and a name different than Engine, then the nativeScopeArgument should be removed and fixed to Engine. |
binaries/data/mods/public/gui/common/functions_utility.js | ||
---|---|---|
48 | unneeded loop |
Implementing an unrestricted ReadJSONFile and a ReadSimulationJSONFile dilutes the point on unification and still doesn't prevent rmgen code from loading gui/ JSON files. One might argue that loading anything from gui/ is blatantly obviously a bad idea and thus not important to prevent in C++. But then that same argument would hold for simulation too. Not easy going. Tried two things (P95) and got nowhere. Retrying with a namespace.
Maybe provide the script interface with a list of VFS folder it can open stuff from from the C++ side, so that the RGMEN script interface could load from maps and sim and the other could be more or less flexible? Dunno.
How many JSON files that aren't in the simulation does the GUI load? Why does providing a special function for loading simulation files that can be used everywhere dilute things more than having the same function give different results based on where it is used? That seems a lot worse than not unifying everything, if that doesn't satisfy you I'm not sure anything can.
Also both of these could still use the same code by just having different script wrappers (shocking concept, I know).
binaries/data/mods/public/globalscripts/Templates.js | ||
---|---|---|
12 | Excuse me for not liking to fight windmills again. Adding new bad code is worse than having somewhat broken code that's already there. Neither of those is nice, but adding to the pile will not end well. Why have a need for the simulation to specify that it wants to read something from the simulation? Up to quite recently hardcoding Engine wasn't possible because there were places that didn't use it. Strange, isn't it? Then someone changed that, but decided a quick replace and a snarky comment was all that was needed. Sometimes interesting things happen and questions are only asked afterwards instead of before when one could actually figure out these things. |
rmgen/ needs to be able to read from simulation/ and maps/ but should for the same reason not be able to read from gui/.
ReadJSON seems simpler than ReadMapsJSON, ReadSimulationJSON, ReadGUIJSON, ReadAIJSON.
Given a quick grep there are 2 cases in which JSON files are read in maps/. That is not counting the one that reads civ files that already uses the helper. I'm not sure why we should have a generic function (that does not have the same behaviour everywhere) and then make things more complicated by restricting that. Two wrappers around the common function including the registration part should count in at something around 12 lines and that is counting blank lines.
ReadJSON seems simpler than ReadMapsJSON, ReadSimulationJSON, ReadGUIJSON, ReadAIJSON.
But why does Engine.ReadJSON(foo) have different behaviour in different places? That makes no sense.
Can someone clarify this whole debate you're having? I don't really understand the issue.
Is it that ReadJSONFile shouldn't, in general, be able to read files in /gui? If so, it's probably best to have a readJSON method for everything but GUI.
- The simulation shouldn't be able to read anything apart from files under simulation/.
- rmgen shouldn't be able to read anything apart from things in maps/ and simulation/.
- GUI can read anything, because why not.
I'm arguing for having ReadSimulationSomething that handles simulation/ and is exposed to all of those. Since we got that maps/ restriction explicitly specified just a short while ago some ReadMapSomething that is exposed to rmgen and GUI. And some ReadSomething for the GUI if that suddenly needs something else (the broken persistent map settings come to mind).
The point with having the specific restricted functions exposed to all that can read from there is that we can use those functions in globalscripts/ without having to do any special casing for who might be calling us.
Sure this adds a few partially (in the GUI case) overlapping functions, but it does not have the issue of the same function name having different behaviour based on where it is called.
With the same function name but the same restrictions as listed above a call ReadSomething("maps/foo") would fail in the simulation, but succeed for rmgen and GUI. Yes, this is a bad example, but you have one function (or what should be one, since the name is the same) that exhibits different behaviour based on who calls it. That seems horribly non-straightforward and very likely to cause confusion in the future.
Multiple functions exposed to JS that are actually just short wrappers roughly like the following
JS::Value JSI_Something::ReadSimulationSomething(auto& context, VfsPath path) { static const auto restriction = { "simulation/" }; return ReadSomethingWithPossibleRestrictionsOnPath(context, path, &restriction); }
does not seem very long or unwieldy. (The variant that takes nullptr for no restrictions is left as an exercise. I'm also quite sure that this is not the best possible code, but it should be plenty for illustration purposes.)
The generic function would have the same behavior everywhere.
Someone not knowing why gui/ files may not be loaded from simulation/ equally wonders why a gui loader function doesn't exist.
A generic function would inform the user that loading things outside of the restrictions violates the design of the folders.
lepers proposal is a definite and uncomplicated improvement to the current situation. But I have to explore more implementation before giving up on minimizing variability and code.
Alright, my 2 cents:
I too feel like a single function that warns/errors on "not-permitted" explicitly (describing what you say above) might be sufficient. It would be surprising the first time, but then you'd know what you can do. IO is the kind of thing where "permissions" are sort of implied, so I wouldn't be too surprised if different scripts act differently.
That being said I don't think leper's approach is bad, I just believe a single, error-explicit function would be slightly better.