Skip to main content ↑ ↓
Topic: r2119 bug and fix: wrong behaviour of convoys in depots (Read 10356 times) previous topic - next topic

r2119 bug and fix: wrong behaviour of convoys in depots

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]
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?

Re: r2119 bug and fix: wrong behaviour of convoys in depots

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

Re: r2119 bug and fix: wrong behaviour of convoys in depots

Reply #2 –
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.

Re: r2119 bug and fix: wrong behaviour of convoys in depots

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

My projects... Tools for messing with Simutrans graphics. Graphic archive - templates and some other stuff for painters. Development logs for most recent information on what is going on. And of course pak128!

Re: r2119 bug and fix: wrong behaviour of convoys in depots

Reply #4 –
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".
Parsley, sage, rosemary, and maggikraut.

Re: r2119 bug and fix: wrong behaviour of convoys in depots

Reply #5 –
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.

Re: r2119 bug and fix: wrong behaviour of convoys in depots

Reply #6 –
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.

Re: r2119 bug and fix: wrong behaviour of convoys in depots

Reply #7 –
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.

Re: r2119 bug and fix: wrong behaviour of convoys in depots

Reply #8 –
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?
Parsley, sage, rosemary, and maggikraut.

Re: r2119 bug and fix: wrong behaviour of convoys in depots

Reply #9 –
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()?

Re: r2119 bug and fix: wrong behaviour of convoys in depots

Reply #10 –
How should the werkzeug know about the the convoy etc?
Parsley, sage, rosemary, and maggikraut.

Re: r2119 bug and fix: wrong behaviour of convoys in depots

Reply #11 –
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?

Re: r2119 bug and fix: wrong behaviour of convoys in depots

Reply #12 –
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.

Re: r2119 bug and fix: wrong behaviour of convoys in depots

Reply #13 –
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]
if(ev->ev_cl**** == 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
Parsley, sage, rosemary, and maggikraut.

Re: r2119 bug and fix: wrong behaviour of convoys in depots

Reply #14 –
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 :(

Re: r2119 bug and fix: wrong behaviour of convoys in depots

Reply #15 –
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.

Re: r2119 bug and fix: wrong behaviour of convoys in depots

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

Re: r2119 bug and fix: wrong behaviour of convoys in depots

Reply #17 –
Quote from: prissi – on
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.

Re: r2119 bug and fix: wrong behaviour of convoys in depots

Reply #18 –
r2124 should fix this too.

Re: r2119 bug and fix: wrong behaviour of convoys in depots

Reply #19 –
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().

Re: r2119 bug and fix: wrong behaviour of convoys in depots

Reply #20 –
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.

Re: r2119 bug and fix: wrong behaviour of convoys in depots

Reply #21 –
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.

Re: r2119 bug and fix: wrong behaviour of convoys in depots

Reply #22 –
Quote from: prissi – on
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.

Re: r2119 bug and fix: wrong behaviour of convoys in depots

Reply #23 –
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.

Re: r2119 bug and fix: wrong behaviour of convoys in depots

Reply #24 –
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.
 

Re: r2119 bug and fix: wrong behaviour of convoys in depots

Reply #25 –
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.