Overflow in totalRideValue

[jensj12]

Operating System

Windows 10

OpenRCT2 build

90b1e7c

Base game

RollerCoaster Tycoon 2

Area(s) with this issue?

No response

Describe the issue

In a free entry & pay for rides park, the message 'entry fee too high' can popup. As someone on the forums discovered, this is due to the totalRideValue overflowing here.

Steps to reproduce

There is a park attacked to the forum post.

Attachments

No response

[eaeiv]

Looked into this issue. The forum post does a good job of accurately diagnosing the problem. totalRideValue is probably sized at money16 mega parks like the one provided in the forum post were clearly never intended to be possible. Hell, I can't get it to run above 10 fps on my rig.

Note:
I am unable to reproduce this bug even with the park file provided. I'm new to contributing so I'd be grateful if someone could help me verify my changes.

The Proposed Fix:
I added an AddClamp_money16() to the accumulator in Park::CaclulateTotalRideValueForMoney(). This prevents the overflow but also "gives up" calculating park totalRideValue after INT16_MAX.

money16 Park::CalculateTotalRideValueForMoney() const
{
    money16 totalRideValue = 0;
    bool ridePricesUnlocked = ParkRidePricesUnlocked() && !(gParkFlags & PARK_FLAGS_NO_MONEY);
    for (auto& ride : GetRideManager())
    {
        if (ride.status != RideStatus::Open)
            continue;
        if (ride.lifecycle_flags & RIDE_LIFECYCLE_BROKEN_DOWN)
            continue;
        if (ride.lifecycle_flags & RIDE_LIFECYCLE_CRASHED)
            continue;

        // Add ride value
        if (ride.value != RIDE_VALUE_UNDEFINED)
        {
            money16 rideValue = static_cast<money16>(ride.value);
            if (ridePricesUnlocked)
            {
                rideValue -= ride.price[0];
            }
            if (rideValue > 0)
            {
                // totalRideValue = += rideValue * 2; // old
                totalRideValue = AddClamp_money16(totalRideValue, rideValue * 2); // new
            }
        }
    }
    return totalRideValue;
}

Other Solutions.
I toyed around with just expanding totalRideValue to a money64, but this causes all sorts of conflicts with the s4 and s6 savefile structs which I dare not modify. However, the value could be calculated and stored during execution as a money64 but truncated to a money16 when saving. Though that feels rather hacky...

[ZehMatt]

You can definitely change the type to money64 except for s4 and s6 import, as for park export you can declare a new save version and continue storing it as money64, we no longer export s6 parks and never exported s4, so it should be safe to do.

[eaeiv]

Then I have most of the changes ready to go.
But, how would I go about declaring a new save version?
The wiki's info on it is a little light.

[ZehMatt]

Have a look at the park importer code, there might be already some case where it checks the version on how to read a field.