HomeWildfire Games

Unit Motion - wrap target state into a struct
AuditedrP22352

Description

Unit Motion - wrap target state into a struct

These variables together held the state for the target of UnitMotion, as set by the MoveTo[X] family of functions.
Wrapping them in a struct reduces the chances that one will accidentally forget to reset part of the state and makes it explicit in-code that these are grouped together.

Calling StopMoving() resets this target, which wasn't before and left the component in an incoherent state.

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

Event Timeline

Stan raised a concern with this commit.EditedJun 20 2019, 8:50 AM
Stan added subscribers: Krinkle, Stan.

Raising concern for a warning reported by @Krinkle. No biggie but the target parameter isn't used anymore.
Why doesn't it set the the class attribute anymore ?

This commit now has outstanding concerns.Jun 20 2019, 8:50 AM
In rP22352#34056, @Stan wrote:

Raising concern for a warning reported by @Krinkle. No biggie but the target parameter isn't used anymore.
Why doesn't it set the the class attribute anymore ?

It doesn't have to, since the struct has a "type" parameter. I guess I could reset it for sanity's sake.

In rP22352#34056, @Stan wrote:

Raising concern for a warning reported by @Krinkle. No biggie but the target parameter isn't used anymore.
Why doesn't it set the the class attribute anymore ?

It doesn't have to, since the struct has a "type" parameter.
Also it does get reset: m_Entity is INVALID_ENTITY by default and we're using the copy constructor, so it is reset correctly.

elexis added a subscriber: elexis.Jun 25 2019, 8:06 PM

(I didn't verify the correctness completely but what I saw looks okay in terms of encapsulation. The other aspect is the performance and memory footprint. I didn't test that.)

Why doesn't it set the the class attribute anymore ?

What class attribute?

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

This unused target.

From this commit message

Calling StopMoving() resets this target, which wasn't before and left the component in an incoherent state.

From this commits revision proposal D1887:

We also reset these when calling StopMoving(), which was missed before (and which left us in a half-broken state). D1886 makes this fine since unitMotion is no longer stopping on its own.

Missed before means missed in rP22351?
So rP22352 fixes an oversight in rP22351?

Missed before means missed in rP22351?

Negative, "missed before" means "missed ever since UM has been developed" here.

wraitii added inline comments.Jun 30 2019, 1:01 PM
/ps/trunk/source/simulation2/components/CCmpUnitMotion.cpp
1074

This is broken - ranged movement against a point will no longer return false.

wraitii requested verification of this commit.Jul 19 2019, 2:45 PM
This commit now requires verification by auditors.Jul 19 2019, 2:45 PM
Stan accepted this commit.Aug 1 2019, 3:26 PM
All concerns with this commit have now been addressed.Aug 1 2019, 3:26 PM