Page MenuHomeWildfire Games

Reset Rallypoint when changing Ownership to gaia
ClosedPublic

Authored by elexis on Feb 7 2017, 12:58 PM.

Details

Summary

If someone garrisons a building and becomes defeated, the rallypoint won't be reset, refs rP18848.
The units will follow the rallypoint command if the building is demolished.
This way we can encounter gaia units gathering resources or attacking some enemy or former ally, which seems peculiar enough to remove it.

Test Plan

Test 1: Garrison the civic center, set a rallypoint on aresource, resign, use the control-all-units cheat to delete the building.

Test 2: Replay

(rP19203) to test the performance on resign with 150 houses. I couldn't notice a difference (1-4ms).

Diff Detail

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

Event Timeline

elexis created this revision.Feb 7 2017, 12:58 PM
elexis edited the test plan for this revision. (Show Details)Feb 7 2017, 1:03 PM
Vulcan added a subscriber: Vulcan.Feb 7 2017, 1:42 PM

Build is green

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

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

fatherbushido resigned from this revision.Feb 8 2017, 6:09 PM
Sandarac accepted this revision.Feb 9 2017, 5:26 AM

Ideally it would be nice to have some tests for the RallyPoint component (http://trac.wildfiregames.com/wiki/SubmittingPatches#Makingsomechanges (last sentence) comes to mind), as there currently are none. But of course, this diff is not a gigantic overhaul of anything.

This revision is now accepted and ready to land.Feb 9 2017, 5:26 AM
elexis updated this revision to Diff 485.Feb 9 2017, 12:30 PM

Don't enforce a double standard upon non-team members.

Vulcan added a comment.Feb 9 2017, 1:14 PM

Build is green

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

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

mimo added a subscriber: mimo.Feb 9 2017, 8:07 PM
mimo added inline comments.
binaries/data/mods/public/simulation/components/Player.js
397

should be removed from the patch

404

to be removed to

elexis added inline comments.Feb 9 2017, 8:08 PM
binaries/data/mods/public/simulation/components/Player.js
404

Absolutely, just kept it in there to make it easier for the reviewer ;)

elexis requested review of this revision.Feb 10 2017, 9:38 AM
elexis edited edge metadata.

(Committing the tests without a review and claiming the patch to be reviewed would stretch the review-reviewers patience too far)

mimo added a comment.Feb 10 2017, 10:40 AM

Strange, when applying the patch with "arc patch D132", i get the following, which I've never got before.
A binaries/data/mods/public/simulation/components/tests/test_RallyPoint.js
svn: E135001: Unrecognized line ending style 'native
\ No newline at end of property' for '/data/kubuntu/jeux/0ad/svnbak/binaries/data/mods/public/simulation/components/tests/test_TechnologyManager.js'
svn: E135001: Unrecognized line ending style 'native
\ No newline at end of property' for '/data/kubuntu/jeux/0ad/svnbak/binaries/data/mods/public/simulation/components/tests/test_Technologies.js'
svn: E135001: Unrecognized line ending style 'native
\ No newline at end of property' for '/data/kubuntu/jeux/0ad/svnbak/binaries/data/mods/public/simulation/components/tests/test_RallyPoint.js'

binaries/data/mods/public/simulation/components/RallyPoint.js
110

you could have kept (msg.from == -1 || msg.to == -1) i.e. only removing the gaia case, but not really important

elexis updated this revision to Diff 506.Feb 10 2017, 6:18 PM

Don't reset the rallypoint when destroying the entity.
Add comments to the tests.

elexis marked an inline comment as done.Feb 10 2017, 7:20 PM
In D132#4812, @mimo wrote:

Strange, when applying the patch with "arc patch D132", i get the following, which I've never got before.

I know you don't want to hear it, but I've investigated this a bit (and we don't have a spoiler field for phabricator):
The diff sets the svn property svn:eol-style to native (as it should be, so that windows and linux users dont add two different kinds of \n to the file, adding weird hunks to the diff, as seen in the last man standing patch at the time being).
But the raw diff somehow adds an \ No newline at end of property to it:

Index: binaries/data/mods/public/simulation/components/tests/test_Technologies.js
===================================================================
--- binaries/data/mods/public/simulation/components/tests/test_Technologies.js	(revision 19216)
+++ binaries/data/mods/public/simulation/components/tests/test_Technologies.js	(working copy)

Property changes on: binaries/data/mods/public/simulation/components/tests/test_Technologies.js
___________________________________________________________________
Added: svn:eol-style
## -0,0 +1 ##
+native
\ No newline at end of property

Couldn't find a way to mute it, couldn't add the requested newline to the properties.

The warning seems to come from http://svn.apache.org/repos/asf/subversion/branches/invoke-diff-cmd-feature/subversion/include/private/svn_diff_private.h and that's where the story ends for today :-P
(I've always marked those eol properties that way and I don't think there is an issue with that, except in phabricator maybe)

binaries/data/mods/public/simulation/components/RallyPoint.js
110
In D132#4812, @mimo wrote:

you could have kept (msg.from == -1 || msg.to == -1) i.e. only removing the gaia case, but not really important

Why past tense? Important enough as the author should understand the function he changes.

mimo accepted this revision.Feb 10 2017, 7:39 PM
This revision is now accepted and ready to land.Feb 10 2017, 7:39 PM
This revision was automatically updated to reflect the committed changes.
elexis marked an inline comment as done.

Build is green

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

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