Page MenuHomeWildfire Games

UnitMotion rewrite
AbandonedPublic

Authored by wraitii on Dec 27 2016, 1:49 PM.

Details

Reviewers
temple
elexis
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Trac Tickets
#4420
Summary

Test it out here: https://github.com/wraitii/0ad/tree/D13_UM_rewrite_core
This includes the absolutely-needed diffs D981, D53 and D438, as well as 2 others.

This is a mostly complete rewrite of UnitMotion (and associated parts). The goals are:

  • vastly increased code clarity and quality
  • reducing chances to shoot oneself in the foot when updating unitMotion
  • bug fixing

The main change is a philosophy change: UnitMotion now always tries to reach the goal, and never assumes more than that. UnitAI is now in charge of stopping when it wants to (this logic has only be partially applied to UnitAI in this diff to keep things… small-ish).


Gameplay-wise, this does:

  • Fix all instances of gliding units (current and hopefully future by structuring the code better)
  • Drastically reduce the number of stuck units by improving short-range/long-range pathfinder compatibility (without changes to the pathfinders themselves!)
  • Allow unitAI to do things while walking (such as fighting or gathering - unimplemented for now, as this will also need animation engine changes)
  • Keep formations and current functionality
  • Set us up for pushing by using a manager.

Performance-wise, ought to be similar (the use of the manager reduces the number of messages sent, but this is offset by probably a little slowness passing around more things)

Test Plan

Play a few games and report anything wrong with unit movements.

Check out the code and see if it feels understandable (and possibly better if you want to compare).

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 278
Build 444: Vulcan BuildJenkins
Build 443: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
wraitii updated this revision to Diff 1707.May 7 2017, 10:54 AM

"Unit motion only" portion of the Unit Motion rewrite, see mentioned patches above and below for the rest.

This is the bulk of the the rewrite, notably rewriting CCmpUnitMotion.cpp and parts of UnitAI.JS to handle it.

Uploaded from a patch because it wouldn't apply neatly I think because of prerequisites.

Vulcan added a comment.May 7 2017, 4:21 PM

Build has FAILED

Link to build: http://jw:8080/job/phabricator/1051/
See console output for more information: http://jw:8080/job/phabricator/1051/console

temple added a subscriber: temple.Jun 21 2017, 6:31 PM
echotangoecho resigned from this revision.Jul 30 2017, 5:37 PM
fatherbushido resigned from this revision.Aug 30 2017, 11:29 PM
Vulcan requested changes to this revision.Feb 10 2018, 10:18 PM
This comment was removed by phabadmin.
This revision now requires changes to proceed.Feb 10 2018, 10:18 PM
Itms removed a reviewer: Vulcan.Feb 10 2018, 10:21 PM
Itms removed subscribers: phabadmin, Vulcan.

And another mistake, sorry for the noise...

New plan: will add back existing formation code, but at the same time add pushing code instead of relying so much on the short-range vertex pathfinder, and then I'm gonna try to optimise the short-range pathfinder by not recomputing so much every time. I have this idea where maybe, maybe this might actually be faster if we do it extensively enough.

To an extent, this will require reviews.

In D13#60555, @wraitii wrote:

existing formation code, but add pushing

I don't know what I'm speaking about, but is it less effort to increase the scope of the changes?
Maybe it is more managable to have the Formation code remain as is where possible until this revision is dealt with.

In D13#60573, @elexis wrote:
In D13#60555, @wraitii wrote:

existing formation code, but add pushing

I don't know what I'm speaking about, but is it less effort to increase the scope of the changes?
Maybe it is more managable to have the Formation code remain as is where possible until this revision is dealt with.

My problem is that implementing pushing (for which I had an idea this morning) will probably require my rewrite. But implementing pushing will be useful for formations as it fixes other issues. And I also have ideas for the vertex pathfinder. Basically expanding the scope might make this less buggy overall, and this is already pretty hard to review correctly, so I don't see that as a much bigger issue.

Basically expanding the scope might make this less buggy overall, and this is already pretty hard to review correctly, so I don't see that as a much bigger issue.

Although it sounds conceivable that squashing two rewrites into one rewrite would make things more buggy,
the point was more about the possibility of having twice the complexity leading to more than twice the endurance needed for each implementation, review and audit (if not making it impossible).
I understand that exploration is an important task, but this patch is already 1.5 years old, had to be split several times already, the discussions in the related patches were long and controversial already,
so better get this committed or rejected with before adding a second rewrite on top if you are not only wanting to explore but get it committed. (Only wanting to explore is ok too, but then don't do this in the reviewspace).

This won't change much about this particular patch, except for adding back formation-specific things, which hopefully will be neat enough.

wraitii updated this revision to Diff 6520.May 11 2018, 2:17 PM
wraitii edited the summary of this revision. (Show Details)
wraitii edited the test plan for this revision. (Show Details)
wraitii edited reviewers, added: temple, elexis; removed: echotangoecho, Itms, fatherbushido.
wraitii updated the Trac tickets for this revision.
wraitii removed subscribers: temple, Lionkanzen, elexis and 10 others.

Big refactoring and update. See summary for details. Test it out here: https://github.com/wraitii/0ad/tree/D13_UM_rewrite_core

I've verified that formations work when walking. I'm not sure how wonky they're supposed to be in SVN in other formats.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/differential/468/display/redirect

Notes on reviewing this:

  • It's probably easier to review UnitMotion without comparing it to what existed before - unless you knew that code well. I've basically rewrote the whole thing and it's different enough that the diff is unhelpful.
wraitii updated this revision to Diff 6522.May 11 2018, 7:59 PM

Fix a number of issue related to this morning's refactoring, as I now needed a few more checks for things to remain sane.

I think there's still a few issues related mainly to unitAI which I'll investigate tomorrow.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/differential/469/display/redirect

wraitii planned changes to this revision.May 12 2018, 7:44 PM

Code needs a few tweaks to work.

wraitii updated this revision to Diff 6535.May 13 2018, 11:17 AM

Fix several issues and actually leverage the refactoring to improve the code. Haven't noticed any obvious flaw when running some basic tests.

This also ships some formation unitAI changes, which I think are fixes (such as double "enter"), but it's still not working perfectly for gathering. However, it seems formation-walking now works better.

The GitHub branch passes all tests (including the unitAI formation tests), though obviously vulcan won't notice.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/differential/482/display/redirect

wraitii updated this revision to Diff 6536.May 13 2018, 4:33 PM

Fix issues with range checks which affected leaveFoundation and probably min-ranges everywhere.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/differential/483/display/redirect

wraitii added inline comments.May 14 2018, 4:42 PM
source/simulation2/components/CCmpUnitMotion.cpp
733–734

Some comments on why svn unitMotion is a pile of hacks that needs to be cleansed with the holy waters of D13.

If you want to have fun, read my commits from Oct 30 to Nov 15 2015, when mimo and I mostly kludged UnitMotion together following the pathfinder commit. They're basically all a mess of "I don't know why this fixes things but it does".

source/simulation2/components/CCmpUnitMotion.cpp
142

Unlike what you might think, this isn't whether the unit is moving the current turn, or whether it's trying to move.

145

So this is the first state enum. You can see already that:

  • it's redundant with m_Moving above.
  • it explicitly contains a hack.
177

Another state, that's not exactly iso with m_State above, but also kind of redundant with both that and m_Moving.

Secondly, this has both "requesting", "following", and "following requesting". Which is annoying.
Furthermore, it has a distinction between long and short paths, which is an anti pattern: it introduces coupling between UnitMotion and the pathfinder, and furthermore unitMotion really shouldn't care what is giving it the path.
And indeed, svn unitMotion doe snot, so this is rather useless.

224

Yet another redundancy, this time with m_Pathstate. Combining m_Moving, m_State, m_PathState and this, we actually can determine two things:

  • Are we trying to move
  • Do we have a path request.

This is done explicitly in D13.

496–497

Replaced by "SetDestination" family, as that's a better name.

498–499

Also these return "false" if we didn't start moving. This can be because we failed or because we are already in range, and there's no way to tell. Returning "true" doesn't actually tell you anything useful except that the entity ack-ed your order.

In D13, these functions return "false" if the move if the target is unreachable, true otherwise. So you can right away tell if the move will succeed, instead of having to rely on a "move will fail" message next turn.

512

Should be compared to "DiscardMove". Here you can see we use the State_Stopping hack, except everything else tells us we're stopped already, and we're not even m_Moving anymore.

537

Compare with D13 HasValidPath. This is checking for extremely redundant stuff. And actually having a path is having waypoints, not this state. So this can end up returning the wrong results if there's an inconsistency.

592

So here we have special functions for starting. Why? Because svn UnitMotion needs that, otherwise we don't go through STATE_STOPPING.

Also notice here that m_Moving means "we have successfully started moving, but not yet completed the move". This is, as said above, neither "we are currently moving", nor "we are trying to move towards a target", since when we haven't started m_Moving is false, but we are trying to move.

699

UnitMotion cannot safely handle a path and not reset is state. D13 can (in fact in D13 PathResult changes no movement related state, only waypoints).

714

Another example of the terrible state coupling.

742

Resetting everything in many places here. Why is this even here? PathResult should not decide that suddenly we're moving.

1222

Again, why does this call state-related functions? Well because this is a mashup of quick fixes.

1657

Complicated and hacks function. Compare "ComputeMotionGoalFromGoal"

source/simulation2/components/CCmpVisualActor.cpp
766

This is bad because it introduces coupling between Visual Actor and unitMotion. And it needs one to listen to Update messages.

In D13 UnitMotion calls visual actor directly to update the speed, which is faster and also less coupled (though Visual actor still calls UnitMotion to get the walk/run speed difference, this could be removed.)

You'll also note that since this doesn't call any of the UnitMotion state, but relies on M_CurrentSpeed, we have the explanation for the STATE_STOPPING hackiness.

wraitii added inline comments.May 16 2018, 7:53 AM
source/simulation2/components/CCmpUnitMotion.cpp
736

I think this is no longer needed with the latest D53 changes, I'll test it out.

wraitii updated this revision to Diff 6558.May 16 2018, 9:02 PM

Low hanging fruits: const-correct, remove unused functions, reorder things.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/differential/500/display/redirect

wraitii abandoned this revision.May 25 2019, 3:13 PM

The point of this diff has been rewritten in about 20 other diffs.