bugFreeciv - Bugs: bug #19821, [metaticket] Numerous issues with...

 
 
Show feedback again

bug #19821: [metaticket] Numerous issues with wipe_unit() wiping transporter with cargo

Submitted by:  Jacob Nevins <jtn>
Submitted on:  Sun 17 Jun 2012 10:56:36 PM UTC  
 
Category: generalSeverity: 3 - Normal
Priority: 5 - NormalStatus: Fixed
Assigned to: Marko Lindqvist <cazfi>Open/Closed: Closed
Release: 2.2.7, 2.3.2, S2_4Operating System: Any
Planned Release: 2.3.4, 2.4.0, 2.5.0

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Fri 25 Jan 2013 12:38:50 PM UTC, comment #10:

#7 is in bug #20454

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Fri 25 Jan 2013 12:04:41 PM UTC, comment #9:

> I checked TRUNK, and there client automatically updates also
> transporter when it receives unit_info for cargo with new
> transporter.


That works for unit owner, but enemies do not receive unit info for unit hiding inside transporter.

1) Start two clients for two players
2) Place two ships (client A) to same tile, with one of them havign cargo and the other not.
3) Place enemy (client B) ship next to previous ones
4) Both clients see that there's one player A ship with cargo and one without
5) Disband ship with cargo
6) Client A correctly sees that cargo has moved to remaining ship. Client B still sees ship as empty.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Fri 25 Jan 2013 11:56:16 AM UTC, comment #8:

> I don't see why a similar issue can't apply to S2_4
> (unit_transport_load() vs unit_transport_load_send(), but I
> didn't notice it in testing.


I checked TRUNK, and there client automatically updates also transporter when it receives unit_info for cargo with new transporter. This code should be identical in S2_4.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sat 19 Jan 2013 07:00:53 AM UTC, comment #7:

#2 is in bug #20442

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sat 19 Jan 2013 06:48:56 AM UTC, comment #6:

It seems that #1 has been fixed a long time ago by bug #19822

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Tue 04 Dec 2012 11:27:40 PM UTC, comment #5:

Most of the issues that apply to S2_3 have been fixed, except for issue 7. I suspect that's not going to be fixed before 2.3.3, unless someone's very quick about it :) (I won't have time)

The remaining issues (1 and 2) are specific to S2_4 and later (recursive transports).

Jacob Nevins <jtn>
Project Administrator
Sat 17 Nov 2012 02:31:10 AM UTC, comment #4:

4) is now bug #20301 concerning all cases where units_lost is not properly increased.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Fri 05 Oct 2012 04:05:53 AM UTC, comment #3:

3 is now bug #20221 concerning all cases where unit is lost without signal being emitted.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Fri 05 Oct 2012 03:54:21 AM UTC, comment #2:

> I believe issues 3, 4, 5, 6 apply on S2_3 as well.


- 5 & 6 were handled in bug #19823
- 3 is potentially disasterous for custom modpack (e.g., victory condition going unnoticed)
- 4 Nasty, but hard to fix. Is this regression relative to what version? (I assume it has been like this for as long as units killed counting has existed)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sun 17 Jun 2012 11:44:36 PM UTC, comment #1:

7. On S2_3, wipe_unit() moving a unit to another transporter doesn't cause the client to be updated -- transporter still looks empty. I suspect that it's due to put_unit_onto_transporter() not doing send_unit_info() for the new transporter (unlike load_unit_onto_transporter()).
(I suspect a similar issue applies to the other "emergency" loading, player_restore_units()' aircraft rescue.)

I don't see why a similar issue can't apply to S2_4 (unit_transport_load() vs unit_transport_load_send(), but I didn't notice it in testing.

Jacob Nevins <jtn>
Project Administrator
Sun 17 Jun 2012 10:56:36 PM UTC, original submission:

Noticed in passing, found through code review, some confirmed through testing (S2_4):

  1. 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.
  2. 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.
  3. Units that go down with the transport don't get a "unit_lost" script signal, only the transporter does. (tested)
  4. 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.
  5. Cosmetic: calls city_units_upkeep() unnecessarily after removing transporter; server_remove_unit() did it already.
  6. 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...

Jacob Nevins <jtn>
Project Administrator

 

(Note: upload size limit is set to 1024 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

No files currently attached

 

Digest:
   bug dependencies.

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -unavailable- added by cazfi (Posted a comment)
  • -unavailable- added by jtn (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 13 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sun 07 Apr 2013 10:11:18 AM UTCjtnDependencies-=>bugs #20722 is dependent
    Sun 27 Jan 2013 10:45:04 PM UTCcazfiCategoryNone=>general
      StatusNone=>Fixed
      Assigned toNone=>cazfi
      Open/ClosedOpen=>Closed
      Planned Release2.3.3, 2.4.0, 2.5.0=>2.3.4, 2.4.0, 2.5.0
    Fri 25 Jan 2013 12:39:05 PM UTCcazfiDependencies-=>Depends on bugs #20454
    Sat 19 Jan 2013 11:13:25 AM UTCjtnDependencies-=>Depends on bugs #20442
    Sat 19 Jan 2013 11:13:11 AM UTCjtnDependencies-=>Depends on bugs #20301
    Sat 19 Jan 2013 11:12:59 AM UTCjtnDependencies-=>Depends on bugs #20221
    Sun 17 Jun 2012 11:01:16 PM UTCjtnSummaryNumerous issues with wipe_unit() wiping transporter with cargo=>[metaticket] Numerous issues with wipe_unit() wiping transporter with cargo
    Sun 17 Jun 2012 11:00:59 PM UTCjtnDependencies-=>Depends on bugs #19823
      Dependencies-=>Depends on bugs #19822
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup