URL:
<http://gna.org/bugs/?19821>
Summary: Numerous issues with wipe_unit() wiping transporter
with cargo
Project: Freeciv
Submitted by: jtn
Submitted on: Sun Jun 17 23:56:36 2012
Category: None
Severity: 3 - Normal
Priority: 5 - Normal
Status: None
Assigned to: None
Originator Email:
Open/Closed: Open
Release: 2.2.7, 2.3.2, S2_4
Discussion Lock: Any
Operating System: Any
Planned Release: 2.3.3, 2.4.0, 2.5.0
_______________________________________________________
Details:
Noticed in passing, found through code review, some confirmed through testing
(S2_4):
0 S2_4 only: when looking for noble cargo (gameloss/undisbandable) to save, it
mistakenly calls unit_transported(punit) where punit is the recently destroyed
transporter. Could lead to nobility not saved before proletariat (seen in
testing) or, presumably, a crash.
0 On S2_4 with recursive transports, if cargo was itself carrying cargo and is
to be lost, there are assertion failures (confirmed in testing) and other bad
things, as unit_lost_with_transport() calls server_remove_unit() which expects
cargo to have been removed first.
0 Units that go down with the transport don't get a "unit_lost" script signal,
only the transporter does. (tested)
0 Nor do they get counted in units_lost or units_killed (wipe_unit() doesn't
handle this at all, callers do, but callers don't and can't account for
cargo). Affects scores.
0 Cosmetic: calls city_units_upkeep() unnecessarily after removing
transporter; server_remove_unit() did it already.
0 Cosmetic: the variable 'drowning' is improperly maintained, being
incremented twice for every decrement. It's used to bail out of iterations
early, so the worst effect is that time will be wasted in rare circumstances.
(This is what drew my eye to this area.)
I believe issues 3, 4, 5, 6 apply on S2_3 as well.
Issues 3, 5, and 6 affect S2_2, FWIW.
On S2_4, the right fix to some of this clearly involves recursion of
wipe_unit(). That may be true for previous branches too.
Moving units_{lost,killed} accounting and signal generation into wipe_unit()
seems like the only answer given the possibility of transport on allied
vessels. But it will need careful handling, with different behaviour depending
on unit_loss_reason (e.g., unit_change_owner() calls wipe_unit(), as does the
editor). But we don't have unit_loss_reason on S2_3 :( -- it came in in patch
#2548.
May also need to pass in aggressor player to wipe_unit() to update their
units_killed.
There are so many issues here, some with different solutions, that this may
become a metaticket...
_______________________________________________________
Reply to this item at:
<http://gna.org/bugs/?19821>
_______________________________________________
Message sent via/by Gna!
http://gna.org/
_______________________________________________
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev
