News:

Simutrans.com Portal
Our Simutrans site. You can find everything about Simutrans from here.

Lack of guaranteed consistency of number_of_classes

Started by ACarlotti, February 03, 2018, 09:53:17 AM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

ACarlotti

I've come to the conclusion that the having vehicle_descs storing number_of_classes that could differ from the 'global' settings is undesirable, since there are so many places in the code where one would want to assume that number_of_classes is consistent. There are presumably similar issues that can arise from other objects that have per class settings (such as buildings). I think it would be much better if we handled all such discrepancies at load time (where we would only have to deal with them once), so that we don't have to worry about them once running the game (where we might use the per class data in lots of different ways.

I initially thought it would be fairly straightforward to correct the number of classes as we load each object in the pakset. However, this approach would (at present) fail, because the (global) numbers of passenger/mail classes are defined in the good.*.pak files, which might not be loaded by the time other objects want to know the data.

I can thus see four possible options for dealing with this:
1. Keep the paksets as they are, and add code to fix the classes at a later stage. This could maybe be done in calls to successfully_loaded() or similar, but would be a lot of hassle compared to 2.
2. Modify the pakset scheme so that the number of classes is defined in simuconf.tab (or other suitable location), which is loaded and can be accessed before we read the .pak files. This would seem to be the simplest approach, barring any complications in moving and accessing that setting. (Note that in this scheme, changing the value in simuconf.tab might impact on balance by merging classes settings together if necessary, but wouldn't fundamentally break any code.)
3. Require the pakset to be consistent. This gives a less robust outcome, but would allow a few more assumptions to be made, as with 1 and 2.
4. Do nothing. This seems like the worst approach to me, as having to deal with inconsistent records of the number of classes is likely to be a disproportionate source of bugs. I've already seen a least one commit that deals with handling this sort of issue, and I wouldn't be surprised if there were other issues that haven't been exposed merely because the paksets have so-far been self-consistent.

I think 2 is probably the best option, although I am viewing this from a perspective of not fully understanding the save/load routines, so I can't say for sure that 1 isn't a better option. Indeed, I find it a bit strange to allow (as far as I can see) the number of classes to be configured in the pakset for all wares, when the existence of separate classes for passengers and mail only is firmly embedded in the code.

jamespetts

Thank you very much for your feedback on this. When I first started coding this feature, the original idea was for number_of_classes to be defined separately in the goods and the individual vehicles, buildings, etc., and for the size of the arrays in each to be consistent with their own sizes; but it came to be apparent that this approach did not entirely work as, as you point out, there needs to be consistency in the number of classes between different vehicles (etc.) when accessing data in many cases.

In terms of your simuconf.tab suggestion, how had you imagined handling the cases where (1) no value is specified in simuconf.tab for the number of classes; and (2) the value specified in simuconf.tab is at variance with the number of classes actually defined in goods.dat?
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

If the value isn't specified, then it can probably default to 1. And in an ideal world, I don't think it should be listed in goods.dat, unless there are plans to allow other goods to have multiple classes.
However, I realise now that the idealistic approach in number 2 won't work because of the need to support pakset versions that list the number of classes in goods.dat. So I now think 1 is the best choice.

jamespetts

Quote from: ACarlotti on February 03, 2018, 08:34:55 PM
If the value isn't specified, then it can probably default to 1. And in an ideal world, I don't think it should be listed in goods.dat, unless there are plans to allow other goods to have multiple classes.
However, I realise now that the idealistic approach in number 2 won't work because of the need to support pakset versions that list the number of classes in goods.dat. So I now think 1 is the best choice.

Yes, I see - the difficulty with no. 2 might also be what happens when the pakset data about the details of the classes in goods.dat (as packed) does not contain all the necessary information.

No. 1 is I think close to what I have been doing more recently (i.e., initialising the class arrays in the actual vehicles, as distinct from the vehicle descriptors, the actual vehicles all being loaded after the pakset has completed loading) to the maximum of the overall number of classes in the pakset and the number of classes in that vehicle's descriptor.
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

Quote from: jamespetts on February 04, 2018, 11:27:35 AM
No. 1 is I think close to what I have been doing more recently (i.e., initialising the class arrays in the actual vehicles, as distinct from the vehicle descriptors, the actual vehicles all being loaded after the pakset has completed loading) to the maximum of the overall number of classes in the pakset and the number of classes in that vehicle's descriptor.

It was in trying to find out why you'd done that that I ended up looking into the load routine.

I shall try to sort out code to make everything consistent at load time - probably by calling methods from successfully_loaded() where needed. Then I won't have to worry about that in fixing the overloading bug.

jamespetts

Quote from: ACarlotti on February 04, 2018, 01:50:36 PM
It was in trying to find out why you'd done that that I ended up looking into the load routine.

I shall try to sort out code to make everything consistent at load time - probably by calling methods from successfully_loaded() where needed. Then I won't have to worry about that in fixing the overloading bug.

That is very 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

Fixes to this now pushed to my branch class_number_consistency.

The functionality is essentially that if information is specified for too many classes, then the attributes of the highest classes are combined (e.g. capacity) or ignored (e.g. comfort). If not enough information is specified, then the remaing data is filled in as if no people/accommodation of that class exists.

Testing has consisted chiefly of recompiling the pakset with the number of passenger and mail classes changed first to 3/2, and then to 6/1, and then loading and saving games and checking that places that included class data appeared to behave correctly. Increasing the set number of classes should have no effect on the game (except that at 254 classes the game runs rather slowly); on the other hand reducing the number of classes and then increasing it again will have lasting effect on savegames (all the existing passengers had their wealth capped, and class reassignments for vehicles would not be restored).

A few additional comments:
The system for determining the class of sent mail from buildings was broken - it effectively had low class passengers sending priority mail, and everyone else sending normal mail. I've changed it to be uniformly random.
I think I've found a few places where you have off-by-one errors in calls to simrand. I've corrected one in this branch, and there's another that I found while working on the other branch I have.
Checking number_of_classes==0 is no longer a valid way to check if a building has class proportions set up (and this led at one point to an infinite loop until I realised what was going wrong). This test is now done by checking whether the sum of the proportions is 0. There was also some code that didn't account for these class proportions being cumulative, which I fixed.

It shouldn't take too much more work now for me to go and finish fixing the bugs I was originally working on.

jamespetts

Splendid, thank you for that: I have now pushed this to the master branch.

I notice that some of the code assumes that only passengers and mail have classes: whilst this is currently true, there are plans to apply the concept of classes to goods (necessitating the use of the class datum for goods of a type other than passengers or mail), albeit in a different way to the way in which it is implemented for passengers and mail. From what I have seen of your code, it looks as though this can be altered in the future without too much difficulty, but the plans to which I refer may well be worth bearing in mind for the future.

Thank you also for fixing the bugs relating to mail generation: that is most helpful. I had not spotted those (and it would be difficult to notice those issues without testing).

Thank you again very much for your work on this: it is 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.

ACarlotti

Ah, yes, I remember reading about goods classes. I took the view here that there were already places in the code where it was assumed that goods only have one class, so it seemed sensible to enforce that for now.

jamespetts

Quote from: ACarlotti on February 06, 2018, 04:56:39 PM
Ah, yes, I remember reading about goods classes. I took the view here that there were already places in the code where it was assumed that goods only have one class, so it seemed sensible to enforce that for now.

Splendid, thank you for confirming. If and when this is implemented, those places will have to be found and modified; this one should at least be easy to find.
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.