News:

SimuTranslator
Make Simutrans speak your language.

Bug: Passengers not always allocated to an accomodation class

Started by ACarlotti, January 29, 2018, 07:38:10 AM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

ACarlotti

I think this is one bug with two symptoms, both of which can be observed in the linked save. The first symptom is that when sorting passengers by (accommodation) class, fewer passengers are displayed than when when sorting them by other means. The second symptom is that vehicles can become overcrowded when, if I understand the pak files correctly, this isn't supposed to be possible.

In the linked save,  convoy '(76) Shire horses (pair)' is a rigid stagecoach apparently carrying 13 passengers, of which 6 are in the accomodation class reassigned to low from medium, and none are in the class reassigned from high to medium. (And clearly 6+0!=13). Furthermore, shortly after the game loads, convoy '(629) Shire horses (pair)', which is at Maliham City Stop (from the west) carrying 5 passengers, will pick up 5 passengers, resulting the same loading of 6 low class and no medium class accommodation occupied.

https://simutrans-germany.com/files/upload/Overloaded_Convoy.sve
(I'm still failing to get more than 24 hours; I don't understand why.)


Edit:
I think this is probably the issue mentioned in https://forum.simutrans.com/index.php?topic=17604.msg168460#msg168460

Some of the gui issues mentioned in this thread are also still outstanding (in particular there're still errors relating to empty categories of goods being followed by nonempty categories); I've been looking into freight_list_sorter.cc and reckon I ought to be able to work out and fix those issues.

jamespetts

Thank you for your report. I suspect that these problems are actually distinct: there are some issues which seem specific to the GUI, but the issue with vehicles overloading seems to be distinct. It is not actually possible for any loaded passengers not to be assigned a class at all, since all passengers are stored in an array indexed by the class of the accommodation.

The more serious issue is that relating to the overloading of convoys; I fixed a similar issue a while ago. Unfortunately, I cannot find and fix the problem unless I am able to reproduce a situation in which the convoy actually becomes overloaded: seeing it already overloaded, while demonstrating that the problem occurs, does not assist me to discover how it has occurred. Are you able to upload a saved game in which a convoy becoming overloaded reliably happens a short time in the future after loading the saved game? That would be extremely helpful.

In respect of the GUI, Ves has been working on the GUI aspects, albeit he is rather busy lately as I believe that he is rehearsing to appear in a concert. If you are able to fix the GUI issues, that would be extremely helpful - thank you.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

ACarlotti

Looking at the gui code, I don't see how it could possibly be handling the sort by accommodation - which matches what I see in game. So I'll have to work out how that should work (including how the accommodation information is stored) once I've sorted out sorting by wealth (I think I've already corrected some errors in the code).

As for the overloading, I'll try to get you an example.

jamespetts

Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

Ves

As James state, I'm quite busy and can't have much simutrans hours recently.

I know the sort by accommodation is a mess and I had to do some hacks.
The initial sorting by accommodation is done already in simconvi.cc, and the freightlistsorter is called once for every class onboard and one last time for rest of the good.

However, I don't remember noticing any inconsistencies with that sorting mode, only with sort by wealth, which somehow gets the passengers and mail mixed up when only mail and no pass is on board.

Thank you for looking into it and sorry for creating the bugs in the first place. 

edit:
I got the game compiling again and took a look on the sort by wealth, which should be fixed now: https://github.com/VictorErik/Simutrans-Experimental-Ves/tree/small-gui-fixes

The other issue in your savegame:
Did you reassign the "high" accommodation recently down to medium? If you reassign it to high, you will see that there is now a bunch of people occupying the accommodation, making 6+4=10 as it should.
This is, however, a limitation to which I have not managed to overcome in the GUI. Really though, I think it should be prohibited by the game to change an accommodation in the middle of a jurney, but rather only at stops. Ideally, when you change an accommodation, the previous accommodation is kept, and the action is first performed whenever the convoy next visits a stop.

jamespetts

Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

ACarlotti

Ves:I had spotted that bug in the sort by wealth, but was going to look over the rest of that code to before sorting out compiling and testing.

I don't think I've reallocated classes on that line for a while, although there might have been a few convoys that had the wrong classes for a while.

ACarlotti

Code at https://github.com/andrewcarlotti/simutrans-extended

I've refactored the code that produces headings (fixing the wealth display bug in th process) - that code is now about 50 lines shorter and, in my opinion, much easier to understand.

I've also modified the displayed data in two other ways:
1. Stations now have a summary passenger/mail count when sorting by wealth (as was previously the case for convoys)
2. simconvoi.cc now passes data for all classes to freight_list_sorter.cc, which should have no effect ... except it reveals that some passengers are being conveyed in classes that have no capacity on the convoy.

To elaborate, the problem with overcapacity (at least as I'm encountering it) is that lots of passengers (I've seen up to 8 at a time) are being put in high class accommodation that doesn't exist (my accommodation is reallocated to 4 medium and 6 low).

jamespetts

Thank you for this: that is extremely helpful.

As to the bug regarding passengers being added to the wrong accommodation slot, are you able to upload a saved game in which this issue can reliably be reproduced (i.e. the passengers being put into the wrong place in the array, rather than already being there)?
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

ACarlotti

Having looked at the code, I can see at least one issue in the code that could be causing it. The trouble is that some bits of code refer to classes according to the designation in the vehicle description, and some bits refer to the class as assigned by the player. This inconsistency is present in the code that puts freight into the vehicle.

I should be able to fix this reasonably quickly, but I think there's an important question to answer about the design first: do we want to store passengers according to the assigned (i.e. fare) class of their accommodation, or according to the original class.

Personally, I am strongly in favour of storing them against the original class, for a couple of different reasons:

1. Conceptually, passengers are occupying different classes of accommodation - even if you allocate both areas of a stagecoach to medium fares, that doesn't mean that the passengers experience the same accommodation.

2. The original accommodation classes should never change (except in pakset update, in which case we have issues anyway), whereas fare classes can be modified at any time. If we store passengers according to the fare class, then that means that we'd have to change where they're stored when we change the fare class, or risk overloading. Plus, as we can see, using the reassigned class is already leading to bugs.

This would have a few other consequences (which probably need to be considered anyway):

3. Charging fares - what happens if the fare class is changed while the vehicle is enroute? I think a decent (and simple to code) choice is to say that passengers are charged according to the lowest of their wealth and the current fare class of the accommodation. (An alternative would be to store the fare class when the passengers board, but I don't think that's necessary, since while passengers may end up paying more than they expected upon boarding, we never force them to pay more than they were originally willing to pay).

4. Passengers can actually be charged according to the comfort they experience (comfort does affect price, right?). So no telling the people on the roof ("but *some* people using that ticket got a nice comfy seat, so we're going to make you pay more for it too").

5. What do we display in the accommodation menus? I think it makes sense (particularly in light of the above) to display passengers grouped by their accommodation class; the code could easily be modified to display any of: the accommodation class, the fare class, or both.

I can't think of any other relevant points right now, but I'm sure some will come up.

I'm not convinced it's worth fixing the existing bug, as I'm not sure how easy it is to fix in the current code, and that code would be replaced anyway in the above proposal.

To summarise, what I am saying is:
'I want to have passengers stored in code according to accommodation class, and not according to the fare class. I am willing to (at least try, and hopefully finish by the weekend) coding this, but want your opinion (and anyone else who has one) first.'

Ves

When I made the gui for the existing window, I initially wanted to sort by accommodation in the convoy detail window, but due to it not being possible, I did it the way it's done now. I don't remember James being principally against it, only that it added extra coding work for him as well as it also could make sense to just show the combined accommodations if they share the same class.

Thank you for doing the work in the sorter! I will have a look in there and perhaps learn a thing or two! :)

jamespetts

Thank you very much for your analysis, and apologies for not having had chance to consider this until now. Your work on this is much appreciated.

I agree with your analysis of how, conceptually, passengers and mail should be allocated to classes of accommodation. Indeed, this is the intention behind the current code: anything that attempts to do something else is in error (and is probably my error, although it might conceivably be Ves if the code is GUI specific).

I also agree with your point no. 3, which I believe was how I had planned for it to work at one stage, although I must confess that I cannot recall exactly whether this was implemented as intended.

Thank you again very much for giving this so much detailed consideration: it is very much appreciated.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

DrSuperGood

I have noticed some stage coaches on the server having 12/11 people in them. I do not know if this is just a UI error, if they can overcrowd or if it is some other bug.

jamespetts

Quote from: DrSuperGood on January 31, 2018, 11:17:17 PM
I have noticed some stage coaches on the server having 12/11 people in them. I do not know if this is just a UI error, if they can overcrowd or if it is some other bug.

Thank you for noting this. The stagecoaches should not overcrowd, so this may well be a (and probably this) bug.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

ACarlotti

I have made reasonable progress on this - I haven't actually changed any of the core loading or unloading code methods yet, but I've made consequential changes to most of the other code that refers to storage or capacity by class, and probably fixed around half a dozen bugs I found in the process (often edge cases). Of particular note for now:

1. I decided it would be a good idea to replace vehicle_t::get_capacity with two separate functions: vehicle_t::get_fare_capacity and vehicle_t::get_accommodation_capacity. These names will make it much easier to select the right capacity whenever a capacity is needed, and the lack of clarity is a clear contributing factor to some of the bugs.
2. I've found + fixed some bugs in line_class_manager and vehicle_class_manager. I see you've also been doing stuff with the vehicle_class_manager, Ves, so I hope I won't cause much conflict there.

I can write up a full list of fixes/changes if you like, and upload the progress, once I've got to a compilable state and checked it's working - hopefully that'll be in the next 24 hours or so, although that depends on how quickly we can resolve the following.

James, I have a couple of questions for you (or for anyone else who has answers):
3. At present, the code for upgrading vehicles is complicated by accounting for changes in the number of classes and shifting the passengers/goods around. I can see why the former might be needed (e.g. if it were possible to convert passenger coaches to a travelling post office, for instance), but I don't see any need to allow for vehicles being upgraded while not empty (other than just discarding the contents). So, should this code allow for goods remaining in the vehicle, or should it just discard any that accidentally happen to remain there?

4. How is overcrowding intended to work? It seems to be just one figure that applies over the whole vehicle (ignoring accommodation classes). Would it make more sense to have separate overcrowding levels? For now, it would be easy to make overcrowding apply strictly to the lowest class of accommodation present, but that may not be the best solution.

jamespetts

Quote from: ACarlotti on February 02, 2018, 09:28:23 AM
I have made reasonable progress on this - I haven't actually changed any of the core loading or unloading code methods yet, but I've made consequential changes to most of the other code that refers to storage or capacity by class, and probably fixed around half a dozen bugs I found in the process (often edge cases). Of particular note for now:

1. I decided it would be a good idea to replace vehicle_t::get_capacity with two separate functions: vehicle_t::get_fare_capacity and vehicle_t::get_accommodation_capacity. These names will make it much easier to select the right capacity whenever a capacity is needed, and the lack of clarity is a clear contributing factor to some of the bugs.
2. I've found + fixed some bugs in line_class_manager and vehicle_class_manager. I see you've also been doing stuff with the vehicle_class_manager, Ves, so I hope I won't cause much conflict there.

I can write up a full list of fixes/changes if you like, and upload the progress, once I've got to a compilable state and checked it's working - hopefully that'll be in the next 24 hours or so, although that depends on how quickly we can resolve the following.

Splendid - thank you very much for your work on this: it is very much appreciated. I address your questions below.

Quote
James, I have a couple of questions for you (or for anyone else who has answers):
3. At present, the code for upgrading vehicles is complicated by accounting for changes in the number of classes and shifting the passengers/goods around. I can see why the former might be needed (e.g. if it were possible to convert passenger coaches to a travelling post office, for instance), but I don't see any need to allow for vehicles being upgraded while not empty (other than just discarding the contents). So, should this code allow for goods remaining in the vehicle, or should it just discard any that accidentally happen to remain there?

I do not think that there is any need to retain passengers/mail/goods after a vehicle is upgraded, as this should occur in the depot when the vehicle is empty, so they can safely be discarded in this case.

Quote
4. How is overcrowding intended to work? It seems to be just one figure that applies over the whole vehicle (ignoring accommodation classes). Would it make more sense to have separate overcrowding levels? For now, it would be easy to make overcrowding apply strictly to the lowest class of accommodation present, but that may not be the best solution.

There is indeed only one overcrowded capacity per vehicle: if a vehicle is overcrowded and people have to stand whatever class of ticket that people buy, people are not likely to be inclined to pay more to be equally uncomfortable as they would be standing in a lower class of accommodation. Indeed, railways have traditionally guaranteed a seat to first class passengers. For that reason, it seems sensible to assume that all overcrowded passengers in a vehicle with multiple classes of accommodation are travelling in its lowest class. The overcrowded capacities of many first class rail vehicles in Pak128.Britain-Ex already make this assumption.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

ACarlotti

At last, I have finally fixed this. (I did get rather diverted by other blocking issues in the codebase). Uploaded to github in the branch vehicle_classes, and packaged as a nice sequence of commits. The commit messages should cover all the bugs (known and unknown) that this covers. I believe this fix should close this thread and the two other mentioning overcapacity stagecoaches.

One minor mistake I think I've noticed (but not changed) is in the code for replacing missing vehicles. As far as I can tell last_desc, it's intended to cache the last vehicle replacement found, but is actually reset to null each time. (Or perhaps I'm just don't understand a relevant aspect of C++ yet). In any case, this mistake (if it is one) will have little, if any, effect on the game - it only affects loading time when the pakset has had vehicles removed. (I don't think this needs much discussion, but if so that should probably be in a new thread).

jamespetts

Excellent - thank you for that: that is very helpful. I have now merged this with the master branch. I have had to make some changes to the code to allow this to compile with Visual Studio - I should be very grateful if you could check that my changes are correct as regards your intention.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

ACarlotti

Yep, looks good to me. (One of the issues was that I had been trying int32 rather than sint32, and didn't investigate fully.)

jamespetts

Quote from: ACarlotti on February 09, 2018, 02:21:28 PM
Yep, looks good to me. (One of the issues was that I had been trying int32 rather than sint32, and didn't investigate fully.)

Splendid, thank you. Have a look at simtypes.h for the relevant definitions. These are there in order to maximise cross-platform compatibility, I believe.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

fam621


jamespetts

The fix to this will be applied in to-morrow's nightly download.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.



jamespetts

Quote from: fam621 on February 10, 2018, 05:54:04 PM
Will I have to download it?

That is the only way of getting any new version to your computer: you can either download it manually or using Dr. Supergood's utility.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

fam621

I use the utility but I was only asking incase I have to download it from the Bridgewater Brunel website.

jamespetts

Quote from: fam621 on February 10, 2018, 08:49:29 PM
I use the utility but I was only asking incase I have to download it from the Bridgewater Brunel website.

Unless something has gone wrong, the latest version of the Master branch should always be available via the utility.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.