I'm wondering if this can't be fixed by using template literals: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals
(though I don't really understand why you need it, if you could provide an example - haven't figured it out really from the diff before either)

In D2934#132886, @vladislavbelov wrote:

Does LTO work correctly for old supported compiler? Maybe it should be disabled by default since it might slow down compilation during development.

Looks correct and probably a good idea to separate the local variable mess that this shader was.

I really fail to see how you want to find a typo in a name automically.

There's automatically and then there's "easily". We could detect close-names (such as capitalized/uncapitalized versions of the same string) quite easily with a script, and likewise typos aren't too complicated. Simply outputting all "known" status effect node-names would make it obvious if there are duplicates, and that's not extremely difficult to do.

In D2808#132510, @Freagarach wrote:

That is also the case for damage types, right?

Just tested -> couldn't reproduce the crash via launching a map or triggering a shrinking GC on the lobby page.

Conceptually seems OK and no issues on my end.

I think the approach of having a template-configurable i18n is good, I'm wondering if it shouldn't be an attribute of the attack:

<Attack>
    <Ranged name="Boiling Oil">
    </Ranged
</Attack>

This would avoid having to think of a name for the tag.

This seems sensible to me.
The fact that Biremes were enabled at town phase dates from rP12240 which introduced tech requirements for warships. It seems to me that biremes have no particularly strong reason to not be in village_phase, given the commit.

Good thing I added a conditional there..

Possibly final cleanup.

My comment above remains valid, my status is "waiting on @Imarok to update the diff" ;)

Make formations walk again.

(The older way was made irrelevant by rP22313 which moved the "Move" call to states instead of orders.)

I think your previous diff had things in the wrong spot, and this one makes it obvious which.
Given the scope of things, I think you can get away with just "approaching" and "garrisoned", but you need to add some more code to "approaching" imo to not make it reliant on unit-motion.

Thanks for the comments @Itms , in that case I'll carry on investigating the FPS issue and move towards committing this :)

@vladislavbelov Do you think the approach is acceptable? Indeed it works, but only by "coincidence" that CSkeletonAnimDef are long-lived. However it does bring a rather tangible improvement.

Updated, thanks for the look and thanks for reminding me I had uploaded this :p

This is 100% nonsense, in every competitive MP game I play, especially the vision tech is researched ALWAYS.

Thanks for taking care of this @Angen :)

For what it's worth, javascript uses UTF-16 strings, so it's probably faster to use CStrW when interacting with JS code (fewer conversions). That being said, performance is not the utmost concern here, so using utf-8 strings is probably neater.

Hm, these reports are odd indeed, I'll investigate.

This seems rather trivially a good change to me.

In rP24013#44179, @Nescio wrote:

Allow training Axeman Cavalry at barracks.

The commit message and title is incorrect and misleading. What it actually does is remove cavalry axemen from Persian barracks.

Seems a solid improvement over the status quo regardless. Thanks for the patch!

In D2948#130307, @Nescio wrote:

Why? Population is clearly a cost.

It is a cost in a semantic sense, but the "Cost" component represents a "resource transaction", which doesn't fit population very well right now. Separating them might actually make sense (and it might not -> haven't investigated enough, but I can see it).

I'll create the ticket before merging this then.

@bb's comments + Rebased.

For what it's worth, there was debate already on D268...

In D2978#130044, @Nescio wrote:

Furthermore, quite a few technologies have "armor" in their name; should those be renamed too?

In D2229#129919, @Freagarach wrote:

I wanted to do that but noticed it needed a rebase and I wanted to let the bots run over it ^^

I think you should go ahead and merge this before rebasing introduces bugs.

I never really understood why Spearmen dealt so much pierce armour, effectively making them ranged units, damage-wise, which was always rather annoying to balance. I'd actually support moving them more towards Hack, if necessary adding some multipliers.

Seems good overall

In D2913#129854, @bb wrote:

Code desperately asking for an animation, needs ticket or so.

I also approve the name change.

In D2837#129718, @Freagarach wrote:

but still, it takes two or three steps to turn around ;)

I mean yeah, but it doesn't really affect my movement speed, as I can turn while moving. Try it, as you say :)
Turning "in place" does take a little time but that's not really relevant here.

I can't find a fault in this version:

• GUI works for controllable/uncontrollable entities of the player, and of other players
• Activating the control-all cheat does allow one to control all uncontrollable entities
• Controllable entities still work, obviously

In D1958#129412, @Freagarach wrote:

It enables the chariot to follow a target such that the subentity can keep attacking it, instead of moving in range and stopping.

I need to test this.
One remark: I think you can delete the FSM logic for duplicating a state.

After a good night's sleep it obviously makes more sense to do this.

Forgot to actually fix the reset() call.

Remove FastMoving from the elephant since it indeed isn't.

Then I really have no choice but to make the minimal diff...

Alternative fix that checks that hunters can actually hunt.

See D2963 for the splash fix.
I think this should wait for that indeed.

In D2962#129125, @Nescio wrote:

If I understand correctly, this means the AI won't be able to train them, since Petra uses the CitizenSoldier class for that (see simulation/ai/petra/attackPlan.js).
The Citizen class is used by auras and technologies to exclude champions and heroes; removing it means elephant archers no longer benefit from those, effectively weakening them, so that means this patch changes gameplay.

In D2229#129120, @Freagarach wrote:

In D2229#129118, @wraitii wrote:

I'm wondering if <Entity> shouldn't be a compulsory tag for simplicity.

I don't think <Entity> should be compulsory. If an entity doesn't have resistance, but can be attacked still. Or one should introduce an Attackable component and use that in range queries and the like. Would make sense. (Then resistance wouldn't be compulsory either.)

I think you missed something in the rebased, TargetKilled is still in Resistance.

In D2960#129087, @Freagarach wrote:

Uhm, what happens when there is no lastPos and an entity gets movementUpdate.failure? They're stuck then, right?

In D2960#129069, @Freagarach wrote:

Are there more places in UnitAI that can benefit from a check?

I don't have a very strong opinion overall, but starting with a more lazy approach seems fine to me.