Use the steps in the ticket, see it's not broken anymore.
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
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Lint Skipped - Unit
Unit Tests Skipped - Build Status
Buildable 6686 Build 11004: Vulcan Build Jenkins
Event Timeline
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/798/
Also, if you place rally point and you click onto building to remove it, it stays visible.
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).
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 | ||
---|---|---|
347 | Put this block on the line above. |
Update the diff with @wraitii's last comment to get an autobuild by Vulcan.
Bump the year.
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/895/