One of those bugs that is likely going to be hard to track. Pressing the Detail window on some of my train convoys on the server is causing the client to fatal error. Since the server is not crashing this is likely a bug with the detail window itself, or more specifically the algorithm used to populate the window.
Since industrial trains have just hit the server I am trying a wide range of convoy configurations to try and decide which to roll out more widely. Some of these convoys feature medium, high and very high passenger classes. Each of these classes has been repurposed. The result is a huge mess of the following variables.
- Medium, High and Very High changed to Very Low, Medium and Very High
- Medium and Very High changed to Very Low and Medium
- Passenger class allocations changed after passengers boarded at old class.
- Some convoys having 3 passenger classes. This used to be a problem with ships but I phased out those ships to only 2 classes for efficiency.
- First time on server game using Very High passenger class. There were no convoys with such a class before now.
- Some convoys have mail which is left at standard class.
- Some coaches might contain standing passengers.
The convoys most prone to this crash have Medium and High which were repurposed to Very Low and Medium without any mail.
My theory is that there is a bug with the detail listing logic when it comes to classes. That seems the most likely cause seeing how it has given problems and buggy behaviour in the past.
Are you able to reproduce this in a small game? If so, then I should be able to track it down (having worked in that area of the codebase recently), but I am unable to run the server game on my current computer.
Still crashing on as good as every train I operate on the server game.
I will try and recreate it on a more manageable test map some time in the future.
Bumping because an edit might get overlooked.
This crash can be recreated using the bug demonstration map posted here...
https://forum.simutrans.com/index.php?topic=18001.msg171486#msg171486
Although the map's purpose is for the one train staff reservation bug, this crash is present in it as well. Select one of the 4 trains and hit the details button. The game immediately crashes.
Crash always occurs when pressing the button using latest nightly x86-64 build on Windows 10. Processor used is an I7 920, in case it is related to instruction set.
I have diagnosed the bug.
The immediate cause of the crash (for the above save) is that the upgrade for the vehicle 'lmr-4wheel-open-coach-short-wheelbase' is NULL, leading to a null dereference. This is a consequence of it being specified in the pakset as upgradable to 'lmr-4wheel-open-coach', which doesn't exist. The intended vehicle is actually named 'LMR-4Wheel-Open-Coach' (note the different capitalisation). Fixing this instance is straightforward; the discussion below is relevant to finding other such issues.
A causal factor is that there is no detection of missing upgrades, and they are not handled gracefully. There are two possible ways of 'fixing' this - either fail fast, or don't fail at all. Not failing at all is the more complicated (and likely messy) option.
Failing fast requires just a one-line change (vehicle_writer.cc:664) to the pakset writer, (it is the pakset itself that determines whether each missing reference is fatal). Since this will not affect existing pakset builds, then it will not cause any new breakages; it will of course require updating the pakset.
An underlying factor is that there appears to be no logging or display of missing references, which means that typos and errors in crossreferences in the pakset can easily go undetected. I think this should be changed also.
I'm going to look into adding better detection of missing references. Also, I think the fail fast approach (as mentioned above) is best, but would appreciate any further remarks on this. (E.g. are broken references used deliberately for WIP in pakset development?)
I support the fail fast approach. Better to catch at build time rather than run time.
It would still be a run time failure. But it would be a guaranteed immediate failure, instead of a failure only when someone takes and action involving a specific vehicle type.
In such a case why not treat such broken links like no upgrade was specified? That seems the most logical approach.
This is required in case people make addons and the user does not want all the upgrades.
I also suggest fixing the incorrect references in the pakset with a merge request over at the pak128 Britain GIT. That would at least defer the problem for now.
Ah, that does sound like a good reason to allow broken links. I'll go for the more complicated approach then.
Thank you for your report: I have now fixed this, and the fix should be in to-morrow's nightly build. The fix that I have applied does not fix the reference errors in the pakset, but simply handles the situation where a specified vehicle is not available without crashing (which it should do in any event). A. Carlotti's fix for the reference issues will still be worthwhile as it addresses a different issues in any event.
I have uploaded a fix for your fix (and a couple of other places that missed out null reference checks). I have also added logging of broken xrefs (as warnings use -debug 2 (or higher) to display them), and made a couple of other logging changes.
I have also uploaded fixes for those broken references in the pakset which I can ascertain to be simple typos. The rest of the broken references (as detailed elsewhere) should be checked by someone more familiar with the intended coupling constraints.
In the latest nightly the game no longer crashes when opening the convoy detail window of those trains. As such this bug can be considered fixed.
If the bigger underlying problem is fixed is another topic.
Splendid, thank you for the further fixes, A. Carlotti, and thank you for confirming the fix, Dr. Supergood. I have now pushed A. Carlotti's fix.
The crash has only been fixed with the most recent nightly. The previous one it could still occur with some engines.