Trigger-based test map, showing rather poor behaviour all around. The patch fixes the particular issue it was made to fix, but not the fact that ships move around the map too much.
I noticed that ship behaviour is bad because they don't adjust when the unit they want to pick up moves, which is because they are following a "known bad path", so they only update the path when it ends (this is WAD but the design x).

This plays into two concepts:

• Controllability (see D1960)
• actually being able to move.

Investigating further:

• it seems one could patrol to a target by changing the GUI. Unlikely to have worked before, fixed by this patch.
• Guarding has some seriously custom code in Transform.js to maintain the list of guards. It seems we could clean it up a bit.
• As noted by bb, fleeing was broken before. This fixes it but plays the "panic" sound again, so I'll add a custom handler.
• Garrison approaching will request a new pickup, which seems fair. Garrisoned early-exits.
• Chasing & repairing have been noted and still work correctly.

Add tests & actually mage visible garrisoned entities have no obstruction at all because it still broke and that makes more sense than hacking around flags.

For the record, given the hack I'm now introducing in ProductionQueue.js, I won't commit this until I either:

• Get some other dev to look at this and agree that it's painful but we should go along with it
• Refactor so I can remove the hack.

Thanks for the look both of you, this was one of these things that had been annoying me for a while :)

I independently rediscovered this on D2768. ReportError seems rather useless (particularly since JSNative functions can return false to indicate failure), and it is removed in later SM versions.

In rP22296#42528, @bb wrote:

Seems like the pmp file is missing, map doesn't load here

In rP23710#42484, @Angen wrote:

Start game with iberians.
Update long wall to gate.
Move unit through gate.
It will never ever close back.

Are there any unintended consequences?

Welp, this broke rP23710

@Itms It seems we can now set up 're-openable diffs' which would be rather convenient here, see https://stackoverflow.com/questions/13633031/how-to-reopen-differential-review-in-phabricator

In rP23710#42505, @Nescio wrote:

Tested by: Nescio

For the record, I don't believe I actually tested D1418; that would have involved rebuilding the source code. What I did was comment that I liked the concept, and ask for its current status.

I'm going to assume the gcc error is CI weirdness, because it compiles locally and on mac/windows.

I'd agree about sending specific "disabled click" messages, then we can handle notifying the player on clicking a disabled button or something.

Was about to nitpick but the nit was trivial so just did it myself.
Also rebased.

I think it's much better to rename the current "GARRISONED" to "GARRISONING" and add a new, basically empty "GARRISONED" state. You're doing too many things in APPROACHING now, while that's only supposed to be used for... Approaching :p

I think we need D2367 first, but this seems like another step in the right direction.

Needs a rebase & see inlines.

I think this only works because the naming is alphabetical, sadly :/

Other RTS games generally have a few buildings with this feature (I'm thinking of the terracotta army in RoN, but Age I think also has buildings like that).

I fully support this change, including for combat behaviour. Champions can be made arbitrarily slower.
That being said, it is controversial, so I'm merely stating my opinion here.

I believe ShouldAbandonChase already has code to do that, but maybe compare with your implementation.

Does seem worth having for consistency, even if it's kind of a slower version of "replace" anyways.
Note that multiply: 0 and add: X are not the same as replace: X as replace early-exits and so further techs are still applied.
That in itself could be seen as a bug, as our ordering is only "kinda" consistent.

It doesn't seem actually worth caching the search range value, since that value is not used that often and further it's rather fast to calculate anyways. You're basically adding a cache on top of the regular modifiers manager cache, as that value probably won't get outdated too often.

I think you can just hook into OnGlobalEntityRenamed. Will need a little care to not break things.

think you should go ahead and commit this one earlier rather than later @vladislavbelov

The patch's idea is interesting, I think the feature is worth having on some level, there are no obvious mistakes in the implementation that I see from a cursory look.
I'm wondering if the ability to have garrisoning sizes + restricting classes is enough to express most of what we want.

Conceptually, I rather like this. I think we need to commit D1960 first though.
Needs a rebase on top of rP23710 .

Probably requires D2702 and D2763 to even think of working properly.

Adding to my review queue, this seems like a move in the right direction

This doesn't seem extremely worthwhile tbh.

Seems better to do a distance-based filtering than this odd hardcoding tbh. I find Angen's approach in D2440

DoA, but running some static analysis on our JS is probably a good idea. Should look into js analysers & lexers.

@Stan if you want to test something :p

This seems still worth doing in some respect, but I would need to get back into it to really see what's happening.

Change for the sake of change imo. I should do a number of those in the range manager in my own rewrite.

Probably not worth spending time on

Abandoning this under the assumption it was superseded by a bunch of things, then bunch of fixes on these things, and so on.

pinging @Itms on this again, as I do believe we still have the underlying bug :)

I do agree with Stan, I ran into this weirdness when doing the i18n but splitting the strings between attack and defender didn't occur to me :/
In general this looks quite good, I do think my "level" idea is better than stacking but that'll be for v4 :p

This is a good idea, but then again it's my suggestion so that's cheating :P

Fix Stan's remark & also prevent starting any scenario by double clicking on them :p

Had the rather good idea of testing this further, and I noticed that I didn't handle the case where a template being produced was changed.

Had left an ASSERT (instead of an ENSURE) which didn't compile.
This should fix that and gcc warnings.

Fix my own test from yesterday's commit, failed to notice it because GitHub has some lag.

In D665#117303, @causative wrote:

I have had enough. Years of inaction on this obviously necessary bug fix to an extremely annoying part of the game which was created on Jun 20 2017, this now being May 29 2020. Finally a few comments. So I have hope, I spend my time making some requested changes. Then, nothing again.

Fix a few issues, and bump years.

Fix tests & remove template changes that Nescio re-introduces in D2760.
Will commit once CI is reasonably green.

Ayyy, just thought of an issue with this. It completely breaks if the "bird-flight" distance is not the right estimator.

I think the concerns about annoyance with regards to micro are valid.
The formation gif is pretty egregious, but that's the result of formations adjusting improperly when patrolling (check out the behaviour of more units.

In rP23681#42428, @Nescio wrote:

Maybe move these into those new damage files?

In D1418#117084, @Nescio wrote:

Many thanks for D2760, I'll commit it alongside this.

Also, when units on walls no longer interfere with gates, then another thing that will be possible is visible garrison slots on wall towers; do you also want a patch for that?

Have to say this feels pretty good and it works perfectly -> in my test setup from #5106 all 4 units were killed in under 15 seconds, which is as good as I got by speeding arrows up 4 times.

Update generateLongStrings (quite a funny output :p ). Also cleanup a a little more in a bunch of places.

In D270#116956, @Stan wrote:

Are most of your breaking attempts coveres by tests ?

In D1418#116576, @Nescio wrote:

Perhaps it's better if you focus this patch on the necessary mechanics changes.

Uploaded the wrong diff :p

Fixup the using.

Of course, as I say that, I realise in D1781 that I made a small mistake.

Slightly cleaner code with D2768 improvements.

Cleanups:

• used tag dispatch instead of SFINAE for call(), resulting in much more readable code
• possibly fix an issue with the private pointer setting, by casting correctly on both write and read.
• slight cleanup to the template code in general.
• fix the "friend" mess I'd left.
• cleanup GUI_OBJECT macro and define the JS Factory there too, so it's even easier to add new types (again, see updated D1781).

(Actually remove the WIP tag)