r2119 bug and fix: wrong behaviour of convoys in depots

[gerw]

Easy to reproduce:
Create station 1,2,3. Create a line between station 1 and 2. Now create a convoy and set his line. Then edit the schedule (in depot window) and add station 3. Start the convoy. The convoy still belongs to our line but is driving between all three stations. Although station 3 won't be connected to the others, because the schedule of convoys belonging to a line is ignored.

The attached patch will fix this behaviour. It also changes the behaviour of the schedule editing window opened by a depot. Now you can change there the line of the convoy (This is needed, because the fahrplan_gui_t must have the ability to release the convoy from the line).

@prissi: if I try "patch -p0 < ../line_bug.patch" it complains

Code Select Expand

patching file gui/depot_frame.cc
Hunk #1 FAILED at 1262.
1 out of 1 hunk FAILED -- saving rejects to file gui/depot_frame.cc.rej
Can you tell me why? It is caused by the umlaut in the comment?

[z9999]

In this patch, if I add depot manually to schedule, convoi lost his line, isn't it ?

[gerw]

Yes, he lost his line, because he belongs no longer to this line and his schedule doesn't match the lines schedule. But of course it's usefull, if he remember his line.

Mmmh. Any suggestions how to solve this? Should the convoy remember his line? But he has to forget his line if an other stop is added.

Edit: I take a look at the code. If you add a depot manually (in the svn version) and then the method "fahrplan_gui_t::action_triggered" is called, the convoy will also lost his line. This method is called if you click into the schedule window. I can't check this now, because I'm at work.

[VS]

Some way to remember and load last line might be nice. Button, fake entry in lines (shown as first?)...

[Dwachs]

So the original bug was: the procedure 'edit schedule, start convoy, close all windows' yields a different result than 'edit schedule, close schedule, start convoy' ? Ie starting the convoy from the depot while the fahrplan_gui was open?

I thought it was always like convoys lose its line, if the schedule is changed.

I would propose to add a variable 'linehandle_t old_line' to 'convoy_t'. In fahrplan_gui.cc  (constructor) the old_line can be added (if available) as second entry after "no line selected".

[gerw]

No. The old bug was that you could have a convoy belonging to a line but with a different schedule. And then this schedule was ignored while calculating the routes for the goods.

[z9999]

Oh sorry, don't mind, that is not a problem. 

An another approach to avoid illegal schedule is that disabling add/insert/remove station buttons if convoi has line.
In this case, one can chane the next station but can't change line itself.
So, one should select "<no line>" first, to edit its schedule.

[gerw]

But I think this behaviour is counterintuitive. Maybe we could do the following as Dwachs suggested: If a convoy is sent to the depot he deletes the depot entry in his schedule and switches back to his old line which he remembered.

[Dwachs]

Just another question to clear things for me: the problem was that the fahrplan_gui has no chance to notice if the users adds more halts. The check for current_schedule != line.schedule is only done if some button in the fahrplan_gui is clicked.

So the move to zeichnen() was necessary to do this check more frequently?

[gerw]

Yes. Other functions of fahrplan_gui only will be called, when you do something with the window, e.g., click a button. The halts will be added by a werkzeug_t. Maybe we could there call fahrplan_gui::recheck_line()?

[Dwachs]

How should the werkzeug know about the the convoy etc?

[gerw]

Mmh. Somehow the werkzeug has to add the new stop. I think the werkzeug knows a handle of the schedule. But it won't know the fahrplan_gui, will it?

[prissi]

Personally, I fell like the convoi itself should decide, if it still belongs to a line. A stop in the depot should be still count as identical line. Additional waypoints are not allowed, since they could be promoted to stops later; but only the line schedule it tested when recalculating connections.

I modified matches and the start routine of the convois, to check whether it is still bound to the correct line.

Thank you for finding out this one.

[Dwachs]

this does not work always. sometimes the convoy has the right schedule and line sometimes not.

I tried several times:
-Create line 1 from a-b
-Create convoy, select that line
-Change schedule with additional halt c
-Start convoy

Sometimes the vehicle belongs to line 1, stops a and b, sometimes it belongs to no line, stops abc. So the check works atleast, but gives inconsistent results.

Removing one else did cure the issue (hopefully): in fahrplan_gui::infowin_event, in the block

Code Select Expand


if(ev->ev_class == INFOWIN  &&  ev->ev_code == WIN_CLOSE  ) {
...
// just recheck if schedules match
// remove the else here
if(  cnv->get_line().is_bound()  &&  !cnv->get_line()->get_fahrplan()->matches( sp->get_welt(), fpl )  ) {
cnv->unset_line();
}

the idea is that the check is done when the window is closed

[prissi]

Without the else I got crashes ... I have to look further :(

Ok, it works; but for changing display during game, it needs to do the check in zeichnen. Very unelegant :(

[z9999]

Tested in r2123.

In this case mentioned below, convoi still keep his line, I think it should be detached from line, too.

How to reproduce:
1. Open depot window
2. Make new line
3. Attach line to convoi
4. Open schedule window and add stop
5. Close schedule window

Result:
Convoi belongs to line but has different stop in schedule.

[prissi]

Well, this will be gone when starting the convoi. But you are right, it should better be shown instantaniously.

[z9999]

Well, this will be gone when starting the convoi.

No. I already tested that, too.
But convoi still belongs to line after starting from depot.
So, I reported this problem.

[prissi]

r2124 should fix this too.

[gerw]

There are some more small and hardly observable bugs in this context. Also routing of goods is inconsistent in some cases. I will take a look at this and I will also reduce the calls of welt->set_schedule_counter().

[gerw]

Ok, here is my patch. It's only a beta version yet.

Please test and report when set_schedule_counter isn't called in a situation in which it would be neccesarry. You will see a output in the console if set_schedule_counter is called. set_schedule_counter should be called if it is neccesarry to reroute the goods, e.g., if you add a new connection between two stops.

[prissi]

destroy is usually called without vehicles anyway (at least hwen done in a depot). Also when a line is changed, it will update the convois and thus the schedule counter shoud be increased. But I will test this more thouroughly.

By the way, why did you comment out the sanity check for matching lines in simdepot?

EDIT:
I feel like frustrating you, sorry. I had a deeper look and the thing with adding removing vehicles seems too much, especially for adding stuff. Just a change to recalc_catg_index() does the trick nicely. Doing this, I found some errors in recalc_catg_index() I would not have found without your efforts.

[gerw]

By the way, why did you comment out the sanity check for matching lines in simdepot?

Because I called fahrplan_gui_t with the cnv and so the depot don't have to check it.

[z9999]

Tested in r2128
It still has a problem.

1. Open depot window
2. Make new line
3. Apply line to convoi
4. Click "Update line" button
5. Add stop to line

Result:
Covoi detached from his line

Desirable behaviour:
Covoi keeps his line

If you change "Schedule", convoi shoud detached from his line, but in the case of "Update line", it should not happen.

[gerw]

I've tested prissis changes now and found two bugs (I used r2127):

When changing a schedule of an existing line the schedule_counter won't but should be increased.
When adding an existing convoy to a line the schedule_counter should also be increased, since the old schedule of the convoy isn't served any longer.

[prissi]

Chaning lines case a set_schedule update, see simline.cc line 101.

The second bug is indeed true and will be fixed when the SVN works again.