HomeWildfire Games

Change Run Speed into a Run multiplier.
AuditedrP22197

Description

Change Run Speed into a Run multiplier.

This changes running speed into a running multiplier (of walk speed).

The advantage is that it simplifies code since you can setup a default run multiplier at the template level and it'll work for all subsequent templates, and technologies cannot forget to change it. It makes specialised unit templates easier to maintain, too.

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

Reviewed By: bb, O2 JS Simulation

Differential Revision: https://code.wildfiregames.com/D438

Details

Auditors
Angen
Committed
wraitiiApr 19 2019, 12:04 PM
Reviewer
Restricted Owners Package
Differential Revision
D438: Change Run Speed into a Run multiplier
Parents
rP22196: Correctly handle receiving 0 damage as not receiving any damage.
Branches
Unknown
Tags
Unknown
Build Status
Buildable 7211
Build 11747: Trigger Windows Autobuild
Build 11746: Post-Commit BuildJenkins

Event Timeline

elexis added a subscriber: elexis.Apr 19 2019, 9:10 PM

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?

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)

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

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.

/ps/trunk/source/simulation2/components/CCmpUnitMotion.cpp
426

This sounds like it could go out-of-sync, because the clients that did not rejoin use the serialized m_SpeedRatio and the rejoined client uses std::min(m_SpeedRatio, m_RunSpeedMultiplier)?

If those two are always the same, then the computation would not be needed?

(If one can compute the current value from the other values, the value wouldn't have to be serialized, minimizing memory a little)

(I see it was pointed out here https://code.wildfiregames.com/D438?id=4500#inline-20639)

(One may test for this with a massive amount of units walking around in formations, with techs, and then running the rejointest commandline feature on different turns. Or reading the code well and trying to deduce in which case it might go OOS.)

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.

/ps/trunk/source/simulation2/components/CCmpUnitMotion.cpp
426

These two lines are exactly the same as SetSpeedRatio() (which I've missed before committing).

I don't think it can OOS, m_SpeedRatio == std::min(m_SpeedRatio, m_RunSpeedMultiplier) is always true since SetSpeedRatio checks for it and this code does too. So even when rejoining with weird tech thingies it _should_ remain coherent.

elexis added inline comments.Apr 20 2019, 7:05 PM
/ps/trunk/source/simulation2/components/CCmpUnitMotion.cpp
426

If m_SpeedRatio == std::min(m_SpeedRatio, m_RunSpeedMultiplier), then why call
m_SpeedRatio = std::min(m_SpeedRatio, m_RunSpeedMultiplier)?

Isn't the statement either a null-statement or triggering an OOS?

(Also one can call SetSpeedRatio instead of replicating it)

(And if it's recomputed from the template values and tech/aura effects, then the serialized value is never read from?)

// Adjust our speed. UnitMotion cannot know if this speed is on purpose or not so always adjust and let unitAI and such adapt.

Sounds like the deserialized unit has unitAI adapting for something that the non-deserialized unit didn't adapt for?

This commit now requires audit.Apr 21 2019, 10:08 AM
bb added a subscriber: bb.Apr 21 2019, 5:34 PM
bb added inline comments.
/ps/trunk/source/simulation2/components/CCmpUnitMotion.cpp
426

Please consider all times this code is ran: MT_ValueModification (the case above with a FALLTHROUGH). In case the RunMultiplier gets lower, and even below m_SpeedRatio, we need to adapt the speed

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.)

This commit no longer requires audit.Apr 21 2019, 7:36 PM

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

Angen added inline comments.May 6 2019, 9:28 AM
/ps/trunk/binaries/data/mods/public/simulation/components/UnitAI.js
2053

why was this reset deleted? now unit keeps run speed after leaving CHASING state.

wraitii added inline comments.May 6 2019, 9:43 AM
/ps/trunk/binaries/data/mods/public/simulation/components/UnitAI.js
2053

I think in an earlier revision the "run speed when chasing" behaviour was removed entirely. It'll need to be added back indeed, feel free to raise a concern, I'll merge it with the upcoming "fix variable names" diff.

Angen raised a concern with this commit.May 6 2019, 9:48 AM
This commit now has outstanding concerns.May 6 2019, 9:48 AM
Angen accepted this commit.May 11 2019, 5:11 PM
All concerns with this commit have now been addressed.May 11 2019, 5:11 PM
Angen added inline comments.
/ps/trunk/binaries/data/mods/public/simulation/components/UnitAI.js
3121

reported by @wowgetoffyourcellphone that walk animation of animal after capture has different behaviour as when it was wild

here should be animation "move" not "walk"
because this is setting animation speed to 1, and "move" is setting animation speed based on its walk speed

Angen added inline comments.May 12 2019, 9:15 PM
/ps/trunk/binaries/data/mods/public/simulation/components/UnitAI.js
3121

actually, @wraitii, why did you change this.GetWalkSpeed() => 1 ?

wraitii added inline comments.May 13 2019, 9:11 AM
/ps/trunk/binaries/data/mods/public/simulation/components/UnitAI.js
3121

Honestly this is also probably a leftover from an version of the patch (possibly going all the way back to D13 in January 2017), and it was missed by bb and I during the review.

The "walk" thing here will be deleted in an upstream patch of mine (it won't be necessary anymore) so I'm not going to fix this specifically, but thanks for reporting it :)