[PATCH] Add tech mods for UnitMotion speeds

[historic_bruno]

Two parts to this:

• Add tech mod support to UnitMotion for walk and run speed
• Dynamically update moving unit speeds when the tech is applied (probably involves changes to UnitAI, possibly VisualActor)

See also IRC discussion on ​2012-05-02 from 21:48 and ​2013-04-06 from 21:11.

[historic_bruno]

wraitii's patch from http://www.wildfiregames.com/forum/index.php?showtopic=17209&st=60#entry269384

[historic_bruno]

So, as mentioned on IRC ​today the patch won't work correctly, not taking into account that tech mods are cumulative and may be applied multiple times by accident.

Possible solutions would be to calculate the tech mod on demand, when e.g. CmpUnitMotion::GetWalkSpeed() is called, or to store the initial speeds and modify those when the relevant tech mod message is received (caching). It's really a matter of efficiency, how often would GetWalkSpeed be called and does it require the caching approach?

The other thing is the patch sets m_Speed to walk speed, since running/walking state isn't stored in UnitMotion, that may mean that running units will begin walking when the tech mod is applied but I haven't tested that. I also don't fully understand the logic of UnitMotion :)

[wraitii]

Looking at my patch, there are actually 4 things: m_WalkSpeed and m_RunSpeed are obvious. There is m_CurSpeed which seems to actually be a statistic (the average speed over the last turn).
It seems to me like the lines doing the actual move are 858 (setting temporary maxspeed), line 883 (setting the maximal distance that can be travelled), and line 891 or 914 (updating the position, then updated line 928).

(note: line numbers are post-my-patch)

I have no idea what we use m_Speed for. A comment suggest that m_Speed is "typically walkspeed but not always". Only it is. I think it might just be a relic from an older time. Relying on WalkSpeed and RunSpeed should be enough, since the movement uses temporary variables to decide which it wants anyway (basicSpeed). Also, given it's asked for pretty often I think (at least once per "Move()"), I actually suggest using a couple of variables: m_InitWalkSpeed and m_WalkSpeed, same with running.

Edit: wait, I'm wrong. The JS actually changes m_Speed, apparently mostly for formations (UnitAI only switches from running to walking, which could use a specific function). I think it'd be safer to call if m_FormationSpeed or something. And make sure we only use it for formation members. Which also means that in a formation we will have to recompute the minimal speed in the JS when a technology is researched.

[historic_bruno]

Yeah, I think the formation controller will need to handle the tech mod message for UnitMotion. I don't like anything about the way unit speed is handled currently and formations make it worse :/

[historic_bruno]

Replying to wraitii:

Edit: wait, I'm wrong. The JS actually changes m_Speed, apparently mostly for formations (UnitAI only switches from running to walking, which could use a specific function). I think it'd be safer to call if m_FormationSpeed or something. And make sure we only use it for formation members.

It could be used anywhere, though, for example once we implement stamina and running/charging some other component might set speeds in between walk and run. So it makes sense to have cached max walk and run speeds with tech mods applied, and a current "desired" speed like m_Speed, and a measured "real" speed based on distance traveled per unit time (previously that was a temp. variable in visual actor).

[sanderd17]

Ideally, it should

• set the walk and run speed, and set m_Speed = walkspeed
• send broadcast a message that the speed has changed
• The message can be read by other components (s.a. garrisonholder), and they can update the m_Speed again.

But as auras will also influence the speed, it would be better for this to wait until auras are in (auras will share functions and messages with technologies).

[sanderd17]

In 14058:

• Fix garisson and formation aura types
• Implement tech mods for UnitMotion speeds, Fixes #1958, based on patch by wraitii
• Implement athen_hero_themistocles ship speed aura as a test

[michael]

Changeset 14058 does not actually work for technologies.

​http://trac.wildfiregames.com/changeset/14058

Works for the ship_speed aura, but not for technologies. I attempted a sample technology with the same modifications format as the aura and receive this error:

WARNING: GetTechModifiedProperty: modification format not recognised (modifying UnitMotion/WalkSpeed): ({affects:Trader, multiply:1.25})

[sanderd17]

The problem is that technologies expect "multiplier", while auras expect "multiply".

This is a problem introduced by implementing auras (I thought the equivalent of "add" would be "multiply", but I didn't check it). But it's not a problem with the implementation of the UnitMotion speeds modifiers.

[michael]

Okay, makes sense. I think though that since the rest of the syntax is almost identical between auras and technologies, then the auras should use the same term "multiplier" as technologies.