Page MenuHomeWildfire Games

Garrisoned units are lost after an upgrade towards a non garrison holder
ClosedPublic

Authored by mimo on Jan 16 2018, 10:47 PM.

Details

Summary

Contrary to what was said in the comment in the code, units were not ejected when upgrading towards a non garrisonHolder

Test Plan

Put some units on a wall and upgrade it to a gate

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

mimo created this revision.Jan 16 2018, 10:47 PM
mimo added a comment.Jan 17 2018, 12:21 AM
In D1228#49874, @bb wrote:

That bug was already there before rP20393 (see line 221 before patch). And anyway, i don't understand what you mean with this quote? more context is usually a good idea.

bb added a comment.Jan 17 2018, 5:41 PM
In D1228#49878, @mimo wrote:
In D1228#49874, @bb wrote:

That bug was already there before rP20393 (see line 221 before patch). And anyway, i don't understand what you mean with this quote? more context is usually a good idea.

Added the backref, to the concern raised there, since this is the bug I mentioned in that revision (so when we commit this the concern should be closed). IIRC back then I checked that the revision was causing the bug, but I might be mistaken in that.

mimo updated this revision to Diff 5362.Jan 17 2018, 8:15 PM

cleanup of the patch

mimo added a comment.Jan 17 2018, 8:18 PM
In D1228#49919, @bb wrote:
In D1228#49878, @mimo wrote:
In D1228#49874, @bb wrote:

That bug was already there before rP20393 (see line 221 before patch). And anyway, i don't understand what you mean with this quote? more context is usually a good idea.

Added the backref, to the concern raised there, since this is the bug I mentioned in that revision (so when we commit this the concern should be closed). IIRC back then I checked that the revision was causing the bug, but I might be mistaken in that.

ok thanks, i was not sure there was not something else. Then that patch should fix your concern. Care to review it :)

This revision was not accepted when it landed; it landed in state Needs Review.Jan 20 2018, 4:22 PM
This revision was automatically updated to reflect the committed changes.