The part about not indicating something that I don't.

Ah, it most likely fails because it cannot nicely convert the enum class into an int (which is the whole point of using an enum class), while not having a hash specialization for the enum class. Seems very much like something that's too complicated. Use a map.

Meh, use a normal map if that works. Not worth worrying to much about this (like it seems last) issue.

In D1029#59924, @Itms wrote:

• I got rid of all the helper methods of that struct. It indeed removes a significant number of lines. However the tuple version makes it difficult to name elements (std::get<0> is quite ugly) and even first, second in the case of a pair is less clear than status and progress, so I didn't go all the way.

I suppose the naming helps with that yes. But the main issue was (and to some extent maybe still is) too much code for what is actually required.

I suppose if you fix those, and think a lot about the struct thing this is close enough to a state I'm not going to disagree with too much...

The below should be most of the things that are still not fixed, there might be more, but there are more interesting things to do than complain about things that were broken relatively recently...

In D1029#59608, @Itms wrote:

Another iteration. Includes @s0600204's change to the download size units, and changes from me after your comments. I also made a change to the error propagation in parsing. It uses the production instance.

One of the recent gui changes seems a step on the road of using stale data if things go wrong.

I take it some of the last batch of comments are yet to be done.

Just to provide some sense of scale, the original patch managed to fit download+verify into 137 lines. This patch here needs 101 lines for most of the status struct.

And while I might not agree with a bunch of commits to the mod selector that happened recently (the count of which has increased recently), I'm still somewhat bound to complain about certain things, even though I know some of you would prefer if I wouldn't.

Clarify some things, since people seem to get tons of things wrong anyway.

Some cleanup, slightly nicer gui, connection warning message box.

Use similar external_libs block for libsodium as for other libs.

Move rationale to the header, and extend it a bit.

Open file as binary to make downloads actually work on win32.

Small cleanup, correct handling of malformed trusted comments.

Verify the global sig, add some rationale, some cleanup.

With tests. One of which exposes breakage in the conversion code.

Support for multiple signatures, some cleanup, temp file name to handle program termination during the download nicely.

Signing.

Untested signature checking implementation (because downloads from upstream are
currently broken).

Update for upstream API changes.

Upstream does not require secrets for downloads anymore.
Some cleanup.

Use a custom write function to simplify hash computation.
Now check the hash. Currently not checking the length (which should be done as long as there's only md5...).

Now with working downloads.

In D1029#48094, @elexis wrote:

Binding to a commercial website from a free & open source gives me a bit stomach ache.

It's more of an API and depending on some development some parts might be open source so one could host it. That said this is both WIP and a PoC.

In case someone didn't notice that this is based on top of my working branch which will not move forward past r20551, then consider yourself informed now.

Small updates for API changes.

I guess the RL test isn't going to happen, and me caring to update this in case someone actually gets around to check this also isn't.

Feel free to commit this in case you think it helps with not invoking undefined behaviour.

Feel free to take over, I guess here you're unlikely to break things you don't care to understand.

Still broken. Well I guess because at some point I stopped reading commits that fixed one thing and broke at least two others.

Thanks for giving the user an easy way to enable and disable mods!

Thanks for cleaning this up, breaking parts of it, not caring about invalidating possibly not obvious parts of the design instead of figuring out or even just asking to clarify that, then being smug about having done so. Have fun mainta^W breaking this more.

I could have written that yesterday already if we would focus our energy on the partial agreement instead of the partial disagreement.

Given that you went ahead and decided to commit something broken I'll take the liberty of not doing the work of fixing your breakage. Have fun.

Does this actually differ from D1081?

See comments elsewhere.

Does not really seem any cleaner.

In D1062#42952, @wraitii wrote:

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.

Works without pch.

In D422#42077, @Itms wrote:

Tested and works [...]

I assume you built passed the parameter for building the shared glooxwrapper?

Now with UNUSED().

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.

Then factor out those common things into two XML files, or possibly even one and unify the capitalization of those. Oh, wait, parts of that have already been done, maybe just do the others instead of trying to do something because something must be done and this is something.

I'd still prefer getting rid of the individual non-{civ} and non-generic techs, might need a little cleanup in a few places, or introduction of replaces: "phase_x", though probably with an array. (Though I guess not introducing the latter is a lot cleaner.

This also removes the "preferential" treatment of enemies over gaia. Not that I guess any of that has been considered since fixing a single map seems important.

Why can't page_foo_bar.xml not be for foo/bar/? Not that there is any need to have all of that in a single folder. Folders make things easier to compartmentalize, use them.

Doesn't fix the one invalid comment before the actual replacement.

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.

Something something reviews.

No, what was done previously just had no sense. If we want to only allow to build structures from the Identity civ, then the unit template should just not have {civ} but the civ we want: It would be useless to have that {civ} replacement here,. The behaviour of the patch (replaced by the civ player, as is done in the ProductionQueue for units) is what has always been expected but was buggy.

Ah, I guess I was somewhat confused by the comment that wasn't fixed in rP16506 that still talks about the entity. Same for the schema here and in the production queue.

In rP20521#25233, @elexis wrote:

(See differential and or ticket)

Those just state "Yeah, that shouldn't be done and now we'll enshrine that after not checking any previous discussion after less than a day of discussion.".

This now prevents C&C style capturing (which allows one to build everything from the other faction). It also somewhat breaks that one cheat.

That seems limiting. Imagine a case where a ship can and does garrison siege engines. Assuming sane people they would try to get to land, but nobody would care about dying just to save some heavy equipment.

The logic is, if a unit can ungarrison when we click the ungarrison button then they should also ungarrison when the ship dies, the two behaviors should be exactly the same.

Depends if we want to reward players for being lazy. Also it seems that the consensus was that we don't want to do that in this case at some point.

Also what @bb said.

In D8#42149, @wraitii wrote:

Rename ITERATOR_GATE to ITERATOR_FENCE as that's the right name for it.

Sentinel might be the word you are looking for.

For example, healers and cavalry can garrison in a bireme, but when it sinks they'll die rather than try to ungarrison:

<GarrisonHolder>
  <EjectClassesOnDestroy datatype="tokens">FemaleCitizen Infantry Dog</EjectClassesOnDestroy>
  <List datatype="tokens">Support Infantry Cavalry Dog</List>

I feel the lists should be the same (which means we can get rid of EjectClassesOnDestroy).

That seems limiting. Imagine a case where a ship can and does garrison siege engines. Assuming sane people they would try to get to land, but nobody would care about dying just to save some heavy equipment.

In rP20369#24249, @elexis wrote:

In rP20369#24246, @bb wrote:

programming.json?

(Wasn't sure if it can be considered a sufficient programming effort.)

Probably not, still it doesn't really hurt to include it.

Ah, hadn't noticed that one.

I'll keep the current thing in my local tree, in case someone cares enough about this to do the work take over.

Now with the correct commit.

Actually fail. Not really more tested than the previous iteration, though I have had this iteration in my local copy for a while.

Remove now useless include. Also include network code changes.