The SVN change is wrong. If SVN is broken, the differential pipeline should try to build upon a working version, so you must not run svn up. If you want to change the paradigm and always build upon the latest SVN, the pipelines should be entirely rewritten.

Very nice improvement, some major comments though ??

Thanks a lot for the quick review!

Address Freagarach's comments.

The code change is correct and, apparently, works under SM45 and SM52.

Yo Stan, I don't see the need for MesonBuild support, I would recommend creating a Trac ticket with "If Time Permits" urgency, a link to this diff in the Patch field, and then abandon this diff for the time being.

Hi @wraitii! I will need this to perform the SM 52->60 migration (see #1389510 on Bugzilla). The ideal route would be to include this under SM52, but if SM60 adds nice code to handle proxy privates, I am fine with doing this as part of the 52->60 code fixes.

Hi there! I rebased https://github.com/na-Itms/0ad/tree/spidermonkey upon this. Please test it, along SM52 ? I'll update the forum post whenever I have the chance.

I disagree with the change for the same reasons as Angen and Freagarach. IMO the only meaningful change we should make is: as part of D11, move the files we use to a new relevant place, and delete the others (moving them to the art repo). For instance the campaign test map should become an actual test campaign within the new campaign framework.

It does look like threading Atlas adds unwanted complexity. Atlas still freezes when the simulation hangs (for instance when changing the terrain with the pathfinder upgrade, we had to figure out a way to improve performance so that Atlas wouldn't lag). So it doesn't seem like the threading is useful. If it adds bugs on top, removing it sounds sensible to me.

Rebase

Debug tests do work, I closed #3753. The current diff is now running on Jenkins, I still have some planned work for posting build warnings as comments.

@wraitii, @vladislavbelov, what do you think about the spaces-around-operators issue? If you agree with me, I will move forward with D2490 (see some discussion there) and try to find a way to fix this one issue with clang-format. If you both think spaces everywhere are better, I can activate this instead.

Just like the others, it would be nice to have that functionality in-engine (mod validation could even happen as part of the archivebuilder workflow!)

The code is good and it fixes a TODO. I also tested it in game and the snapping along the sides now work ??

Ha sorry! I really have to test it, then ?

In D2726#115102, @Stan wrote:

I will post the discussion with @janwas after cleaning it a bit.

Nice addition. I only reviewed the math/code, didn't test yet.

In rP23917#43905, @irishninja wrote:

In this case, I think it is preferable to make the setup as easy as possible and installing the mods automatically when spinning up the RL interface seems like it could remove a step in the process.

In rP23917#43903, @irishninja wrote:

I believe there was also a discussion around the loading of mods in the RL interface. This could be related to my limited experience with mods but I figured that if there are mods that edit units or create new maps, these should also be available when running 0 AD as an RL environment. It looks like there are some mods that add/modify civilizations which would certainly be relevant when using 0 AD as an RL environment. Most GUI mods would likely not be important.

In D2490#128449, @Stan wrote:

I think @vladislavbelov might have better input.

I think so too. As Vlad told me yesterday, the style linter will not manage to detect enforce everything in our coding conventions (for instance proper naming of variables).

For my part if it does the trick why not. Why do we need spaces around binary operators?

You misunderstood. Sometimes we need to NOT have spaces. For instance, for calculating a 2D distance, x*x + y*y is more readable than x * x + y * y (the relevant binary operator is * in this case). However, clang-format forces the use of spaces, and there is no option to change that, as of today. The detection of missing spaces is optional in astyle.

Proposal to use Artistic Style instead of clang-format.

Here is a working version using Coala (not enabled in .coafile here).

@irishninja, could you take a look at the issues Vlad mentions? We should have detected them before, sorry ? You can read the IRC logs for a few more details.

Slight fixes.
Post build warnings as Phabricator comments, so that they are not missed in case of a successful build.
Post lint as inline comments.

I am testing a few Jenkins improvements on this diff, sorry for the noise!

I am testing a few Jenkins improvements on this diff, sorry for the noise!

Adapt LicenseYearBear to the current version of the python package svn.

Can I commandeer this? ?

I am restarting the build here to pass the fixed cppcheck on this. Sorry in advance if there is a false negative..

Obviously.

Fix on Python 3.8 (sent upstream https://github.com/coala/coala-bears/pull/2976)
Fix caching in a docker container (not sent yet)
Display file name of cppcheck results

Hi Stefan, thanks for this patch. Indeed, we are merely providing a bundled version of premake, and we try to keep it as close as possible from upstream.

Address coala warnings.

Correct dockerfile.

Move files in a way that arcanist understands.

Wouldn't it be better to include it in RLInterface.cpp? It is when compiling that file that the inclusion is missing. I am a bit confused by the issue, just as I was confused during the review ?

That's interesting, but IMO, if this is a C++17 feature, we shouldn't use it (yet) regardless of what the MSVC documentation says. It's not always trustworthy. Of course, if the bug is present in current Visual Studio, it is an issue. But I am totally fine with the current code after rP22617 and rP22636 ?

In D2788#126885, @wraitii wrote:

I really have no idea why we get this failure. This is supposed to be defined in Appkit.h, which I do include -_- .

@Itms would you have an idea maybe? We might need to link against appkit ?

Yes, the more I look at it the more I'm convinced this is the issue. I really can't do the rebase today but should be possible to see whether the crash is fixed in the upcoming days. ?

By the way, a better name for the diff and for a future commit would be Fix rooting mistake in GuiPoll{New,Historic}Messages. But it would be great to add a fixes #5655, of course ? I'll rebase the branch upon this commit so that we can test if the crash is fixed.

Ha, this makes far more sense, thank you! ?

The root of the issue might be around this part of the code, but I'm not convinced with the patch.

In D2911#126651, @wraitii wrote:

At the moment I don't think we plan on stopping citizen soldiers from being soldiers, so that's a likely no for now.

Update with the version from the SM52 git migration branch.

Thanks again for the great work ?

Fix the map!

I do not know the current unit motion as well as wraitii and Freagarach do, so I'm trusting Freagarach's review here ? The C++ changes do not seem problematic at first sight.

Very fun tool to play with, great job! And the code looks pretty nice, it's self-contained and easy to read.

Use CanBarter rather than HasMarket.
Test cmpPlayer and add a unit test for cmpPlayer.SetPlayerID while I'm at it.

Fixed Jenkins lint.

Hey! I'm the one at fault here.

I believe that you can also delete the SetSerializablePrototypes and IsSerializablePrototypefrom the (de)serializer, as you don't replace them by anything. They were only called by this AI code.

In D2746#124989, @wraitii wrote:

Currently, if one stores a Vector2D in a component, it generally will OOS when deserializing since it is deserialised as a dict.

Hi wraitii, while I was looking into those serializable prototypes the other day, I discovered that most of this code is currently dead. I do not know if you noticed, I didn't see this mentioned above.

Thanks for the help asterix but this is not reviewable, I just uploaded it to be able to show it to other devs. In his branch, Bellaz changed the key type slightly, which could be a possible fix but does not match upstream. I am going to try a different way based on my discussion with SM devs.

Sorry, yes you can abandon this one, I'll include it as planned when I'm looking at NVTT again.

In D665#123681, @wraitii wrote:

I'm planning to commit this sometimes in July. Testing & reviewing obviously appreciated until then.

This looks really nice at first sight. I'm keeping forward on the SM upgrade work and I'll let you know if I prefer get this in before or after a specific version.

As reported on Trac, the patch works for me, I have no news of GunChleoc. If someone with an Ubuntu, or derived, could test this... I am also interested in users of various distributions where the system nvtt creates the issue.