Cause ride breakdowns

[GoogleCodeExporter]

This patch causes rides to break down after an interval drawn from an 
exponential distribution, based on the ride's reliability. (The patch sets 
every ride's mean breakdown interval to 182 days, or ~2 times/yr. Perhaps there 
is some way to read reliability from RCD files?) Each ride has a grace period 
after it is opened during which it will not break at all. This is currently set 
to 30 days.

However, the patch only adds limited support for breakdowns; for instance, 
since I haven't implemented mechanics yet, the rides can never be fixed :( 
Fortunately, being broken down doesn't affect the visual behaviour of the ride, 
as far as I can tell. These features will be coming in subsequent patches.

It was tested against r1272.

Original issue reported on code.google.com by stanekj...@gmail.com on 19 Aug 2014 at 2:31

Attachments:

• breakdown.patch

[GoogleCodeExporter]

Some comments, as they occurred to me (sorry for the haphazard order!) :

- breakdown grace period is unnecessary, imo. RCT never had it, and real life 
doesn't either...

- age of ride in days should be fine

- some spelling errors

- the Exponential function looks a bit odd to me.. certainly, instead of lots 
of casting, you could just divide by "10000.0".
- casting to a uint32 while returning a uint16? looks wrong
- some explanation of the 10000 value?

- "for (auto instance : this->instances)" loop for the instances OnNewDay

- Not sure about keeping Random as a member variable - all randomness is static 
data anyway (i know Guests do it that way, but perhaps it shouldn't...)

Original comment by CharlesP...@googlemail.com on 19 Aug 2014 at 4:48

[GoogleCodeExporter]

For exponential distribution, use double u = 
this->DrawNumber()*(1.0/4294967296.0);
This will be a [0,1)  interval.

for exponential distribution do double value = -lambda * log(1-u)

lambda is the average value of the distribution (ie 1/rate). This number is 
nicer to work with when you have low rates. I used "1-u", since that cannot 
become exactly 0 (and thus not crash on log(0) ).

The breakdown magic values look like they need a separate enum value for 
tracking the breakdown state (instead of trying to merge it all in the counter 
with negative values that distribute to everywhere). I suspect that will also 
become useful in the future when you have to ask for a handy man.

The breakdown grace period could perhaps also be implemented by adding the 
grace period after drawing a value for the first time.

Why do you want to stop age counting rides when they are not open?  Maybe split 
age measuring away from breakdowns to a separate patch? (smaller patches are 
easier to handle, in general).

Original comment by Alberth2...@gmail.com on 19 Aug 2014 at 6:05

[GoogleCodeExporter]

Here is the updated version of the patch. I addressed most of the issues you 
both raised. I removed aging from this patch and added a new enum for breakdown 
state, and also cleaned up Random::Exponential() as suggested.

Original comment by stanekj...@gmail.com on 19 Aug 2014 at 7:28

Attachments:

• new-bd.patch

[GoogleCodeExporter]

Hi,

Your new patch seems to describe a change from the previous patch to this one. 
Could you make a patch against a game revision instead? That's much easier to 
read and apply :)

I like that you make your patch filenames unique. It prevents a lot of 
confusion. If you ever get tired of inventing a new name every time, you can 
try use a more systematic way of naming them, like breakdown_V2_r1234.patch. 
(Just a friendly suggestion; I like that you give each patch file a new name, 
in what way you derive a name isn't that interesting for me.)

In this patch you seem to be confused about comments; we use the convention 
that lines with just comment use /* .. */ style (these are often longer blocks 
of comment spanning several lines, so block comments are more convenient). 
Comments at the end of the line are // style (as it saves typing).

I wondered where 182 came from (even with your comment for some reason), until 
I realized you meant 365 / 2 . I usually keep such constant calculations, as it 
describes better what value is intended. The compiler will most likely rewrite 
it to a single number so there is no performance penalty.

"RideInstance#breakdown_ctr" you're used to java doc?
In Doxygen, you write RideInstance::breakdown_ctr instead (ie just what C++ 
itself does), maybe your style is accepted too, but I could not quickly find 
your syntax in the documentation.
Inside a class you can write "#foo" to refer to class members.

"bool has_been_opened;" is not being initialized nor is it being used in this 
patch.

"bds" is a bit short, you'd never guess what it means from the name. I'd prefer 
something longer, like "breakdown_state", or "brkdown_state" or so? (I'd be 
temped to use the latter, although the former is probably better in the long 
run.)

You can simplify the random generator call by switching from rate to average as 
parameter. You do now

this->rnd.Exponential(1.0 / (double) this->reliability);
...
(uint16) (-log(1 - u) / lambda)

ie you compute (uint16) (-log(1 - u) / (1.0 / (double) this->reliability) ).
if you throw out both divisions and replace it with multiplication (a/(1/b) == 
a * b), you get

this->rnd.Exponential(this->reliability);
...
-lambda * log(1 - u)

which looks a little more clean.

Maybe you want to add a few /// \todo  lines to define what code if further 
expected at some point in the future? In this patch it's quite clear what 
belongs together, but in the code, your breakdown code will not be that easily 
visible, so it may be difficult to find the right point to add further 
functionality.

Original comment by Alberth2...@gmail.com on 21 Aug 2014 at 8:38

[GoogleCodeExporter]

Typo in last lines: "if further expected" should be "is further expected".

Original comment by Alberth2...@gmail.com on 21 Aug 2014 at 8:41

[GoogleCodeExporter]

Thanks for your patience. This is my first time working with a significant 
codebase and I'm still a bit new to C++. 

The attached patch should fix all the listed problems.

I have one tangential question, though: What would be the best way to assign a 
different reliability to different ride types in the future? Would it be stored 
in RCD files? (Also, does any documentation exist on RCD files?) Or would it be 
stored elsewhere?

Original comment by stanekj...@gmail.com on 21 Aug 2014 at 11:25

Attachments:

• breakdown_V3_r1272.patch

[GoogleCodeExporter]

Sorry, the previous patch is incorrect. I wrote the Exponential function 
incorrectly, so it only outputs negative values. The attatched patch has been 
tested and works as expected.

Original comment by stanekj...@gmail.com on 22 Aug 2014 at 4:12

Attachments:

• breakdown_V4_r1272.patch

[GoogleCodeExporter]

I have thrown the patch in trunk, r1278
I made some small modifications, the most important one is removal of 
RIS_BROKEN.

There is code that tests for RIS_OPEN to decide whether the ride is open, and 
if you add another semi-open value, that code will break (since it will not 
consider the ride open). Trying to add a new value is ok, but I'd expect more 
changes than just adding an enum value then. (Unless I am wrong here, please 
prove me wrong in that case.)

I am not sure all code is at exactly the right spot, but that will become clear 
in time, hopefully. For example, by adding breakdown to RideInstance, all ride 
types have breakdown now, including eg shops.

As for your reliability question, yes, RCD file would be the place to add it. I 
think you want a reliability life cycle, that is a reliability throughout time, 
for example since first opening of the ride.
doc/data_format.rst is the RCD block documentation file.
src/rcdgen is the generator/compiler. It takes the graphics/rcd/*.txt files, 
processes them, and generates *.rcd files in the format described by the 
documentation.
You want to study rcdgen/checkdata.cpp, and rcdgen/nodes.* of rcdgen, if you 
want to understand it enough to add a few fields.

Original comment by Alberth2...@gmail.com on 22 Aug 2014 at 9:19