Bad numbers for p****enger revenues, at least in the 'goods window'

Topic: Bad numbers for p****enger revenues, at least in the 'goods window' (Read 3872 times) previous topic - next topic

Bad numbers for p****enger revenues, at least in the 'goods window'

March 21, 2010, 09:58:31 pm

Running pak128.Britain (either the current version for Standard *or* 0.4 for Experimental) with simutrans-experimental version 7.1, there's some really odd behavior in the 'goods list' window regarding p****enger revenues. I'm suspicious that this reflects really odd (miscomputed) p****enger revenue behavior!

At a distance setting of 1, everything is completely insensitive to speedbonus.
Now, in 1800 or 1830, I tried setting distance to 16, and comfort to 50.
At a distance setting of 16, keeping comfort constant, *slower trips generate more revenue*. According to the goods list.

This is clearly wrong. I am unsure whether it is simply a bug in the computation for the goods list, or a bug in the actual revenue computation, but either way it's wrong.

Computations appear to give numbers moving the correct direction for significantly longer distances and significantly higher speeds. They also seem to be working OK for goods.

I think there's a negative sign creeping in where there shouldn't be. This is probably due to either a missing absolute value operator in a computation, or roundoff error, or integer overflow. I'd have to know what section of the code to look at to find out what's going wrong.

Note that this could account for some the problems people have encountered with p****enger revenue in the early game. If you get more revenue by running slower vehicles, that's very counterintuitive and renders the faster (and more expensive) ones unusable.

Re: Bad numbers for p****enger revenues, at least in the 'goods window'

Reply #1 – March 22, 2010, 12:06:47 am

Neroden,

thank you very much for the feedback. It is not necessarily the case that increasing the journey distance can lower the revenue at certain settings, as the effect of a relatively low comfort setting of 50 combined with a sufficiently slow speed to make the journey fall above the time threshold at which comfort level 50 is tolerable might outweigh the linear increase in base revenue per unit of distance.

However, I am far from infallible and there may indeed be a bug or twelve in the code somewhere. If you want to have a look at the code to see for yourself, the method in question is convoi_t::calc_revenue(ware_t& ware) in simconvoi.cc.

The difference between p****engers and goods might be accountable for by the fact that it is only for p****engers that comfort is a factor. Thank you again for your investigative work! Apologies that the development is going slowly at present - I have less time than I formerly had, and the merging in of some changes into Simutrans-Standard over the Christmas period in readiness for playing over a network have made some major structural changes to the code that takes a lot of intensive work to accommodate. Thank you very much for your patience and support!

Re: Bad numbers for p****enger revenues, at least in the 'goods window'

Reply #2 – March 22, 2010, 01:19:25 am

Quote from: jamespetts – on March 22, 2010, 12:06:47 am

Neroden,

thank you very much for the feedback. It is not necessarily the case that increasing the journey distance can lower the revenue at certain settings, as the effect of a relatively low comfort setting of 50 combined with a sufficiently slow speed to make the journey fall above the time threshold at which comfort level 50 is tolerable might outweigh the linear increase in base revenue per unit of distance.

I tried it with a comfort setting of 100 too. I'm also getting a nearly linear decrease in predicted revenue with increased speed, not a sudden dropoff. Which means most likely a negative sign is in there somewhere.

Quote

However, I am far from infallible and there may indeed be a bug or twelve in the code somewhere. If you want to have a look at the code to see for yourself, the method in question is convoi_t::calc_revenue(ware_t& ware) in simconvoi.cc.

Is this routine actually used by the 'goods list' window? I'm suspicious that the bug may only be in the goods list window and not in actual revenue, and I should trace that first.

(edit) I'm unfortunately discovering a lot of duplicated code. :-( Both gui/goods_stats_t.cc and gui/goods_frame_t.h perform duplicate computations, all of which are in fact rather cryptic. The codebase needs some routines broken out, specifically relating to time and distance....

(edit) Definitely related to the comfort code. If I max out comfort the pricing stabilizes.

(edit) Take a look at this line at the top of the comfort code in gui/goods_stats_t.cc:

const uint16 journey_minutes = (((float)distance / (float)welt->get_average_speed(way_type)*
bonus)/100) * welt->get_einstellungen()->get_distance_per_tile() * 60;

Are you sure this is parenthesized correctly?
distance/average speed should give a number of hours.
this is then multiplied by bonus/100. Higher bonus -> more hours, lower bonus -> fewer hours.

I think this is the problem.

I also think this code is duplicated in the sorting code in gui/goods_frame_t.cc.

I'm quite sure that the computations should not be duplicated both here and in simconvoi.cc. My suggestion? If possible, set up a "virtual convoy", an instance of convoi_t which doesn't represent a convoy but represents the abstract convoy used by the goods window. Alternatively, if there are serious problems with that, break out revenue computation into an abstract mix-in cl**** inherited by both simconvoi.cc and gui/goods_stats_t.cc.

Also, have goods_frame_t.cc use the data from goods_stats_t.cc via an accessor for sorting, instead of computing it fresh. (This might require restructuring the relationship between the two, to make sure there's one instance of goods_stats_t.cc for each good, but would simplify goods_frame_t.cc immensely.)

As a side note, the endless unit conversions for distance, time, and speed are really way too hairy, and need badly to be broken out as macros or static inlines and named. (No more of these arbitrary multiplications by 60 and 0.3 and other magic numbers.) But it should probably be done in standard as well, which is just as hairy.

Re: Bad numbers for p****enger revenues, at least in the 'goods window'

Reply #3 – March 22, 2010, 11:19:27 pm

Neroden,

thank you very much for your investigations! I have changed the offending line of code to:

Code: [Select]

const uint16 journey_minutes = ((float)distance / (((float)welt->get_average_speed(way_type) * bonus) / 100)) * welt->get_einstellungen()->get_distance_per_tile() * 60;

That fix will be applied in the next release. As to the structural changes that you suggest - there is much sense in making all the code less "hairy" (but I cannot practically unilaterally change Simutrans-Standard code just to make it less "hairy" if I do not intend a function change, as the advantages of the cleaner code would be outweighed by the disadvantages of having to deal with merge conflicts when Simutrans-Standard is updated), but my time is very limited, and I have spent a rather long time trying to get the automatic convoy replacer to work properly with the new network code and add a few features to it while I'm about it, and I still have a long way to go. If you feel like helping, however, it would be very much appreciated! Are you able to get hold of the code via Github and compile it yourself?

Thank you again for the most helpful bug report.

Re: Bad numbers for p****enger revenues, at least in the 'goods window'

Reply #4 – March 23, 2010, 12:08:58 am

Quote from: jamespetts – on March 22, 2010, 11:19:27 pm

As to the structural changes that you suggest - there is much sense in making all the code less "hairy" (but I cannot practically unilaterally change Simutrans-Standard code just to make it less "hairy" if I do not intend a function change, as the advantages of the cleaner code would be outweighed by the disadvantages of having to deal with merge conflicts when Simutrans-Standard is updated), but my time is very limited,

I wasn't suggesting that you do it! :-) Thinking out loud here.

Quote

Are you able to get hold of the code via Github and compile it yourself?

Yeah, though I have trouble with git; I never quite figured it out. So any patches would be in plain old diff/patch format.

I think my next coding project will be to abstract out those time conversions and so forth in standard. Unfortunately, I have no idea when I'll have time to do my next coding project (I've been deeply enmeshed in family medical problems for years now).

Re: Bad numbers for p****enger revenues, at least in the 'goods window'

Reply #5 – March 24, 2010, 12:00:21 am

Neroden,

patches in .diff format are still useful, although Git isn't too hard to master once you get used to it. Have you looked up the tutorials? Any patches (in either format) would be most welcome!