Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ride groups #5504

Merged
merged 10 commits into from Jun 14, 2017
Merged

Ride groups #5504

merged 10 commits into from Jun 14, 2017

Conversation

Gymnasiast
Copy link
Member

@Gymnasiast Gymnasiast commented May 24, 2017

Fixes #4055, #4054, #5515 and #5400.

Currently, when using select-by-track-type, everything is merged. There are four cases where it makes more sense to split ride types again, although not in the same way RCT2 did. This PR will split the Junior RC, Corkscrew RC, Steel Twister and Car Ride into two groups each.

This PR is a work in progress. The following work still has to be carried out:

Besides the ride groups, this PR also cleans up a handful of things along the way:

  • It will no longer show track elements that the train is not capable of using
  • The maximum height is now taken from the ride group and not from the vehicle (unless the user has turned select-by-track-type off, in which case it uses RCT2 behaviour)
  • The descriptions now fit better. For example, the Steel Twister will no longer carry the description of the Hyper-Twister.
  • The new ride window is cleaned up a bit and has more comments
  • When in select-by-track-type mode, the loaded objects are no longer modified to support all track types. Instead, the supported elements are determined when starting construction.
  • The Hyper-Twister will now always be properly displayed as such, even when in non-select-by-track-type mode.
  • ride_create has been refactored to split off the naming off and eliminate some gotos.
  • A function has been created to fetch the name for a ride, avoiding lots of ifs elsewhere
  • Some string additions have been eliminated and replaced with the above function or with RideNaming[type].

Basically, this aims to iron out the oddities of the existing implementation, and make it behave more like RCT1 while being a bit more user-friendly (in that it will prevent building a roller coaster that contains elements the vehicle does not support).

@janisozaur
Copy link
Member

Remember to add changelog entry

@Gymnasiast
Copy link
Member Author

@IntelOrca Would you be able to help me with points 1 and 2?

@IntelOrca
Copy link
Contributor

yes

@@ -17,6 +17,7 @@
#pragma once

#include "../common.h"
#include "ride_group.h"
Copy link
Contributor

@IntelOrca IntelOrca May 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be wrapped in extern C, as this is a C++ header file. You can avoid the include altogether by forward declaring ride_group.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#ifdef __cplusplus
extern "C"
{
#endif
    #include "ride_group.h"
#ifdef __cplusplus
}
#endif

appears to have done the trick. Thanks!

@Gymnasiast
Copy link
Member Author

I am currently at a point where everything works, so that leaves refactoring, fixing CI and optionally moving to C++.


ride_group ride_group_hypercoaster = {
.track_type = RIDE_TYPE_CORKSCREW_ROLLER_COASTER,
.maximum_height = 28,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maximum height needs to be fixed here.

_legacyType.ride_type[i] == RIDE_TYPE_TWISTER_ROLLER_COASTER ||
_legacyType.ride_type[i] == RIDE_TYPE_VERTICAL_DROP_ROLLER_COASTER ||
_legacyType.ride_type[i] == RIDE_TYPE_GIGA_COASTER ||
_legacyType.ride_type[i] == RIDE_TYPE_JUNIOR_ROLLER_COASTER)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think here we can make a function in rct2.c / rct2.h, given a ride type says whether it should be given boosters. That would then remove the repetition of _legacyType.ride_type[i].

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving it to a function is good, but it would be more consistent to have in RideObject.cpp, since that's also where PerformRCT1CompatibilityFixes resides. (I also think it's a better location in general.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So for RCT1 we have rct1/tables.cpp which contains these kinds of functions. I was thinking we also need one for RCT2.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rct1/tables.cpp contains code to convert S4 files (as well as some stuff that doesn't belong there, like some select-by-track-type data, that should go in ride_data IMO).

This code, along with PerformRCT1CompatibilityFixes, isn't really about converting S4 or S6 files, but about making sure .DAT files play nicely. Therefore I think it should stay in the RideObject.cpp file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just think it would be good to separate anything to do with RCT1 or RCT2 into the RCT1/2 sub directories - which would be helpful if we ever move them out to its own library.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not directly have to do with RCT1 and RCT2. It does have to do with .DAT files, which RideObject.cpp handles.


// Find the first non-null ride type, to be used when checking the ride group
uint8 rideTypeIndex = 0;
while (_legacyType.ride_type[rideTypeIndex] == RIDE_TYPE_NULL && rideTypeIndex < 3)
Copy link
Contributor

@IntelOrca IntelOrca May 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to move the rideTypeIndex < 3 check to the beginning of the expression, otherwise it will access element 3 (out of range) of ride_type still.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has now been addressed.


ride_group ride_group_hypercoaster = {
.track_type = RIDE_TYPE_CORKSCREW_ROLLER_COASTER,
.maximum_height = 28,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix height

@janisozaur
Copy link
Member

The "Fix Xcode project 3df6bb5" commit looks overly large, can you check what's going on in there?

@Gymnasiast
Copy link
Member Author

@janisozaur It cancels out a rebase error. If you look at the diff of all the commits you will see it ends up adding just the ride group files to the Xcode project, as intended.

@Gymnasiast
Copy link
Member Author

For some reason, two of the Travis tests fail:

/home/travis/build/OpenRCT2/OpenRCT2/src/openrct2/ride/../object/RideObject.h:29:51: error: could not convert '0' from 'int' to 'rct_ride_entry::<anonymous union>'

     rct_ride_entry              _legacyType = { 0 };
                                                   ^

I did not touch this bit, so I'm not sure why it fails and how to fix it.
@IntelOrca @janisozaur

@janisozaur
Copy link
Member

can you try changing it to just {}?

rct_string_id name;
rct_string_id description;
} rct_ride_name;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't size of this struct be validated too?

Copy link
Member

@janisozaur janisozaur Jun 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not, then it should be moved out of #pragma pack region

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could add that, yes. Is the size to check 4 bytes?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is 4 bytes, but the code was moved in from a non-packed area to a packed one, so it would be better to keep it outside. You can check if the places where rct_ride_name is used put any constraints on size or layout of it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless its used in an s4 or s6 struct, or inside an RCT2 interop define block, it does not need user defined packing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is used in rct_ride_entry.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typedef struct rct_ride_entry {
I can't see where

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

size vaildation is still mising


bool ride_groups_are_equal(ride_group * a, ride_group * b)
{
if (a->naming.name == b->naming.name && a->naming.description == b->naming.description)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just return (a->naming.name == b->naming.name && a->naming.description == b->naming.description)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this notation easier to read.

@rwjuk
Copy link
Contributor

rwjuk commented Jun 7, 2017

Found a bug. Sorry, my mistake, I hadn't seen that this PR doesn't touch the wild mouse ride at all - I suppose my question is now instead "can ride groups fix this as well, or is this a separate issue?".

  1. Load Forest Frontiers.
  2. Edit the invention list so that the Wooden Wild Mouse is invented.
  3. Build Mini Miner.
  4. Observe that the vehicle is set to Wooden Wild Mine Ride, but that option isn't selectable in the drop-down (the inverse happens if you invent the mine ride, and build a 'mouse' track).

ridegroupsbug

I imagine it should build the ride with the mouse cars, but give you the RCT1-style 'ride will be built with different vehicle type' warning in the track list?

Here's a save demonstrating this Forest Frontiers_bug.zip

@rwjuk rwjuk added the pending rebase PR needs to be rebased. label Jun 8, 2017
@Gymnasiast
Copy link
Member Author

@rwjuk Is this reproducible on develop?

@rwjuk
Copy link
Contributor

rwjuk commented Jun 8, 2017

@Gymnasiast Yes, it is.

@Gymnasiast Gymnasiast removed the pending rebase PR needs to be rebased. label Jun 8, 2017
@Gymnasiast
Copy link
Member Author

I addressed the bug @rwjuk reported here, as well as another one he reported in chat. It should now work properly.

Could you take a look at the code, @IntelOrca ? If you're OK with the implementation it can be merged AFAIC.

@rwjuk
Copy link
Contributor

rwjuk commented Jun 13, 2017

Both bugs I identified are fixed as of the latest commit.

@rwjuk
Copy link
Contributor

rwjuk commented Jun 13, 2017

I think I've found a bug.

  1. Add the School Bus Ride and San Francisco Tram to a park
  2. Build a Miniature Railway track
  3. Set the vehicle type to 'School Bus'
  4. Set the vehicle type (back) to 'San Francisco Tram'
  5. See below - '0 cars in train' and no visual.

capture

#include "track_data.h"
}

class RideGroup final : public IRideGroup
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be called RideGroupManager or RideGroupService.

ride_group corkscrew_rc_groups[MAX_RIDE_GROUPS_PER_RIDE_TYPE] = { ride_group_corkscrew_rc, ride_group_hypercoaster };
ride_group junior_rc_groups[MAX_RIDE_GROUPS_PER_RIDE_TYPE] = { ride_group_junior_rc, ride_group_midi_coaster };
ride_group car_ride_groups[MAX_RIDE_GROUPS_PER_RIDE_TYPE] = { ride_group_car_ride, ride_group_monster_trucks };
ride_group twister_rc_groups[MAX_RIDE_GROUPS_PER_RIDE_TYPE] = { ride_group_steel_twister_rc, ride_group_hyper_twister };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anything that is only used inside the class can be made private.

ride_group car_ride_groups[MAX_RIDE_GROUPS_PER_RIDE_TYPE] = { ride_group_car_ride, ride_group_monster_trucks };
ride_group twister_rc_groups[MAX_RIDE_GROUPS_PER_RIDE_TYPE] = { ride_group_steel_twister_rc, ride_group_hyper_twister };

ride_group * GetRideGroup(uint8 trackType, rct_ride_entry * rideEntry) const override
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this return a const ride_group *?

return rideGroup;
}

bool RideGroupsAreEqual(ride_group * a, ride_group * b) const override
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const ride_group *

return false;
}

bool RideGroupIsInvented(ride_group * rideGroup) const override
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const ride_group *

};
struct
{
uint16 NumThemeObjects;
rct_object_entry * ThemeObjects;
uint8 Pad;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary

@@ -181,18 +182,34 @@ void research_finish_item(sint32 entryIndex)

if (entryIndex >= 0x10000) {
// Ride
rct_string_id availabilityString;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this variable can be sunk to the block below

availabilityString = STR_NEWS_ITEM_RESEARCH_NEW_RIDE_AVAILABLE;

set_format_arg(0, rct_string_id, ((rideEntry->flags & RIDE_ENTRY_FLAG_SEPARATE_RIDE_NAME)) ?
rideEntry->name : get_friendly_track_type_name(base_ride_type, rideEntry)); // Makes sure the correct track name is displayed,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this inline comment above the call? I understand it concerns the condition, but also makes this line overly long. If needed, you can extract the condition result to another variable.

bool ride_group_is_invented(ride_group * rideGroup);
#ifdef __cplusplus
}
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing newline

size_t GetCountForRideGroup(uint8 rideType, ride_group * rideGroup) const override
{
size_t count = 0;
IObjectRepository * repo = GetObjectRepository();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const IObjectRepository *

size_t count = 0;
IObjectRepository * repo = GetObjectRepository();

for (const auto item : _items)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const auto &item

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't do that in the other methods.

size_t GetItemsForRideGroup(track_design_file_ref **outRefs, uint8 rideType, ride_group * rideGroup) const override
{
std::vector<track_design_file_ref> refs;
IObjectRepository * repo = GetObjectRepository();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't do that in the other methods.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you fix them too?

std::vector<track_design_file_ref> refs;
IObjectRepository * repo = GetObjectRepository();

for (const auto item : _items)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

&

@@ -424,12 +481,24 @@ extern "C"
return repo->GetCountForObjectEntry(rideType, String::ToStd(entry));
}

size_t track_repository_get_count_for_ride_group(uint8 rideType, ride_group * rideGroup)
{
ITrackDesignRepository * repo = GetTrackDesignRepository();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const, and in functions below too

@Gymnasiast
Copy link
Member Author

@rwjuk I tested it with develop, and it also happens there, so it's not introduced by this PR. Could you create an issue for it?

@rwjuk
Copy link
Contributor

rwjuk commented Jun 13, 2017

@Gymnasiast Whoops - I thought I'd checked and it didn't happen there, but I must have been thinking of something else. Either way... #5609

Copy link
Member

@janisozaur janisozaur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only looked at the code and didn't find any obvious bugs other than few issues already fixed – so LGTM.

This could use many, many more const modifiers, but we haven't given anyone hard time over that in the past, so I don't see reason to start now.

_legacyType.ride_type[i] == RIDE_TYPE_VERTICAL_DROP_ROLLER_COASTER ||
_legacyType.ride_type[i] == RIDE_TYPE_GIGA_COASTER ||
_legacyType.ride_type[i] == RIDE_TYPE_JUNIOR_ROLLER_COASTER
) {
Copy link
Contributor

@IntelOrca IntelOrca Jun 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this would be moved into a new function, even if in the same file?
if (RideTypeHasBoosters(_legacyType.ride_type[i]))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unrelated, and I don't like snowballing this PR even further, even though I agree otherwise. I have created issue #5613 for it.

{
if (ride_groups_are_equal(irg, rideGroup))
{
rideGroupIndex = i;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it break after finding a matching ride group?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should, indeed.

continue;
}

const ObjectRepositoryItem * ori = repo->FindObject(item.ObjectEntry.c_str());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if ori is null?

@Gymnasiast Gymnasiast merged commit 96a7a8a into OpenRCT2:develop Jun 14, 2017
@Gymnasiast Gymnasiast deleted the feature/ride-groups branch June 14, 2017 19:14
janisozaur added a commit that referenced this pull request Jul 12, 2017
- Feature: [#1399 (partial), #5177] Add window that displays any missing/corrupt objects when loading a park
- Feature: [#5056] Add cheat to own all land.
- Feature: [#5133] Add option to display guest expenditure (as seen in RCTC).
- Feature: [#5196] Add cheat to disable ride ageing.
- Feature: [#5504] Group vehicles into ride groups
- Feature: [#5576] Add a persistent 'display real names of guests' setting.
- Feature: [#5611] Add support for Android
- Feature: [#5706] Add support for OpenBSD
- Feature: OpenRCT2 now starts up on the display it was last shown on.
- Feature: Park entrance fee can now be set to amounts up to £200.
- Improved: Construction rights can now be placed on park entrances.
- Improved: Mouse can now be dragged to select scenery when saving track designs
- Fix: [#259] Money making glitch involving swamps (original bug)
- Fix: [#441] Construction rights over entrance path erased (original bug)
- Fix: [#578] Ride ghosts show up in ride list during construction (original bug)
- Fix: [#597] 'Finish 5 roller coasters' goal not properly checked (original bug)
- Fix: [#667] Incorrect banner limit calculation (original bug)
- Fix: [#739] Crocodile Ride (Log Flume) never allows more than five boats (original bug)
- Fix: [#837] Can't move windows on title screen to where the toolbar would be (original bug)
- Fix: [#1705] Time Twister's Medieval entrance has incorrect scrolling (original bug)
- Fix: [#3178, #5456] Paths with non-ASCII characters not handled properly on macOS.
- Fix: [#3346] Crash when extra long train breaks down at the back
- Fix: [#3479] Building in pause mode creates too many floating numbers, crashing the game
- Fix: [#3565] Multiplayer server crash
- Fix: [#3681] Steel Twister rollercoaster always shows all track designs
- Fix: [#3846, #5749] Crash when testing coaster with a diagonal lift in block brake mode
- Fix: [#4054] Sorting rides by track type: Misleading research messages
- Fix: [#4055] Sort rides by track type: Sorting rule is not really clear (inconsistent?)
- Fix: [#4512] Invisible map edge tiles corrupted
- Fix: [#5009] Ride rating calculations can overflow
- Fix: [#5253] RCT1 park value conversion factor too high
- Fix: [#5400] New Ride window does not focus properly on newly invented ride.
- Fix: [#5489] Sprite index crash for car view on car ride.
- Fix: [#5730] Unable to uncheck 'No money' in the Scenario Editor.
- Fix: [#5750] Game freezes when ride queue linked list is corrupted.
- Fix: [#5819] Vertical multi-dimension coaster tunnels drawn incorrectly
- Fix: Non-invented vehicles can be used via track designs in select-by-track-type mode.
- Fix: Track components added by OpenRCT2 are now usable in older scenarios.
- Technical: [#5047] Add ride ratings tests
- Technical: [#5458] Begin offering headless build with reduced compile- and run-time dependencies
- Technical: [#5755] Title sequence wait periods use milliseconds
- Technical: Fix many desync sources
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing required PR needs to be tested before it is merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants