@Nescio, @Stan: Do we want to rename actors as well? (I guess Icons not due to them being art?)

General comments about style (see also @wiki CC):

• We use let as much as possible as opposed to var to limit the scope of a variable.
• Comments start with a capital and end in a dot.
• We like to keep the templates in alphabetical order (there is a script for that in source/tools/templatessorter).

That I cannot place the theatre to the south-east or north-west facings of the wonder (small sides) because it snaps and I guess the obstructions overlap.

• Small refactor of game loop when nonvisual

I removed the mod installation from RunRLServer then realized it was pretty easy to decrease the code duplication from that method (as @vladislavbelov suggested over irc, iirc). Removing the code duplication resulted in keeping the mod installation logic. I am happy to revert if that is preferred; let me know what you think!

I just took another pass at it and removed the duplicate logic from RunRLServer. (I believe @vladislavbelov mentioned this over IRC.) I still need to test it when running the GUI but thought I would push it anyway to get feedback. As this reuses more code, it does still include mod installation but I am open to changing it, if preferred!

• Merge RunRLServer logic into main loop

Yeah, that makes sense. It is probably also unnecessary for the containerized case since you can just mount the files into the container and the environment can still be spun up with a single command.

In D2945#128436, @Freagarach wrote:

I do have trouble with e.g. the theatre, but that was present before.

• Educate PetraAI.

In D2760#118111, @Nescio wrote:

Any opinions on the number of visible garrison slots on gates? In principle they could accomodate more units.

Personally I would say: "pack 'm full" ;)

So maybe a population bonus with the CC could be justified but that's quite a huge bonus at the start.

If Resistance is obligatory one can save the attackers and call targeted functions.

I'm not sure there are any balancing considerations at this point since stone walls are severely under used at the moment. Looks like a good improvement though.

• Rebased.

Also, instead of using UnitAI, we could perhaps query TurretHolder for the abilities of its subunits?

• Rebased.
• Remove extra space.
• Update test_Attacking.js.

Do the symmetric ones fill up one tower first then the other? Or are entities added evenly? If the former, perhaps do the latter. Also perhaps you can use more descriptive names for the turret points of the symmetric ones (e.g. Tower1Pos1 or something).

• Make sure value stays positive.
• Square only once.

No problem! It is just that I think a review of this kind of diffs need more of a code review than "I tested, it works -> accept." ;)

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

I neither fully understand the math, nor the code. I was merely answering to the call for testing ;)

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.

• Exclude INVALID_PLAYER when adding a gatherer.
• Use for-of-loop.
• Don't check for ownership in UnitAI FindNearbyResource.

Isn't this basically what is performed on OnDestroy in UnitAI?

• Fix typo.
• Don't call GetOwner() twice.

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.

Makes sense. I admit that I had been thinking that there was no need to change most of the initial startup for the game when running the RL interface so I just left it in.

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.

I have addressed the concerns in D2947. Let me know what you think!

I think @vladislavbelov might have better input.

• Add more const's

Rename function.

Address some of Stan's feedback

• Add extra const's
• Use reference in for loop

I do have trouble with e.g. the theatre, but that was present before.

Proposal to use Artistic Style instead of clang-format.

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

@Itms, Yeah, I will check those issues out and make a new revision!

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

I havent see screnshots but this is my guess

No, I mean the player color band below the unit name.

After making screenshots both without and with the patch, I now see what you meant. It's not intentional, I'm a bit at a loss though. The player colour band is defined in `single_details_area.xml line 116, but not its size, which is apparently determined indirectly. Increasing the vertical space of its parent (line 114), grandparent (line 99) or primary ancestor (line 4) affects the height of the band, but also the vertical position of the player name and underlying civ emblem, which shouldn't be changed.

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.

Successful build - Chance fights ever on the side of the prudent.

Successful build - Chance fights ever on the side of the prudent.

Successful build - Chance fights ever on the side of the prudent.

Grmbl, I added a dot again after the diff. Can you close this one also please, @Nescio?

Successful build - Chance fights ever on the side of the prudent.

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

Successful build - Chance fights ever on the side of the prudent.

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

Successful build - Chance fights ever on the side of the prudent.

Build was aborted.

I'll commit when green.

Well it's not a priority but it a nice feature for testing audio :)

In D2723#128335, @snokis wrote:

In D2723#126380, @Stan wrote:

Hey, any news?

Yep, soon.. Sorry, started working again, lost interest and then went off-grid for a few weeks :) Is this something you want for the next release?

In D2723#126380, @Stan wrote:

Hey, any news?

Fixes my issue on Intel with GLSL on
Before:

Successful build - Chance fights ever on the side of the prudent.

fix tests

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

You mean the top panel? I didn't touch that.

No, I mean the player color band below the unit name.

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

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

gui

did you reduce the band with the civ emblem ?

You mean the top panel? I didn't touch that.

The minimap buttons no longer fit the circle (as in they have a bigger gap)

See the MiniMap.xml file. What I did is assign the minimap buttons each a width and height of 50% of the minimap panel's. It turns out they were somewhat larger before, hence the gaps.

the bottom bells are now bigger than previously ?

Initially this patch only shrunk the icons in the right selection panel, from 37×37 to 32×32 (with borders from 43×43 to 38×38). Then came the request to give the icons in the left selection panel the same size, which I did. So yes, the stance, formation, and alert icons are now slighlty larger, from 30×30 to 32×32 (with borders from 36×36 to 38×38), i.e. +1 pixel on each side.
I didn't change the size of the command icons in the bottom row of the middle panel. They're 32×32, but unlike the icons elsewhere, their border is inside (on top of) the icon, rather than around. I experimented with standardizing those too, but failed to find a satisfying solution, since their size with borders (38×38) means the middle panel is not wide enough to support six of them. Therefore I kept their sizes unchanged, but vertically aligned their centres with those on the bottom row of the left and right selection panels.

increase the middle panel one as well?

The limiting factor is everything should fit inside 1024×768. Increasing the size of a panel therefore means reducing the size of others.