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



Noticed in passing, found through code review, some confirmed through testing
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
May also need to pass in aggressor player to wipe_unit() to update their

There are so many issues here, some with different solutions, that this may
become a metaticket...


Reply to this item at:


  Message sent via/by Gna!

Freeciv-dev mailing list

Reply via email to