See also IRC today -> some changes for i18n needed.

Add the proper context, do the cleanup.

Think this looks good now, see inline and I'll commit this.

There's a few bits of debug code I left which I'll clean up later.

When trade is 0, this means it's impossible to set up trade between the two markets, without notification, which seems to be compatible with what existed before.

Complete and good, thanks for the patch.

Thanks for looking at this, indeed that's broken.

TBH I think you've over-engineered it. What's 'Order', what's 'Main' ?
I think you don't need to make something so generic. Just have 'deletable' and 'orderable' for now and handle everything from there.

In rP22982#38295, @Nescio wrote:

Out of curiosity, why did you decide to keep the final s? https://code.wildfiregames.com/D2234?id=9536 moved the templates to palisade_*, without s.

In D2281#97075, @Stan wrote:

Looks crowded. Shouldn't there be an icon in the unit icon ? One could then hover over it to get information.

Part of it is that the attack tooltip wraps, so we'd need to extend the width further.
Part of it is the text description, which probably shouldn't appear in this hover tooltip.
Part of it is that we need to rethink information entirely.

In rP22984#38288, @Freagarach wrote:

list champions before heroes in the production queues of fortresses
Where is this?

Better comments. See also discussion on IRC today (2019-09-23) about the order of entities and the 'break'.

Invert the concat order so that enemies come before Gaia

Slight improvement to EntitiesNearPoint

Works, all civs have correct actors and templates, fortresses correctly updated. I've simply moved the icon next to the fortress for keeping city-phase buildings together.

Move the workshop icon next to the fortress icons as those are both city phase buildings and I think this makes more sense.

Accepting the change, I think the template rename is a good move.
I'll commit this with some proper svn mv magic.

Ran a quick AI 2v1 with some tributing and looked at all other loops in the code, this looks to have been the only miss.

-_- Forgot to write 'Patch By: Freagarach', sorry.

This seems like a good move.

Rebased before committing

Remove debug code

But it is not necessarily a bonus, it can be a debonus as well?

One more (y) >> { y; } change. I'm keeping the one that calls ForEach because it's not really more readable and the linter is fine with it.

Mh, then I suggest keeping ElevationRangeBonus for now and it could be renamed HeightOffsetRangeBonus when/if that gets implemented.

Elexis comments, and hopefully jenkins now works since the latest autobuild contains prereqs changes

Jenkins was correct, the last revision uploaded was correct, the committed one didnt commit all files it seems.

This one should run

Indeed, that appears to be what I've done. I generally download the latest patch from Phabricator when committing, but I must have messed sphere, perhaps because my last downloaded file was already this diff but in an earlier version.

Erh, I did that change and tested it locally... I went wrong somewhere but not sure how.

Did the changes I suggested above because I was going to change the XML schema to handle <Interval> better, but then I discovered D2315.

Works fine and I think this is nice enough right now, making the game more moddable with little added complexity. The question of extending JSON files will have to be tackled some day but so far I don't have a good answer.

I thought that the engine did not care about rotation?

It doesn't really right now, since anything can turn instantly. So adding that wouldn't make a ton of sense. But we'll need it if we want realistic-ish tanks or battleships (or horse riders) down the line.

Stan's comments

Move files into a folder, final cleanups.

TBH I find 'AttackHeightOffset' much less understandable than 'ElevationRangeBonus'.

TBH this sounds like a relatively good candidate for CRTP.

Thanks for the look everyone, I will take this as confirmation that despite the relative bug-possibilities (tbh this seems safe enough to me), there is some good from digging deeper in this project.

In D2295#96153, @elexis wrote:

Sorry that I was too lazy to review the original patch, one needs to know the circumstances of the hotkey code which takes some time to get back into again.

TBH that's fine. Your time, like mine, is limited. You'll never have the time to really look at everything in advance, but if you look at what gets committed that optimises your time and the code still gets to where it needs to be.
Indeed this patch was fairly tricky, I too had to dive deep again before committing because I'd forgotten why it worked.

In D2303#96116, @elexis wrote:

But is the patch correct and complete too? I remember seeing Autogarrison, refs D885, D1146, D1228, D1403

Change is positive and this works on my machine -> Accept.

Pretty weird bug haha.

Add missing autorequests.