I've added some +. to comments but only where you already changed the line, it would be the only extra change and the word you changed was at the end of the sentence.

• Better visibility of the option.
• New dropdown and setting instead of reusing the normal one.
• Show proper number at replays.
• Show proper number when winning as sole player left.

(Sorry, totally missed your comment...)

The player is probably not alive anymore, that leaves only GAIA. AlivePlayers -1 == 0.

Ah, when a player has won. That reproduces it for me as well :)

Thanks for testing and the feedback @Nescio!

In D2426#101023, @Nescio wrote:

I tried one game with three AI players and a world population cap of 300. When one player was defeated, the population cap went up as expected (100→150), but when another was beaten, the surviving player was displayed as having a maximum population of “Infinity”, which is odd, because that didn't happen without this patch (i.e. always at 300).

Strange, this does not happen with me? Where did it show that? Is that on SVN or A23b?

Use action determining functionality from input.js.

Thanks @Angen I forgot to raise a concern here ;)

Changed comment.

D2343 Would make the enabledObserver thing unnecessary.

It would be good to replace the controlsPlayer with a more easily extendable check, as suggested also in the ToDo in unit_commands.js L158.

Moved the observer-focus-rally-check.

• Works like a charm.
• Code looks good.
  • No deduplication possible.
  • No extension possible/needed.
• Nice addition.

In rP21121#29216, @mimo wrote:

When a field has not 100% health, we can no more see the number of gatherers, even when no builders because the test on numBuilders > 0 has been removed from line 247.

It seems that this was the concern? That has been fixed (number of gatherers show even though field is not 100% health).

• Move most of the function to unit_actions.js.

Multiplication concern is fixed.

Include && msg.data.added.length.

I am glad we came to the same conclusion, albeit you expressed it clearly, and I had trouble transferring it from my mind to here ;)

https://wildfiregames.com/forum/index.php?/topic/27214-borg-expansion-pack-mod-implementation-in-0ad-alpha-24-release/&do=findComment&comment=388029
We might check for this.isIdle in the LosRangeUpdate but I'm not sure that would not introduce other side effects. And setting up a MWE for this specific case is not too easy ;)

• Units: "trained", buildings: "build".
• Ternary return.
• Extended test.

To js.

• Less redundant checking.
• Ternary for observer check.
• Back to checking for undefined explicitly.

Exteded.

Mod version.

Include "focus-rally".

Change 12 hour Last-Man-Standing TimeNotification timeout to Infinity.

Because it feels like infinity or is treated code-wise as infinity?

Patch is compleat and useful.

Combine return statement.

• Allow healers to be functional on a chariot.
• Some inlines.

• Garrisonable class.
• Simple test.

Garrisonable class.

• Split angular restriction (D2402).
• Add turret restoration function (GUI still needed).

(There is also slaughter attack but one could ingore that.)

Since the timer ought to be checked as well when an entity changes ownership (out of scope here), it seems beneficial to already have the checking code in a seperate function.
And while at it, the start timer and cancel timer can be merged in that, unless there is a strong reason to keep them seperate?

It is necessary to mention Melee and Ranged here because Capture is not affected.

I did some more testing, it might be that the problem seen here is not specific to your patch but just a consequence of what is fixed in D2358.

Comment.

Remove redundant variable.

Looks like you forgot to revert something?

JSDOC.

I don't understand what parameter is not necessary?
If we would react the same way in APPROACHING and/or CHASING, we would indeed be leaking information through the FOW, although I do understand your point.

Use existing canControlPlayer-function.

Works like a charm :) And is quite useful I would say.
One could add a test?

Yeah, one thing at a time xD I was still figuring how that could be achieved.

active - enabled.

• GUI-only.
• Less duplication.

In D2396#99854, @elexis wrote:

I wonder if this isn't a feature exclusive to the GUI. The GUI has enough information to decide what unit action the player would like to order and whether there are idlers around. So the GUI can construct the simulation command itself already without adding a new command type, no?

I was wondering that as well :) And already started trying to make it GUI-only ;)

Hotkey.

Concern fixed by rP23098 :D

Also, there is kart instead of cart in one occasion.

I have some trouble designing the angular restriction on the attack.
Cases to work with:

1. Turret on UnitAI garrisonHolder: turret fully turnable, so when in range it can attack, no matter what the angle of the garrisonHolder is. (E.g. archer on a chariot.)
2. Turret on UnitAI garrisonHolder: turret has a max attack angle, so when in range, it has to ask the garrisonHolder to turn before it can attack. (That means splitting the CanAttack from a CanAttackRightNow or something.) (E.g. horse archer.)
3. Turret on non-UnitAI garrisonHolder: turret fully turnable, follow case 1. (E.g. unit atop a wall.)
4. Turret on non-UnitAI garrisonHolder: turret has a max attack angle so when in range but not in angle range it cannot attack and should pick another target. (E.g. turnable cannon on socket.)

canControlAll does not function as intended, see respective inline.