Page MenuHomeWildfire Games

Get rid of special_units folder for packable siege
ClosedPublic

Authored by fatherbushido on Sep 22 2017, 9:10 AM.

Details

Summary

The idea (in rP12937) was to used that to avoid duplication of Attack entry (which is needed to order attack for unpacked and packed units).
In fact we could imo get rid of that by using another inheritence.

(We could remove it to unplaceable filters in D878)

Test Plan
  • Agree or disagree with that schema.
  • Tests already done, but you can check again:
    • launching unit scenario demo map
    • in game test
    • attached paste are the(inherited) templates before and after .

(only parents are different in the full parsed templates as shown with a by word diff like git diff --color-words old.txt new.txt)

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

fatherbushido created this revision.Sep 22 2017, 9:10 AM
fatherbushido edited the summary of this revision. (Show Details)Sep 22 2017, 9:37 AM
fatherbushido edited the test plan for this revision. (Show Details)
Vulcan added a subscriber: Vulcan.Sep 22 2017, 9:58 AM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jenkins-master:8080/job/phabricator/2059/ for more details.

Executing section Default...
Executing section Source...
Executing section JS...
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jenkins-master:8080/job/phabricator_lint/541/ for more details.

fatherbushido edited the test plan for this revision. (Show Details)Sep 22 2017, 10:16 AM
fatherbushido edited the test plan for this revision. (Show Details)
elexis accepted this revision.Sep 22 2017, 12:41 PM
elexis added a subscriber: elexis.

That (inheriting the packed from the unpacked template) is a good idea to get rid of the parent templates without copying anything to the child templates. Agree!

git diff --color-words old.txt new.txt

Guess you'd have to post the output of that or link the branch. (Using git is a very good idea when working on multiple patches depending on each other)
Couldn't really notice the difference in the files.

Tested some units on the scenario demo map and it seems to be working fine.
Accepting assuming that you edited responsibly and tested more carefully than me.
Thanks for the patch.

This revision is now accepted and ready to land.Sep 22 2017, 12:41 PM
fatherbushido edited the summary of this revision. (Show Details)Sep 22 2017, 3:40 PM
fatherbushido edited the test plan for this revision. (Show Details)

Thanks for the input, I will check again before commiting.

fatherbushido edited the summary of this revision. (Show Details)Sep 22 2017, 5:56 PM
This revision was automatically updated to reflect the committed changes.