[11.x] copying a convoy with an open schedule window causes trouble

[Philip]

There is a minor bug in simdepot.cc that will result in convoys that cannot be sent to the depot. I've actually run into this playing, so it's likely other users will make the same mistake. (It's possible I ran into a similar bug, but this is the one I could reproduce)

Unfortunately, this problem won't survive a save/load cycle, so I can't give a save game to demonstrate. Steps to reproduce:
1. click on depot
2. buy vehicle
3. click Schedule
4. set schedule, NOT closing the schedule window afterwards
5. click Copy Convoy
6. click Start
7. click on new convoy
8. click Go to depot

expected result: convoy goes to depot
actual result: nothing happens

The problem, if I am right, is that depot_t::copy_convoi will create a convoi with a schedule with ist_abgeschlossen() == false; depot_t::start_convoi will try to fix that by deleting the schedule window, but there is no window, so the convoi is started with a schedule with ist_abgeschlossen() == false. There is a debug warning at simdepot.cc:593:

Code Select Expand


} else if (!cnv->get_schedule()->ist_abgeschlossen()) {
dbg->warning("depot_t::start_convoi()","Schedule is incomplete/not finished");
}
but that check is never reached in our case.

I think the right solution is simply to refuse to copy a convoy whose schedule window is open/ist_abgeschlossen() is false; in analogy to what we do when we can't afford to buy a new copy of the convoy. The problem is that I also don't understand our behaviour when copying a convoy we can't afford: we appear to be creating empty convoys (but not activating them)! That can't be right? It results in a crash in the following month.

Here's a briefly-tested patch that I think should improve matters:

Code Select Expand


diff --git a/simdepot.cc b/simdepot.cc
index 810f194..74763a3 100644
--- a/simdepot.cc
+++ b/simdepot.cc
@@ -376,6 +376,11 @@ convoihandle_t depot_t::copy_convoi(convoihandle_t old_cnv, bool local_execution
{
        if(  old_cnv.is_bound()  &&  !convoihandle_t::is_exhausted()  &&
                old_cnv->get_vehikel_anzahl() > 0  &&  get_waytype() == old_cnv->front()->get_besch()->get_waytype() )
+               if( old_cnv->get_schedule() && (!old_cnv->get_schedule()->ist_abgeschlossen()) ) {
+                       
+                       create_win( new news_img("Schedule is incomplete/not finished"), w_time_delete, magic_none);
+                       return convoihandle_t();
+               }

                convoihandle_t new_cnv = add_convoi( false );
                new_cnv->set_name(old_cnv->get_internal_name());
@@ -396,10 +401,17 @@ convoihandle_t depot_t::copy_convoi(convoihandle_t old_cnv, bool local_execution
                                        if(!get_besitzer()->can_afford(total_price))
                                        {
                                                create_win( new news_img(CREDIT_MESSAGE), w_time_delete, magic_none);
-                                               if(!new_cnv.is_bound() || new_cnv->get_vehikel_anzahl() == 0)
+                                               if(!new_cnv.is_bound())
                                                {
                                                        return new_cnv;
                                                }
+
+                                               if(new_cnv->get_vehikel_anzahl() == 0)
+                                               {
+                                                       disassemble_convoi(new_cnv, true);
+                                                       return convoihandle_t();
+                                               }
+
                                                break; // ... and what happens with the first few vehicles, if you: ret
                                               
                                        }



[jamespetts]

That is extremely helpful, thank you! I am away for the bank holiday, so won't be able to test/apply your patch for a few days, but this is much appreciated.

[Philip]

I'm afraid I'm still running into convoys with ->ist_abgeschlossen() = false with my first patch. I think it's easy to fix (set abgeschlossen to true in the schedule_t constructor, or call eingabe_abschliessen() in simline_t::create_schedule()), but I'm rather confused about what the abgeschlossen logic is supposed to do (in particular, I don't understand why wkz:stop_moving_t::do_work checks it, and the comment doesn't appear to match the condition being checked). I'll have to read some more code, I guess.

Thank you, James, for always being so kind and polite. It really does make a difference.

[jamespetts]

Thank you for this - this is most helpful. I have now added your fix to the 11.x branch. Apologies for the delay: I was not able to do coding work whilst I was away.

As to what ist_abgeschlossen() does, excactly, I must confess I am not entirely sure: this is part of the code that was written long before I became involved. Google Translate suggests that the phrase translates from German as "is complete", which suggests that what is being checked is whether the schedule is in an unfinished state or not.

Do the examples that you find where ist_abgeschlossen() is false in moving convoys cause any actual problems such as that which you found in your original post, so far as you can tell? If not, it might be best to leave things as they are for the present.

Thank you again very much for your helpful input - it is most welcome.

[TurfIt]

Popping up an error message on all players for the actions of one would be rather irritating - check local_execution...
I think you're looking in the wrong spot to fix this. Following the recipe in Standard results in the 'no schedule' message at step 6. As expected. If the schedule window has not been closed, then copying the convoi should result in the copied convoi having no schedule, since the source has no schedule yet (it only exists in the schedule window at this point). So Experimental is either allowing convois with no schedule to start, or somehow copying a non-existing one.

[jamespetts]

TurfIt - thank you for the suggestion. I have added a check for local execution before displaying the error message.