|
Sun 07 Apr 2013 06:19:55 AM UTC, comment #16:
> the only known user-visible change here is to fix the unloading
> in assert-less release builds?
Yes
|
Sat 06 Apr 2013 06:59:35 PM UTC, comment #15:
I'm still struggling to describe the change made here for NEWS.
Am I right in thinking that the original symptom was finally explained by bug #20699, and that the only known user-visible change here is to fix the unloading in assert-less release builds?
|
Mon 01 Apr 2013 02:16:14 PM UTC, comment #14:
> Maybe so, I had no reproducible case to test against.
But now I had, and it got fixed almost accidentally by bug #20699 patch. When pirate ship is wiped (usually retired) and it has both undisbandable Barbarian Leader and men in it, and Leader gets teleported away (should this happen for barbarian leaders?), counter of units to drown gets subtrackted by one, but that unit then is not the Leader himself...
|
Thu 14 Mar 2013 09:44:25 AM UTC, comment #13:
> unit.c:
> fc_assert(unit_transport_unload(pcargo)); ->
> unit_transport_unload(pcargo);
Quite right. I was indeed snooze.
|
Thu 14 Mar 2013 07:27:18 AM UTC, comment #12:
> I can't actually see how unit_list_iterate could do the wrong
> thing here, where only the current unit can be removed.
Maybe so, I had no reproducible case to test against. But then, as far as I can see, unit.c assert can happen only if unit_transport_unload() fails to remove unit from the list -> list size > 0 even after unit_transport_unload() called for all units in it.
> I don't see anything in the committed change about moving
> functionality out of asserts
unit.c:
fc_assert(unit_transport_unload(pcargo)); ->
unit_transport_unload(pcargo);
|
Thu 14 Mar 2013 01:10:51 AM UTC, comment #11:
Also, while this is a fine change, I can't actually see how unit_list_iterate could do the wrong thing here, where only the current unit can be removed. Perhaps I'm just being dim.
Has this change been observed to remove a symptom?
Another opportunity to prove I'm asleep: comment #5 says:
> 1) It's done in fc_assert() so won't do anything in release build
I don't see anything in the committed change about moving functionality out of asserts. Has this been dealt with? I can't spot the original problem. (This wasn't bug #20519, was it? -- but fix committed before the comment was made, and caused no bug anyway.)
|
Wed 13 Mar 2013 11:54:47 PM UTC, comment #10:
I think the code had been this way since patch #2477, more or less. So it probably caused some form of trouble in 2.4.0-beta1?
|
Tue 05 Mar 2013 07:34:31 AM UTC, SVN revision 22476:
Fixed iteration through units to be unloaded from transport.
See bug #20498
(Browse SVN revision 22476) |
Tue 05 Mar 2013 07:34:26 AM UTC, SVN revision 22475:
Fixed iteration through units to be unloaded from transport.
See bug #20498
(Browse SVN revision 22475) |
Wed 27 Feb 2013 11:57:06 AM UTC, comment #7:
Fix
(file #17363)
|
Wed 27 Feb 2013 11:38:23 AM UTC, comment #6:
> 2) It uses unit_list_iterate() when it should use
> unit_list_iterate_safe() as units are removed from the list
> during the iteration
...which gave me the idea that I should check other users of unit_transport_unload(), and indeed wipe_unit() has the same error. That one can explain all these problems, I think.
|
Wed 27 Feb 2013 11:26:39 AM UTC, comment #5:
> [13:14:57] in unit_virtual_destroy()
> [../../src/common/unit.c::1890]:
> assertion 'unit_list_size(punit->transporting) == 0' failed.
This at least indicates easy to spot bug. It might be separate issue, as I didn't see this failed assert as the first one but only later appearance.
Code above this assert tries to unload all units from transport, so soemthing has to go wrong with it for the assert to fail. And indeed, there's two problems in unloading:
1) It's done in fc_assert() so won't do anything in release build
2) It uses unit_list_iterate() when it should use unit_list_iterate_safe() as units are removed from the list during the iteration
|
Wed 27 Feb 2013 11:17:42 AM UTC, comment #4:
In a longish game I first got for a long time (many turns) every turn several:
in begin_phase() [../../src/server/srv_main.c::945]: assertion 'ptrans != ((void *)0)' failed
...and now, maybe related...
in unit_transport_unload() [../../src/common/unit.c::2272]: assertion 'same_pos(unit_tile(pcargo), unit_tile(ptrans))' failed.
[13:14:57] Please report this message at http://gna.org/projects/freeciv/
[13:14:57] in unit_virtual_destroy() [../../src/common/unit.c::1890]: assertion 'unit_list_size(punit->transporting) == 0' failed.
[13:14:57] Please report this message at http://gna.org/projects/freeciv/
[13:14:57] in wipe_unit() [../../src/server/unittools.c::1767]: assertion 'drowning == 0' failed.
|
Sun 17 Feb 2013 08:36:59 AM UTC, comment #3:
Now I no longer have seen that... there's client side problems while observing, but those are not so critical.
|
Mon 11 Feb 2013 03:38:39 PM UTC, comment #2:
I've seen this in TRUNK only, but as it started after recursive transport wiping fixes went in, I'm quite sure that S2_4 too is affected. Don't want regression like that to beta2.
There was another transport fix that went to S2_3 too, but I hope it's not the faulty one, and we can release 2.3.4 as planned next weekend.
|
Mon 11 Feb 2013 04:42:27 AM UTC, comment #1:
I don't see how it could explain sanity check failures with rules in use, but barbarian creation just creates a ferry and then units inside, without checking if they are suitable cargo for the ferry. Creation of the units try to load them to ferry with 'force' being FALSE, so they would end failing the loading to ferry.
|
Mon 11 Feb 2013 04:22:42 AM UTC, original submission:
I've lately got "ptrans != NULL" sanity failures. One time I managed to debug it down to the fact it was Pirate Archer in Deep Ocean tile. The unit was already several turns old when sanity check first failed, so it was not just spawned there without ferry.
|
Related Recipes:
Freeciv - Bugs: bug #20498, Pirate archers alone in the water