Some of these only used once, so can be inlined, but probably not for consistency

But they should all be named fooRadius

Perhaps move some element(s) from this line to previous for equal line length

inline placer and painter

newline in end

Perhaps place this in random.js. under GetRandomAmounts(array, count)

AFAIK OnOwnershipChange function are always placed on the bottom of the file, just above the trigger definitions

the first part works, but second is bugged (better not done now)

after ungarisson, the ship should patrol again

add newline in end

No to inlining that, to keep it consistent

Don't see the point in adding duplicate code if it saves 2 or 3 seconds when loading the map. We should rather fix the TriggerHelper to skip the footprint thing, which will be needed by many maps, see above.

Oh, the commenting wasn't reverted apparently

Yep, for consistency, easier editing, also JIT smart to auto inline.

lel at Radus

k

The idea is to have them grouped logically, so doing something for codestyle would make this signifcantly harder to understand

thx

aye

Wanted to keep it easy to replace with that one github branch commit, but idc

Variables not used once

neither

=> neither

yep

Your intention was the indentation

Exists, otherwise phab would complain

Originally did for heroes, but removed. If we do that, we should do it with all templates then. Then we still need to define all the classes, which might indeed be better for mods and future changes.

A missing unused argument that would be nice to use, but needs gameplay testing, consideration and might even depend on a gamesetup option

good catch

IIRC polar sea too

Since this should be used by survival too, could be done in a separate diff, especially since people don't like new maps changing existing library code

Those templates do, meh, fine. Stupid mod support

120 characters is my tolerance limit, this one has 121, so ok. In particular consistent with below
Yep, as said above above

If they don't have a visual component, what are they doing on the map?
It would crash so modders can fix their template.
Don't see the point, but I'll just add it because I don't have time to go through another revision for no real reason. Adding the cmpIdentit check too for consistency.
put, put, put; split, split split; build built built.

oyes. Since we're likely running into naming conflicts, DanubiusOwnershipChange?

check, nice

meh, might also want to add a check whether the component exists :P

Nah, the siegeRatio is the most important part in the balancing consideration IMO

technically yes, but would look odd with that comment and without a newline

must be a relict, good catch.

No complaint about duplicate garrisonholder getter?

Must be a performance concern. Should use a new function then, refs D185

prior unload command that should really not be overwritten?

Right! Someone could survive on an island or the water and rebuild later.

But better not cancelling the timer in the first place

s/cleaner/actually functional

Hm, removing that comment which I had intended to workaround once apparently.

Adding a AttackAndPatrol call, but it will mean that we will queue duplicate AttackAndPatrol orders once the timeout kicks in again. Not a big issue since the ungarrison walkcommand thing will reset the order queue more frequently than queueing new orders, so we will never have more than twice those patrol+stance things queued.

Adding a check for the unitAI order queue sounds bad and ideally we should avoid making assumptions about the sim as far as possible to reduce maintenance cost as far as possible

In accordance with every other trigger script file I put my hands on. To keep it notably different from the function scopes.

:-S. added

-.- who said we have to differ this each time wave instead of each match? but yes, those globals should not be functions then

exists

wrong rename, should be tDist, since it are trees.

at the first wave (when t == 0) then there won't be any garrison, perhaps add a constant.

shouldn't they also try to attack docks and trade and fishing ships?

we don't garrison them here anymore

could simply be OnOwnershipChange as is in sotf.

my arc is yelling about it though...

I suggest:

setWaterColor(0.317, 0.396, 0.294);
setWaterTint(0.439, 0.403, 0.262);