bugFreeciv - Bugs: bug #20663, Unloading assert failure when...

 
 
Show feedback again

bug #20663: Unloading assert failure when autoattack kills transport

Submitted by:  Marko Lindqvist <cazfi>
Submitted on:  Wed 27 Mar 2013 10:58:38 AM UTC  
 
Category: generalSeverity: 3 - Normal
Priority: 5 - NormalStatus: Fixed
Assigned to: Marko Lindqvist <cazfi>Open/Closed: Closed
Release: Operating System: None
Planned Release: 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)

Sun 07 Apr 2013 10:04:32 AM UTC, comment #11:

Actually, bug #20705 is the very corner-case issue.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sun 07 Apr 2013 09:59:26 AM UTC, comment #10:

> Is this what the fix for bug #20678 is dealing with?


No, though somewhat connected. bug #20678 is about server not checking if unit died and proceeding in movement handling even if it did (likely to cause server crash). This ticket is about unit dying before client knows where it would die. That said, I should now open separate ticket for the remaining corner-case. I just wanted the common and easily fixed cases resolved already. The remaining issue is likely to be challenging (recursive dependencies between actions make no order of actions correct...)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sun 07 Apr 2013 09:14:59 AM UTC, comment #9:

> After that the only possibility of already moved transporter
> to die before also cargo has moved would be if it moves to
> conquer enemy city and city loss lua-script would kill it.

Is this what the fix for bug #20678 is dealing with?

Jacob Nevins <jtn>
Project Administrator
Tue 02 Apr 2013 07:42:33 PM UTC, SVN revision 22653:

Move cargo to the same tile with transport a bit earlier when transport moves.
Cargo is already in same tile with transport when lua signal is emitted for
transport movement, autoattack may kill transport (and cargo), and when huts
are handled.

See bug #20663

(Browse SVN revision 22653)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Tue 02 Apr 2013 07:42:27 PM UTC, SVN revision 22652:

Move cargo to the same tile with transport a bit earlier when transport moves.
Cargo is already in same tile with transport when lua signal is emitted for
transport movement, autoattack may kill transport (and cargo), and when huts
are handled.

See bug #20663

(Browse SVN revision 22652)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sun 31 Mar 2013 07:33:23 PM UTC, comment #6:

> I think it's possible to fix this in respect to supplied
> rulesets by moving block of code that moves trasnported units to
> new tile to be before handling of autoattack and huts.


Attached patch does just that + cargo movement is also before emitting scripting signal, so script sees cargo in same tile as transport (that's important at last with requested teleporter like tile support). Note that movement of cargo does not trigger any of the huts, scripting signal, or autoattack, so they shouldn't happen before they are called for transport itself.

In supplied rulesets, bug was (at least in theory) present even with autoattack disabled in experimental ruleset - loaded trireme could get killed by hut barbarians on river tile.

(file #17630)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sat 30 Mar 2013 02:13:15 PM UTC, comment #5:

> For the client side I also wonder what happens when allied
> transport (of which you see units inside) moves out of sight.
> Does transport get removed from the client when cargo is still
> in another tile?

I assume this case is why we have the separate transported_by ID on the client at all -- the code talks about the possibility that the client can't always see the transport of a cargo unit it knows about, and I was struggling to think of a case where this could happen. I guess an allied submarine would do?

Feels like if you have units on board a transport, the server ought to always show you any enclosing transports as a special case (but if you launch your last missile from someone else's sub, then you no longer get to see where it is unless you have shared vision). Would need to decide whether you see other cargo; if not then may need care handling capacity limits on the client side. This is all almost certainly a new ticket.

Jacob Nevins <jtn>
Project Administrator
Sat 30 Mar 2013 02:06:04 PM UTC, comment #4:

I think it's the same. I got assert from server side on autogame, but client would probably get same assert upon handling packets server sends in same order.

For the client side I also wonder what happens when allied transport (of which you see units inside) moves out of sight. Does transport get removed from the client when cargo is still in another tile?

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sat 30 Mar 2013 12:26:29 PM UTC, comment #3:

> I got failed assert about cargo and transport being in same tile
> when wipe_unit() tries to unload cargo.

Is this the same assertion that was failing at the start of bug #20045?
(unit_transport_unload() [unit.c::2072]: assertion 'same_pos(unit_tile(pcargo), unit_tile(ptrans))' failed.)

Jacob Nevins <jtn>
Project Administrator
Fri 29 Mar 2013 06:32:08 PM UTC, comment #2:

I think it's possible to fix this in respect to supplied rulesets by moving block of code that moves trasnported units to new tile to be before handling of autoattack and huts.
After that the only possibility of already moved transporter to die before also cargo has moved would be if it moves to conquer enemy city and city loss lua-script would kill it.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Fri 29 Mar 2013 10:39:31 AM UTC, comment #1:

As long as I've been hacking freeciv, I've been unhappy with how transported units are assinged to a tile, and not just to their transport. This bug seems like a argument for doing it so. Having transported units really in the transporter, movement of whole package would be atomic, and there would be cases where some parts of the package are in one tile and other parts in another tile.

Not that I'd want such a fundamental change to stable branches (or even TRUNK this close to S2_5 branching) for resolving this particular ticket, but something we may want to do in the future.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Wed 27 Mar 2013 10:58:38 AM UTC, original submission:

I got failed assert about cargo and transport being in same tile when wip_unit() tries to unload cargo.

From stack trace it's apparent that transport was killed by autoattack when it moved.

I assume that cargo has simply not been moved to new tile yet at that point. Current transport moving might be broken in general case of transport dying immediately when it moves (entering hut comes to mind as another case in addition to autoattack - and lua scripting)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.

 

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

Attach File(s):
   
   
Comment:
   

Attached Files
file #17630:  CargoInTransport.patch added by cazfi (877B - text/x-diff)

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -unavailable- added by jtn (Posted a comment)
  • -unavailable- added by cazfi (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 9 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sun 07 Apr 2013 10:08:10 AM UTCjtnDependencies-=>bugs #20722 is dependent
    Tue 02 Apr 2013 07:42:51 PM UTCcazfiStatusReady For Test=>Fixed
      Assigned toNone=>cazfi
      Open/ClosedOpen=>Closed
    Sun 31 Mar 2013 07:33:23 PM UTCcazfiAttached File-=>Added CargoInTransport.patch, #17630
      StatusNone=>Ready For Test
    Fri 29 Mar 2013 09:47:42 AM UTCcazfiDependencies-=>bugs #20679 is dependent
    Wed 27 Mar 2013 09:31:04 PM UTCcazfiCategoryNone=>general
      Planned Release=>2.4.0, 2.5.0
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup