Bug: Buggy replace screen.

[DrSuperGood]

When trying to replace one of my Stage Coaches on the server game the replace window became what can only be described as buggy.

* The transport capacity of the Stage Coach would disappear despite the convoy assembly being valid.
* The running cost of the convoy would some times increase and other times decrease as more horses were added. In any case they did not match the same convoy assembly in the depot, a real working convoy.

This points towards the replace window not correctly accumulating the stats for the entire convoy, eg not factoring in the Stage Coach or skipping a horse pair unit when measuring the convoy assembly. This might be linked to why the game was crashing when replacing convoys in the first place.

[Ves]

I did not alter the price code, only the positions. I will, however take a look on the capacity issue.

edit:
It appears to be a bit more fundamental than it seems.

The function that displays most of the info in the replace window is shared with the depot window and is called "gui_convoy_assembler_t".
In here, the

Code Select Expand

vector_tpl<const vehicle_desc_t *> vehicles;
.. is used to find anything about the vehicles which is then displayed.
What is going wrong is that the

Code Select Expand

vehicles.get_count(){
.. which, among others, is used when counting the goods in each vehicle, does not update it self when using the replace window, so it thinks the convoy still has the old amount of vehicles in it self. Therefore, if you try to replace a 3 vehicle convoy with a 4 vehicle convoy, the information of the last vehicle will be lost.
I must say that I dont understand enough of this to start modifying it deeply. I gues that this is related to the running cost issue, as well as a bunch of other entries not updating correctly.

Maybe James could have a look, or, if you where to be interrested, you have a look?

[jamespetts]

I am looking at this now. The actual number of vehicles in the "vehicles" vector is increased when vehicles are added to the convoy in the replace window: see line 1388 of gui_convoy_assembler.cc. This line is consistently hit when vehicles are added.

Is the problem that you have found that the function vehicles.get_count() is returning the wrong number, or rather that the result of vehicles.get_count() is being stored in a variable somewhere and that variable still used after the number has changed? If the former, it would help me greatly to know the number of the line on which vehicles.get_count() is returning the wrong value. If the latter, it would help me greatly to know what variable(s) are used - in principle, this should be an easy fix if this is the issue, just replacing the variables with a call to the function.

[Ves]

it is in line 673 in gui_convoy_assembler:

Code Select Expand

const uint32 number_of_vehicles = vehicles.get_count();

I had tried to remove the "const", but with no effect.

[jamespetts]

it is in line 673 in gui_convoy_assembler:

Code Select Expand

const uint32 number_of_vehicles = vehicles.get_count();

I had tried to remove the "const", but with no effect.

I am not sure that I follow - does vehicles.get_count() return the wrong number in this place, or is the issue that number_of_vehicles is reused later when it ought to update that number by calling vehicles.get_count() again?

[Ves]

If I in the for loop at line 717 replace

Code Select Expand

for (unsigned i = 0; i < number_of_vehicles; i++)
to:

Code Select Expand

for (unsigned i = 0; i < vehicles.get_count(); i++)
... it doesnt behave differently, which suggests that the vehicle count is not updated with the new convoy.

[jamespetts]

I think that I have fixed this - this was caused by an error in a previous fix. Originally, this window had caused crashes at line 751 (and others in the same block of code) when the new convoy was longer than the old convoy, as, in that case, v would be NULL, and calling v->get_capacity() would result in an access violation/segmentation fault. To fix this, I added a line which would break out of the loop entirely if v were NULL, but this then prevented total_pax and total_standing_pax from being incremented, causing the capacity errors noted. I have now amended the code so that it no longer does this, and the capacity now seems to be reported correctly. I should be grateful if anyone could re-test and confirm this.