Differential D439

D13 prerequisite 3: remove existing formation code
AbandonedPublic
Actions

Authored by wraitii on May 7 2017, 10:22 AM.

Details

Reviewers

Itms

Group Reviewers

Restricted Owners Package

(Owns No Changed Paths)

Summary

(from D13 split)

Current formation code suffers from a number of problems, the foremost one being in my opinion that it forces code to be added everywhere for formation support, which is incredibly annoying and error prone.

As part of D13, I suggest removing it and replacing it with a group-walk order instead (see later patches on the GitHub branch). If we ever want to reimplement tight integrated formations, which should in my opinion do it as a single entity that deals with its own subunits or whatever on its own, without having to hack in other piece of codes.

This does not remove ALL the formation code because most of it is reusable later for the group-walk order, but I could change that if you want. In particular Commands.JS and UnitMotion.CPP are mostly unchanged.

Test Plan

Check that I haven't forgotten bits somewhere. Check that it still loads.

Diff Detail

Repository

rP 0 A.D. Public Repository

Branch

/ps/trunk

Lint

Lint OK

Unit

No Unit Test Coverage

Build Status

Buildable 1502
Build 2372: Vulcan Build	Jenkins
Build 2371: arc lint + arc unit

Event Timeline

wraitii created this revision.May 7 2017, 10:22 AM

Owners added a subscriber: Restricted Owners Package.May 7 2017, 10:22 AM

wraitii mentioned this in D444: D13 post-quisite 10: group-walk order.May 7 2017, 11:02 AM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/1045/ for more details.

Harbormaster completed remote builds in B1502: Diff 1701.May 7 2017, 11:24 AM

wraitii added a parent revision: D13: UnitMotion rewrite.Oct 29 2017, 3:30 PM

wraitii removed a parent revision: D13: UnitMotion rewrite.Nov 26 2017, 10:05 AM

wraitii added a child revision: D13: UnitMotion rewrite.Nov 26 2017, 10:09 AM

As an explanation for why I want the formation code removed, as I believe it's utterly broken and didn't support it in D13:

It's a considerable code simplification in unitAI, and it wasn't self contained: it crept in several helpers all over the place. Supporting additional features would have meant more creep.

It doesn't do much that's actually useful: movement is weird, units get stuck, it can't attack as a formation.

I strongly believe that proper formations must be treated as single entities to have any chance of working without feature creep everywhere. @Itms agrees.

The code can always be reused later as it's versioned.

Basically if we ever want formations we'd still imo have to take it all out first, so let's do it now.

The drawbacks:

Unit placement in formation needs to be done manually (as I didn't reimplement all current formations in the group walk) but that shouldn't be too difficult to change.