Page MenuHomeWildfire Games

Delete CMapGeneratorWorker::GetCivData, badly located JS rmgen civ data parsing code and gui/common/functions_civinfo.js
ClosedPublic

Authored by elexis on Nov 25 2017, 2:01 AM.

Details

Summary

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.

Test Plan

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Nov 25 2017, 2:01 AM
leper requested changes to this revision.Nov 25 2017, 2:38 AM

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 ↗(On Diff #4352)

Maybe explain why, also this should not be the case.

12 ↗(On Diff #4352)

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 ↗(On Diff #4352)

It is still declared in the header.

This revision now requires changes to proceed.Nov 25 2017, 2:38 AM
Vulcan added a subscriber: Vulcan.Nov 25 2017, 2:56 AM

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.
elexis added inline comments.Nov 25 2017, 3:08 AM
binaries/data/mods/public/globalscripts/Templates.js
3 ↗(On Diff #4352)

An explanation would be either lengthy or incomplete and as weird as the weirdness it describes.
Better fix the weirdness than explain it.

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 ↗(On Diff #4352)

It always is Engine, just that Engine is called RMS in the rmgen context (and there are about 700 references to that).
Engine is not the global object.
Engine and RMS are the nativeScopeName of the ScriptInterface.cpp instance.

Optional arguments mean people don't find out what they should pass unless the default doesn't coincidentally work as intended.
Also optional arguments mean that the value is hidden from from the caller.

source/graphics/MapGenerator.cpp
200 ↗(On Diff #4352)

Oups, thanks

elexis updated this revision to Diff 4353.Nov 25 2017, 3:11 AM

Delete header entries, including the two forgotton in rP20507.

leper requested changes to this revision.Nov 25 2017, 3:24 AM
leper added inline comments.
binaries/data/mods/public/globalscripts/Templates.js
3 ↗(On Diff #4352)

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 ↗(On Diff #4352)

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.

This revision now requires changes to proceed.Nov 25 2017, 3:24 AM
elexis added inline comments.Nov 25 2017, 4:03 AM
binaries/data/mods/public/globalscripts/Templates.js
3 ↗(On Diff #4352)

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 ↗(On Diff #4352)

We only have 2 calls to this function, one RMS and one GUI.

I don't expect people to have to care about the fact that RMS is used in one place, and Engine everywhere else

And they don't have to.
Readers of the simulation code don't have to know the name of the scriptinterface in RMS, GUI, AI or any other place.
They only have to know the name in the simulation.

Hiding values behind defaults when the caller has to care about the correctness of the value counteracts.
The function needs to be called in all cases with the correct value, hence the value should be visible so it can be validated.

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/

leper added inline comments.Nov 25 2017, 4:12 AM
binaries/data/mods/public/globalscripts/Templates.js
3 ↗(On Diff #4352)

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 ↗(On Diff #4352)

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.
elexis added inline comments.Nov 25 2017, 5:27 AM
binaries/data/mods/public/globalscripts/Templates.js
3 ↗(On Diff #4352)

Ah right, it goes through the GUIInterface, not directly from the GUI. Then I can sleep better.

12 ↗(On Diff #4352)

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 values are hidden they are not proofread and thus correctness is rather coincidental, so default values are a bad code pattern.
Especially globalscripts/ ought to avoid default values and hand over the responsability to each caller.

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

leper added inline comments.Nov 25 2017, 6:03 AM
binaries/data/mods/public/globalscripts/Templates.js
12 ↗(On Diff #4352)

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.

elexis added inline comments.Nov 25 2017, 12:46 PM
binaries/data/mods/public/globalscripts/Templates.js
12 ↗(On Diff #4352)

scriptinterfaces, not really

Let's go for engine, shall we?

globalscripts is about being usable by all contexts

Making it optional doesn't increase the usability.

Just wrong

Not convinced, sorry.

RMS should pay the cost of being like that

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.

If renamed, no changes needed

Would buy that argument if it were an order of magnitude more calls than 3

I'll not accept

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.

Resources.js

is a good example where we could mitigate different code paths for each context by reducing the differences in C++

l10n.js

That can solely be used by the GUI of the public and modmod mod.
Maybe it could be moved somewhere to prevent simulation calling into that.

elexis updated this revision to Diff 4363.Nov 25 2017, 1:08 PM

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.
leper added inline comments.Nov 25 2017, 10:40 PM
binaries/data/mods/public/globalscripts/Templates.js
12 ↗(On Diff #4352)

Let's go for engine, shall we?

Still not that nice.

Making it optional doesn't increase the usability.

For non-RMS code it does.

Obfuscating the value makes every caller pay.

The caller shouldn't need to care, so requiring it to care (in the non-RMS case) is making it pay.

The readers of the simulation are still confronted with the unwanted variability once they peel off the obfuscation.

Until we fix that part, which unless I missed something was the point of a certain ticket.

Would buy that argument if it were an order of magnitude more calls than 3

So we are going to write shitty code because it is only used twice currently?

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.

Well, or you could change the RMS code to do var Engine = RMS; and have nice code in globalscripts.

Another possibility would be avoiding the argument altogether and setting let engine = Engine || RMS; // RMS is stupid to the globalscripts function.

See above. I still consider the current workaround somewhat ugly.

>> Resources.js

is a good example where we could mitigate different code paths for each context by reducing the differences in C++

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.

l10n.js

That can solely be used by the GUI of the public and modmod mod.
Maybe it could be moved somewhere to prevent simulation calling into that.

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.

elexis added inline comments.Nov 25 2017, 11:37 PM
binaries/data/mods/public/globalscripts/Templates.js
12 ↗(On Diff #4352)

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.
The latter then implies luck that the default value is the correct value.
Otherwise we pay with additional cost to lookup the used value.
Hence in most cases default values are an anti-pattern.

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.

elexis updated this revision to Diff 4378.Nov 25 2017, 11:43 PM

weirjgwiefg5 z+3zq3prg dfg.

Build failure - The Moirai have given mortals hearts that can endure.

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.
elexis added inline comments.Nov 26 2017, 1:53 PM
binaries/data/mods/public/globalscripts/Templates.js
12 ↗(On Diff #4352)

Defaults: One example where I have used default values is the upcoming starting base unification patch.
There are like 20 or 30 default values used in the new function call in every of the about 55 files. One wrong number can easily introduce an undesired effect, so one still has to consider the implied values with great care (which is why its still not commited).
So the cost to either lookup the default values each time or memorize them is an addition in comparison to repeating the defaults in every file.
However I believe that in this case this additional cost is preferable because it means we have like 25*55 = 1375 less lines of code.
(I didn't forget to update the wiki page, I just want to finish transfering the code I kind of finished months ago, so that I can write the final description without interruption)

ReadJSONFile:
At this moment I don't really see how ReadJSONFile(arbitrary/path/file) + ReadSimulationDataJSONFile("subdirectory/file") is preferable over only one ReadJSONFile("simulation/data/subdirectory/file") if both variants called from the simulation prevent reads to non simulation data directories, while the latter means having less functions, less function name variability and less filepath variability making it easier to read (and thus more usable if we take that definition of the word). There might be some catch revealed after the implementation phase, but I cannot get there without having this diff pending.

l10n.js:
If adding one line of code that encodes the variable name of the engine object is 'shit' because it breaks the globalscripts context-agnosticism paradigm, then having a lot of translate calls that completely break the simulation paradigm is by an order of magnitude shittier.
common GUI code that is invalid in other contexts should be in common/gui/. Noone said that every page needs its own globals caching strings. The cache can be in the gui/common/l10n.js file. If someone states that common/ should not contain any state, then someone should even more want to avoid GUI states in globalscripts/. But not really a reason to avoid common globals in common/, because both globalscripts/ and gui/common/ is reloaded when opening or switching a GUI page and we already use common GUI globals to avoid repetition (that implies possibility of inconsistency bugs).

Thanks for your comments and replies, even if I don't agree with all of them.

elexis added inline comments.Nov 26 2017, 2:09 PM
binaries/data/mods/public/globalscripts/Templates.js
12 ↗(On Diff #4352)

(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.
If it isn't bug but a feature, then there should be a reason that applies to globalscripts but not ScriptInterface explaining why it isn't equally available to globalscripts)

elexis added inline comments.Nov 26 2017, 2:16 PM
binaries/data/mods/public/gui/common/functions_utility.js
48 ↗(On Diff #4378)

unneeded loop

This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Nov 26 2017, 2:31 PM

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.

In D1062#42638, @elexis wrote:

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.

In D1062#42638, @elexis wrote:

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.

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 ↗(On Diff #4352)

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?
Sure the GUI can use the other function everywhere, but exposing the same wrapper function (shocking that such things as script wrappers exist) to both and using that in shared code seems a lot nicer than having strange constructs just to avoid having something that is simpler than strange restrictions in multiple places that just end up confusing people. "Why can't I open "foo/bar" when I can with the same call in the other place???????!??!"

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.

In D1062#42930, @elexis wrote:

rmgen/ needs to be able to read from simulation/ and maps/ but should for the same reason not be able to read from gui/.

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.

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.

(I totally got this, hold on)