Could these arrays be made constexpr if they were C-style array ?

Remove renaming

In D5181#220538, @phosit wrote:

I read the comment above the macro as guide for the usage of the macro. It does mention/promote the use of (posX, posZ).

Mh, I read it as just explaining the algorithm, but I do agree it's un-necessarily confusing.

In D5181#220508, @vladislavbelov wrote:

You mean like #1229?

Yeah that's the ticket for it, but I think we try to hide very small "reverse holes" ? aka very small patches of territory. The ticket is about actual holes.

In D5181#220503, @phosit wrote:

The "function-object" you "pass" to the macro is "invoked" with the current tile (posX, posZ) and the neighboring tile (nx, nz).
It does go over all current-neighbor-pairs.

Could you double-check that? My reading is that it doesn't. The 'initial tile' is (i, j). As I read it, code is never called for this initial tile, we're directly over a neighbouring tile.

In D5181#220496, @phosit wrote:

No, you look only at the neighbors (nx, nz) you could look at the current tile with (posX, posZ).

I don't get what you mean sorry? The FLOODFILL macro only looks at the neighbours of the initial (x,z) tile ?

Fix formatting / warnings / errors

Add tests. This revealed another issue: the FLOODFILL macro only looks at neighbors, so we were missing the original territory tile when counting territory.
I believe this also affected blinking territory, but I'm slightly less sure there - unlikely it would show up in game since it can only affect the center of a group of territory tiles, and unless I'm wrong we don't show borders when the borders are very small?

In D4770#220440, @phosit wrote:

Would it be better to not introduce Engine.StartNetworkSavedGame and use an optional argument to Engine.StartNetworkGame?

I think optional arguments are a bit dodgy in the C++ / JS conversion process (makes it harder to do type checking), using explicit names is probably better.

You probably should fix the linter as well.

In D4770#220283, @phosit wrote:

1. It would make it less self-contained. (If we have the same notion of self-contained)
2. Many things would still have to listen on it. (to call setEnabled)

The GUI elements already listen to a lot of UI settings, but you wouldn't introduce coupling with another UI element, just with the settings. Basically this:

g_GameSettings.map.watch(() => this.render(), ["type"]);
g_GameSettings.savedGame.watch(() => this.render(), ["id"]); // this.gameSettingsController.registerSettingsChangeHandler(this.render.bind(this));

In D5157#220257, @phosit wrote:

This doesn't manipulate / mutate the pointer.

It's doing pointer comparison, both equality and ordering, which I'd classify under 'manipulation'.

This wasn't the Pimpl pattern.

It kind of is though? It's not the complete pattern, but it's pretty much the same.

Not a fan of this. The new syntax is worse, more verbose and it also makes it less obvious that this is actually doing pointer manipulations.

This looks fairly clean and self-contained from a cursory glance - at least nothing too invasive or obviously bad, which I'd call good enough™

Would be good to have Stan's opinion on this, I believe he's the most up-to-date on the Fcollada 'port' that we essentially maintain.

Seems like Premake is shipping with libzip 0.11.2, which is absolutely ancient at this point, which probably explains this issue.

final touchup

In D5013#215340, @phosit wrote:

It feels wrong that a ScriptRequest is sometime it's own realm and sometimes it's an alias for another.

I'm not entirely sure what you mean - we're always in the 'own realm', it's just that sometimes we can be sure we don't need to 'enter' it because we've already entered it.

Can't you use ScriptRequest& for the aliasing?

You mean cast m_FormerRealm ?

Fix debug mode.
The code has an, in retrospect, obvious error: nullptr is a valid value for the FormerRealm when we enter the first realm, so we never exited it.
Instead, use a safe value for the optimised case -> the address of the ScriptRequest object.

In D4646#215272, @Freagarach wrote:

[10:18:09] <elexis1> each line in cmd_line_args.js should be moved to gamesettings/attributes/
[10:22:52] <elexis1> if we can have a fromInitAttributes then a fromCmdLineArgs can be implemented too

In D5033#215217, @phosit wrote:

Stop doing that much in one diff.

Yeah this was unfortunately a bit of a stream-of-consciousness diff as I worked on other stuff. I'll see if I can easily split it, might not go through the effort though tbh.

log-scale can be usefull.

I've given it some thought and I don't think it's a good idea altogether, it's rather harder to analyse precisely.

Slight refactoring for cleaner code, and separate the password length to a common file.

In D4646#215175, @sera wrote:

Wraitii is right when he said it's not easy to do properly. What we have here is a hack used while bootstrapping the project being copy pasted to js. It doesn't fix a bug nor does it enable anything beyond what is already possible nor does it bring us any closer to a proper design.

I'm not entirely sure what you're talking about specifically here, but I want to point out that this diff does not intend to improve the --help situation whatsoever, merely:

• move some C++ engine hardcoding to JS, making it more moddable and more appropriately placed
• Clean up the code because JS is easier to read/write here than C++-using-the-JSAPI

In D4646#215161, @phosit wrote:

This patch does to much: I think we require games to support multiple players (not human players) and I don't want that every mod does parse it themself so the "number of players" should be parsed in the engine. I'm not sure if we also require that games support teams.

Actually, those requirements are all defined in the public mod. IMO it makes no sense to parse this in C++ when nothing in the C++ cares directly.

In D4646#215128, @sera wrote:

Well, pyrogenesis --help should print all valid arguments and descriptions and an invalid argument should terminate pyrogenesis and print a message. I have a hard time imagining this working if parsing happens all over the place. Basically I think there should be a parser where you register the options and then run a parser.parse(argv) once. A mechanisms to register options from mods might be nice but somewhat tricky, maybe a json file that list option name, type and help string might work, but for that the mod needs to be loaded already for it to be visible. So somewhat a chicken and egg problem.
Edit: basically you'd want to be able to throw on a typo-ed option, not just a typo-ed value to an option

In D4646#215126, @sera wrote:

This looks like it will get us further from a usage command(pyrogenesis --help) and args validation.

Clarify a few things - mostly this was breaking the contract that GetScriptInterface returned the entered object.
This isn't actually used anywhere, but it's useful to be able to get the ScriptInterface of the current realm from ScriptRequest, so keep that under a different name.

Vlad's comments

Feel like this should maybe be defaulted to INVALID_PLAYER but it should work.

We probably want to change how this whole 'enable shared vision' thing works at some point but this looks good

This appears to error, per gameboy and myself https://wildfiregames.com/forum/topic/107586-an-error-occurred/#comment-553741 (I bisected it to this commit)

Fix a number of issues, move the comments to JS.
Also rebased on top of rP27692

Less dramatic comment

Fix rebase error that broke everything

Pull changes from D5000

Only the FunctionWrapper.h changes

According to Stan we drop XP support before SM78.

We did

In D4981#214564, @vladislavbelov wrote:

In D4981#214556, @wraitii wrote:

Mh, guideline makes sense. Shame the syntax is pretty bad...
I'm fine with {} but you should add braces on either side per the JS coding conventions IMO, at least one before {

I agree that int x{ 1 } is more readable than int x{1}. But then we have inconsistency with Object x(1).

Mh, guideline makes sense. Shame the syntax is pretty bad...
I'm fine with {} but you should add braces on either side per the JS coding conventions IMO, at least one before {

(assuming this works - haven't tested)

Tests are good, consider inlines.

Good by me, consider inline (we generally use {} syntax for non-POD / non-trivial stuff)

Gonna accept this though I'd still like to hide the lambdas under the assumption that you'll end up cleaning up the map generator at some point. I think incrementally it's moving in the right direction.

Think this is good with the changes from the further diffs too

In D4959#214485, @phosit wrote:

I didn't find a usage since we use SVN

This also looks good, but it'd be nice to look into the blame and find out when the last use-case was removed, to gain confidence that we won't need to add it back later

Inline both.

In D5012#213547, @Stan wrote:

What happens if the value is NaN/null?