Implement a Motion Manager around UnitMotion.
rP25071
Actions

Description

Implement a Motion Manager around UnitMotion.

This new MotionManager handles movement for UnitMotion components (not UnitMotionFlying).
This is a first step towards unit pushing, by giving a central place for the relevant units to collide.

One important side-effect is that movement is effectively synchronous - the positions are not actually updated until all units have moved for a turn (refs rP24798).

As a side-effect, it's an optimisation: fewer messages are being sent overall, which leads to a slight speedup (negligible without a lot of units though).

This is a first step - ideally, the movement functions called from UnitMotionManager would actually be moved there.

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

Details

Committed

wraitii

Mar 17 2021, 6:04 PM

Differential Revision

D3509: Motion Manager

Parents

rP25070: Removes joystick from camera controller.

Branches

Unknown

Tags

Unknown

Build Status

Buildable 16225
Build 36375: Trigger Windows Autobuild
Build 36374: Post-Commit Build	Jenkins
Build 36373: Post-Commit Build (macOS)	Jenkins

Event Timeline

wraitii committed rP25071: Implement a Motion Manager around UnitMotion..Mar 17 2021, 6:04 PM

wraitii added an edge: D3509: Motion Manager.

Harbormaster completed building B16225: rP25071: Implement a Motion Manager around UnitMotion..Mar 17 2021, 6:42 PM

vladislavbelov added a subscriber: vladislavbelov.Mar 17 2021, 9:47 PM

vladislavbelov added inline comments.

/ps/trunk/source/simulation2/components/CCmpUnitMotionManager.cpp
22–23	Includes should have alphabet order.
25	Useless empty line.
31–38	This section should be after `public`.
40	That also can break `--without-pch`.
79	Not needed space after `>`.
87	Not needed space after `>`.
/ps/trunk/source/simulation2/components/ICmpUnitMotion.cpp
51–55	A `private` section shouldn't interrupt a `public` section.
/ps/trunk/source/simulation2/components/ICmpUnitMotion.h
39	Strong coupling.
/ps/trunk/source/simulation2/components/ICmpUnitMotionManager.h
33–34	That broke `--without-pch`.
37–42	Also.

Thanks for noting noPCH breakage, will fix.

/ps/trunk/source/simulation2/components/CCmpUnitMotionManager.cpp
31–38	Most components actually define their variables on top (in fact, usually after ClassInit / DEFAULT_COMPONENT_ALLOCATOR). The reason I can think of is that components usually are not split header/C++, meaning they have a lot of functions and some are inlined, some outlined, so it's generally better for readability if you know member variables are at the top. Further, the interface is defined in another file, so there's limited need for that on top.
/ps/trunk/source/simulation2/components/ICmpUnitMotion.h
39	It certainly is... In fact, I gave this some more thought and I probably should just define CCmpUnitMotion inside the same TU as CCmpUnitMotionManager because they are, in effect, 100% coupled. Would allow me to remove this ugliness from the interface. (We have a similar problem with CmpObstruction/CmpObstructionManager)

vladislavbelov added inline comments.Mar 18 2021, 10:39 AM

/ps/trunk/source/simulation2/components/CCmpUnitMotionManager.cpp
31–38	I suppose all functions with non-empty bodies might be outlined and then you'd need to less scroll to the private section, because it's under the public one.

wraitii mentioned this in D3707: Fix issues with motionManager / rP25071.Mar 18 2021, 12:20 PM

wraitii mentioned this in rP25078: Fix issues with motionManager / rP25071.Mar 18 2021, 5:40 PM

wraitii mentioned this in D3732: Enforce tight coupling of CCmpUnitMotion/CCmpUnitMotionManager.Mar 24 2021, 11:00 AM

Not sure if this is intended, but with this commit I get multiple units standing on exactly the same spot.
Notice how the blue and the white coat are intersecting:

To reproduce just order a bunch (50) of units to the same spot without using formations.

In rP25071#50071, @Imarok wrote:

Not sure if this is intended, but with this commit I get multiple units standing on exactly the same spot.
To reproduce just order a bunch (50) of units to the same spot without using formations.

Freagarach reported that too. It's indeed this commit, but it should be fairly hard to reproduce, and I intend to fix it with unit pushing. At the moment I'm treating this as "working as designed", might need to reconsider that in the future but I'll see.

In rP25071#50082, @wraitii wrote:

In rP25071#50071, @Imarok wrote:

Not sure if this is intended, but with this commit I get multiple units standing on exactly the same spot.
To reproduce just order a bunch (50) of units to the same spot without using formations.

Freagarach reported that too. It's indeed this commit, but it should be fairly hard to reproduce, and I intend to fix it with unit pushing. At the moment I'm treating this as "working as designed", might need to reconsider that in the future but I'll see.

Ahm, I'm sorry but it's very easy to reproduce.

In rP25071#50086, @Imarok wrote:

Ahm, I'm sorry but it's very easy to reproduce.

Yeah, this isn't really what I meant either, let me reword that :P
From what I've experience, it happens, but in less than 5-10% of cases or so (i.e. rarely / not systematically) -> rarely enough that I don't think it I should fix it right now, as it doesn't really prevent playing in a super significant way.
Unit Pushing will separate such 'stuck' units and that's what I intend to use to fix this problem - should I fail to get a working diff for that I'll have to update the functions a bit.

Edit -> though TBH it'd be trivial to fix SVN, might still do so anyways.

wraitii mentioned this in D3746: Fix unit overlapping following rp25071.Mar 26 2021, 4:05 PM

In rP25071#50087, @wraitii wrote:

In rP25071#50086, @Imarok wrote:

Ahm, I'm sorry but it's very easy to reproduce.

Yeah, this isn't really what I meant either, let me reword that :P
From what I've experience, it happens, but in less than 5-10% of cases or so (i.e. rarely / not systematically) -> rarely enough that I don't think it I should fix it right now, as it doesn't really prevent playing in a super significant way.
Unit Pushing will separate such 'stuck' units and that's what I intend to use to fix this problem - should I fail to get a working diff for that I'll have to update the functions a bit.

Ok, fine. I was just wondering because this side-effect was stated nowhere. The description of this diff also sounds like it's more a change in code structure and performance and not in functionality. Thanks for clarifying.

In rP25071#50089, @Imarok wrote:

Ok, fine. I was just wondering because this side-effect was stated nowhere. The description of this diff also sounds like it's more a change in code structure and performance and not in functionality. Thanks for clarifying.

See D3746 anyways, I think this makes the pushing comparison fairer so I'll fix it :)

In rP25071#50091, @wraitii wrote:

In rP25071#50089, @Imarok wrote:

Ok, fine. I was just wondering because this side-effect was stated nowhere. The description of this diff also sounds like it's more a change in code structure and performance and not in functionality. Thanks for clarifying.

See D3746 anyways, I think this makes the pushing comparison fairer so I'll fix it :)