Formations have a 100 run multiplier which effectively sets their maximal walking speed at 100

https://code.wildfiregames.com/D438?id=7178#inline-34668

Sounds hackish

Indeed, why is that run multiplier 100 and not left undefined, or to be defined by the specific formations?

If I look at CCmpUnitMotion.cpp, it seems like the previous RunSpeed was exactly 1?

For some reason (couldn't quickly find why) a formation moves extremely slow right after it is created, when reforming (as in setting another shape) the problem is solved.

Was it that?

In rP22197#32874, @elexis wrote:

Formations have a 100 run multiplier which effectively sets their maximal walking speed at 100

https://code.wildfiregames.com/D438?id=7178#inline-34668

Sounds hackish

Indeed, why is that run multiplier 100 and not left undefined, or to be defined by the specific formations?

Well, that would be a new feature. Note that since the walk calculation is cmpUnitMotion.SetSpeedRatio(minSpeed / cmpUnitMotion.GetWalkSpeed()); you can also adjust the walk speed to speed up/slow down by default.

If I look at CCmpUnitMotion.cpp, it seems like the previous RunSpeed was exactly 1?

For some reason (couldn't quickly find why) a formation moves extremely slow right after it is created, when reforming (as in setting another shape) the problem is solved.

Was it that?

The problem is that SetSpeed didn't perform any checks that the new speed was above the Run Speed, allowing formations to go above it. My new code checks that the run multiplier is below the maximum run multiplier.
I suppose an alternative approach would be to make the run multiplier optional and handle that specifically, but I don't really see the point (in terms of C++ code vs template cleanliness).

make the run multiplier optional and handle that specifically

That's what I was wondering about - whether there was a limit check in place before somewhere, I didn't find one from a quick look.

but I don't really see the point (in terms of C++ code vs template cleanliness).

(I can't imagine that it would leave the C++ code in an uglier state)

In rP22197#32879, @elexis wrote:

but I don't really see the point (in terms of C++ code vs template cleanliness).

(I can't imagine that it would leave the C++ code in an uglier state)

Well, no, but it's already pretty ugly, and it'd code that ends up being formation-specific which is a bit annoying (can't think of a use case where a legitimate unit might want to have an arbitrary top speed). Then I'd rather special-case in the template than in the code.

Yes, the RunMultiplier could just be optional

In rP22197#32880, @wraitii wrote:

In rP22197#32879, @elexis wrote:

but I don't really see the point (in terms of C++ code vs template cleanliness).

(I can't imagine that it would leave the C++ code in an uglier state)

Well, no, but it's already pretty ugly, and it'd code that ends up being formation-specific which is a bit annoying (can't think of a use case where a legitimate unit might want to have an arbitrary top speed). Then I'd rather special-case in the template than in the code.

I agree, there shouldn't be template hardcoding in the code.
Was more wondering whether the RunSpeed multiplier could become optional in the sense that it avoids the "upper limit" code.
Then formations and crazy mods could use it without C++ hardcoding any templates.
However I still didn't understand how the previous code treated formation running - didn't it just set runSpeed = walkSpeed? I don't find the "ignore upper limit" code in the previous revision.

In rP22197#32885, @elexis wrote:

Was more wondering whether the RunSpeed multiplier could become optional in the sense that it avoids the "upper limit" code.

They could I suppose, but at this point it'll be for another diff.

However I still didn't understand how the previous code treated formation running - didn't it just set runSpeed = walkSpeed? I don't find the "ignore upper limit" code in the previous revision.

Formation.js called cmpUnitMotion.SetSpeed(minSpeed); which just did m_Speed = speed;. Nowhere in the code it checked that the new speed could be above the run-speed - the only thing that was useful for was setting the speed of units "running" in formation when they caught up to other units and fleeing.

I guess I can just remove the std::min call and then we're back to the same situation as before, there's no verification and there's no upper limit to running.

Given that the statement m_SpeedRatio = is only intended for the switch fallthrough cases and not MT_Deserialize the basis for my suspicion fall away.

(One can attempt to demonstrate that this commit cannot have introduced an OOS by reducing the current deserialize code to the previous deserialize.)

possibly related bug:
https://wildfiregames.com/forum/index.php?/topic/25840-my-units-get-stuck-in-run-mode/

That appears to be an off-by one rounding issue and TBH I don't really see how this revision changed that...