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

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.

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.

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

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

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.

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.

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/

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.

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

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.

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.

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.

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.

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.

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

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.