Page MenuHomeWildfire Games

Rally points are not deleted if one selects the building just before it is captured.
ClosedPublic

Authored by Stan on Nov 24 2018, 5:20 PM.

Details

Reviewers
elexis
vladislavbelov
wraitii
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP22016: Properly reset rally points on ownership changes.
Trac Tickets
#5352
Test Plan

Use the steps in the ticket, see it's not broken anymore.

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

Stan created this revision.Nov 24 2018, 5:20 PM
Stan updated the Trac tickets for this revision.
Vulcan added a subscriber: Vulcan.Nov 24 2018, 5:22 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/798/

wraitii added a reviewer: Restricted Owners Package.Dec 31 2018, 10:00 AM
Stan updated the Trac tickets for this revision.Jan 3 2019, 2:50 PM
Angen added a subscriber: Angen.Jan 3 2019, 3:04 PM

Also, if you place rally point and you click onto building to remove it, it stays visible.

Stan added a comment.Jan 3 2019, 3:06 PM

@Angen is it fixed by this patch ?

Angen added a comment.Jan 3 2019, 3:27 PM

yes it is fixed

Angen awarded a token.Jan 3 2019, 3:29 PM
elexis added a comment.Jan 3 2019, 4:11 PM

It sounds like RecomputeAllRallyPointPaths and UpdateMessageSubscriptions trigger the removal of some residue that may be solved more directly? On MT_Destroy, every involved data should be deleted? But it maybe doesn't need to do the rest of the work of that function and instead could delete more directly?
(Otherwise, if the current diff is the most straight-forward one, then one should check that the other callers to Reset also intend to have the other members reset).

wraitii accepted this revision.Jan 3 2019, 7:09 PM
wraitii added a subscriber: wraitii.

Checked: other calls to Reset do intend to reset.

The problem in the original ticket's case is with MT_ownershipchanged. RallyPoint.Js calls reset, which doesn't reset markers, and the CmpRallyPointRenderer.cpp goes through MT_OwnerShipChanged, which updates markers but that doesn't delete them.

So this patch is correct in upgrading reset() to delete markers. Calling Reset on destroy also makes sense.

The question of "should CmpRallyPointRenderer.cpp handle ownership changes" is one for a refactoring of this component, which is sorely needed but not in the works atm.

source/simulation2/components/CCmpRallyPointRenderer.cpp
344 ↗(On Diff #7003)

Put this block on the line above.

This revision is now accepted and ready to land.Jan 3 2019, 7:09 PM
Stan updated this revision to Diff 7226.Jan 3 2019, 8:22 PM

Update the diff with @wraitii's last comment to get an autobuild by Vulcan.
Bump the year.

Vulcan added a comment.Jan 3 2019, 8:25 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/895/

This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Jan 3 2019, 8:33 PM