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

Added button to renew ride #3473

Merged
merged 9 commits into from
May 23, 2018
Merged

Added button to renew ride #3473

merged 9 commits into from
May 23, 2018

Conversation

jensj12
Copy link
Contributor

@jensj12 jensj12 commented May 2, 2016

This PR adds a button in the maintenance window that renews the ride (https://openrct2.org/forums/topic/719-ride-refurbishment/). I will try to figure out how to calculate the costs properly and update it once implemented. It works now (but for free) and the button should only show up when debugging tools are enabled. Any tips are welcome.

@@ -3396,6 +3398,9 @@ static void window_ride_maintenance_mouseup(rct_window *w, int widgetIndex)
case WIDX_LOCATE_MECHANIC:
window_ride_locate_mechanic(w);
break;
case WIDX_RENEW_RIDE:
game_do_command(0,1,0,w->number,GAME_COMMAND_RENEW_RIDE,0,0);
Copy link
Member

Choose a reason for hiding this comment

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

Needs spaces after commas.

@Gymnasiast
Copy link
Member

Has duplicate code, as 'Renew rides' (in the Cheats window) does exactly the same, but for all rides at once. Could you refactor it so the existing cheat calls your function for every ride? Also, it has to be checked if this unreliability_factor has to be reset as well.

@duncanspumpkin
Copy link
Contributor

I don't see why this has to be a new game command. There is already a ride set param game command

@Gymnasiast
Copy link
Member

@duncanspumpkin Under which class is the 'Force breakdown' command? This feature fall in the same category, I'd say.

@Broxzier
Copy link
Member

Broxzier commented May 2, 2016

I like this feature. It would be nice to take costs into acount, otherwise it's pretty much an alternative way of using the renew rides cheat.

I don't think this feature belongs to debugging, but is something regular users can use as well. Only thing is that it needs to calculate the cost to really make a difference between the cheat and this button.

The button should be disabled for when a ride is not old at all, and has a reliability of 100%. this prevents the player accidentally clicking the button twice, and losing a huge sum of cash.

@@ -4114,6 +4114,7 @@ STR_5805 :{SMALLFONT}{BLACK}If checked, your server will be added to the{NEWL
STR_5807 :{WINDOW_COLOUR_2}Number of rides: {BLACK}{COMMA16}
STR_5808 :{WINDOW_COLOUR_2}Number of shops and stalls: {BLACK}{COMMA16}
STR_5809 :{WINDOW_COLOUR_2}Number of information kiosks and other facilities: {BLACK}{COMMA16}
STR_5810 :{SMALLFONT}{BLACK}Makes the ride brand new.{NEWLINE}Resets ride age and reliability
Copy link
Contributor

Choose a reason for hiding this comment

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

Given you have implemented this like a gameplay feature, I find this far too technical for a user tooltip. It should say something like, refurbishes the ride.

Copy link
Member

Choose a reason for hiding this comment

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

He has implemented it as a debugging tools feature. Though rewording might be needed if we were to follow Broxzier's idea.

@IntelOrca
Copy link
Contributor

I agree with @Broxzier, this is just a cheat at the moment. It should cost money to renew a ride, at least the amount that it costs to build it again.

@jensj12
Copy link
Contributor Author

jensj12 commented May 2, 2016

It is meant to cost money, just couldn't code that yet. Thanks for the help. I'll make it available without debugging tools.
Didn't know about the set_ride_param game command yet, will fix that, as well as the refactor the renew rides cheat. edit: did you mean set_ride_setting? Force breakdown doesn't use a game command.

@Broxzier
Copy link
Member

Broxzier commented May 2, 2016

I can see this being abused for merged rides, if you were to only take the cost of the ride's track into account. And if you were to follow the track and remove and place each track piece again, then what about rides with multiple circuits?

Example

@jensj12
Copy link
Contributor Author

jensj12 commented May 2, 2016

@Broxzier Merged rides are not possible in 'normal' gameplay. If cheat users want to play fair, they can refurbish all rides in the circuit.

unreliabily_factor seems to be a static value that is multiplied by a value ranging from 1 to 3 (depending on the age) when it's used (https://github.com/OpenRCT2/OpenRCT2/blob/develop/src/ride/ride.c#L2196). Resetting ride age and reliability is enough to make the ride as reliable as if it was new. I've not checked if the popularity/statisfaction needs to be reset.

I'm still lost at what game command to use, none of the existing ones (in ride.c) make sense to me.

@IntelOrca
Copy link
Contributor

@Broxzier I think that will have to remain a technical restriction until we create our own save format which allows us to use different sprites for track elements associated with a particular ride ID.

@jensj12
Copy link
Contributor Author

jensj12 commented May 2, 2016

Which game command should I use? 'Force breakdown' doesn't use any, _set_ride_param doesn't exist and _set_ride_status/setting are for open/close and the ride mode / settings tab. Should I create game_command_set_ride_param and make 'force breakdown' use that one too?

@duncanspumpkin
Copy link
Contributor

I was meaning set ride setting but if you think that wouldn't make sense keep as is

@jensj12
Copy link
Contributor Author

jensj12 commented May 4, 2016

The cost calculation is based on the sell price. The sell price of a ride is 70% and refurb price is half of that, so it nicely adds up to 105% (and thus sell + rebuild is slightly cheaper).

@Broxzier
Copy link
Member

Broxzier commented May 4, 2016

There are quite some merge commits in this PR now, make sure to rebase this on develop.

@@ -4117,6 +4117,8 @@ STR_5808 :{WINDOW_COLOUR_2}Number of shops and stalls: {BLACK}{COMMA16}
STR_5809 :{WINDOW_COLOUR_2}Number of information kiosks and other facilities: {BLACK}{COMMA16}
STR_5810 :Disable train length limit
STR_5811 :{SMALLFONT}{BLACK}If checked, you can have up to{NEWLINE}255 cars per train
STR_5812 :{SMALLFONT}{BLACK}Refurbishes the ride
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be a bit unclear so maybe can add:

STR_5812    :{SMALLFONT}{BLACK}Refurbishes the ride{NEWLINE}which sets the reliability back to 100% for a price

Copy link
Contributor

Choose a reason for hiding this comment

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

That's too technical, its better to describe the overall outcome in doing it - like Refurbishes the ride to help attract more guests.

Copy link
Contributor

@Nubbie Nubbie May 4, 2016

Choose a reason for hiding this comment

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

Wouldn't it be better to explain what it does? Especially if it cost money

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to include that it costs something, something like Refurbishes the ride,{NEWLINE}makes it like new at a cost

Copy link
Contributor

Choose a reason for hiding this comment

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

It might as well tell you how much its going to cost if it is to be helpful in any way.

Copy link
Member

Choose a reason for hiding this comment

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

Or display it in a confirmation prompt, and if there is one then the button does not have to be disabled either.

@jensj12
Copy link
Contributor Author

jensj12 commented May 13, 2016

The button now makes use the demolish ride game command instead of it's own (so it also includes some proper checks). I couldn't rename it yet because of some external links I can't edit.
The game pops up a confirmation dialog (stolen from the demolish ride prompt) which shows how much it costs. Only one demolish/refurbish ride prompt can be opened at the same time, it switches nicely if another refurbish/demolish button is clicked.
Unless someone else finds something to improve, I think it's ready to be merged.

@Gymnasiast Gymnasiast added pending review testing required PR needs to be tested before it is merged. and removed pending improvement labels May 13, 2016
@jensj12
Copy link
Contributor Author

jensj12 commented May 20, 2016

Even though the button works as far as I've tested, I'm not sure about the no money scenario's. Using this would be cheating as players can keep using it for free to prevent a lot of breakdowns. Three possible solutions (it should always work when the goal is 'Have fun!' or the goal is already reached):

  1. Add a warning that using the function would be cheating
  2. Only allow it once the ride is at least n years old, show the 'ride doesn't need refurbishing' error if not.
  3. Disable it altogether (like I've done with mazes)

@IntelOrca
Copy link
Contributor

IntelOrca commented May 20, 2016

I'd go with option 2 which is what you do anyway right?

@Gymnasiast
Copy link
Member

You missed some ride types. Crooked Houses never get broken either, and there might be more.

@IntelOrca
Copy link
Contributor

So are we saying that this is only to improve the reliability of the ride and not affect whether guests choose to go on it or not?

@Broxzier
Copy link
Member

ride->build_date = gDateMonthsElapsed;

It resets the ride age, which affects the likeliness of people wanting to ride it.

@jensj12
Copy link
Contributor Author

jensj12 commented May 20, 2016

The problem with mazes is that I can't calculate the cost of them without modifying the ride.
Currently it allows the refurbishment of rides as soon as they have been opened, as it can be helpful to reset the sell price to 100% when (re)building some parts of the coaster.

@IntelOrca
Copy link
Contributor

Now that I think about it more, I don't think it should be this easy to attract guests again to a ride. I think the player should have to build a new ride and kind of like IRL, you sell old rides and build new better ones (probably because you have more money and the money you sold the old ride for). With a better peep AI which we will do at some point we can help distribute the guests around the rides more.

So I think renew ride should only increase reliability to reduce breakdown, and its only available if the ride is maybe 1 or 2 years old... or if reliability is lower than 50% or something.

@Wuzzy2
Copy link
Contributor

Wuzzy2 commented Jul 11, 2016

I support this idea, as long it is really just a more accessible alternative to manually demolishing the ride, then building the exact same track on the spot.
When you play a park for a very long time, your old custom-built roller coasters can seriously bring down your park value, and it is unpleasant to renew them manually (even if I could totally afford it), so usually I decide to keep them. So yeah, this is a good improvement over RCT2.

Does it really cost the same as if you would manually demolish the track (get cash) and then manually build the same track again at the exact same spot (cost cash)?
This is important, I think, because any cost difference would probably divert too much from RCT2 gameplay.
Maybe there are other RCT2 aspects which need to be considered when a new ride is created this way (e.g. resetting of crash statistics).

I have a small suggestion: Display the cost for renewing the ride (build cost of new ride minus sell earnings of old ride) because renewing an entire rollercoaster can sometimes get very expensive.

@mrtnptrs
Copy link
Contributor

mrtnptrs commented Aug 8, 2016

I think this is a really nice addition for now as cheats, but.... is this ready to be merged?

@AaronVanGeffen AaronVanGeffen removed the pending rebase PR needs to be rebased. label Apr 7, 2018
@AaronVanGeffen
Copy link
Member

Unfortunately, we didn't get around to reviewing your PR in time... It has some merge conflicts again. I tried pushing a rebased version, but it looks like the right permissions have not been granted on your repository.

Could you rebase your PR once more? Sorry for the wait.

@Broxzier Broxzier added the network version Network version needs updating - double check before merging! label May 4, 2018
@jensj12
Copy link
Contributor Author

jensj12 commented May 4, 2018

This PR is from before edit permissions were introduced, but you should be able to edit this now. Somehow the rebase is conflicting some things not touched in my PR, so can you push yours instead?

@Broxzier
Copy link
Member

I've just rebased and tested this PR with multiple parks. I've tested it with and without money, different rides, and even ones with merged parts and arbitary ride types. Only merged rides were very cheap, but as stated before those can not be achieved through normal gameplay. Everything else works nicely, and the prices are reasonable, so I reckon this can (finally) get merged.

@Gymnasiast
Copy link
Member

Gymnasiast commented May 15, 2018

Don't merge it yet, want to review it.

@AaronVanGeffen
Copy link
Member

AaronVanGeffen commented May 19, 2018

In general, I'd like to echo @Broxzier in that it seems to work quite well. I'm not too sure about how this is implemented within the RideDemolishAction GameAction, though. It would be better for the refurbishment to be an action of its own, I think. However, I don't want to block the merger because of this either.

Copy link
Member

@Gymnasiast Gymnasiast left a comment

Choose a reason for hiding this comment

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

I'm still not sure about this PR. I'm going to leave it to @AaronVanGeffen and I hope it has been tested thoroughly.

Whatever the outcome, the tabs have to be addressed and the network version raised.

@@ -3910,6 +3910,16 @@ enum {
STR_VIEW_CLIPPING_SELECT_AREA = 6241,
STR_VIEW_CLIPPING_CLEAR_SELECTION = 6242,

STR_REFURBISH_RIDE_TIP = 6243,
Copy link
Member

Choose a reason for hiding this comment

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

Use spaces, not tabs.

@AaronVanGeffen
Copy link
Member

Did another test on multiplayer. State is synced properly, but the confirmation window is not closed automatically when clicking confirm, even though the refurbishment takes place. This does not happen in single player mode.


if (type != TRACK_ELEM_INVERTED_90_DEG_UP_TO_FLAT_QUARTER_LOOP){
Copy link
Member

Choose a reason for hiding this comment

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

This check appears to have been lost, perhaps in a rebase at some point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check is still there, but here. The old ride_get_refund_price function was no longer used (and had a wrong name, should have been named DemolishTracks), so I completely rewrote that function to do what it says and used it.
TLDR: the removed code in ride_get_refund_price has already been moved.


// Using GAME_COMMAND_FLAG_2 for below commands as a HACK to stop fences from being removed
Copy link
Member

Choose a reason for hiding this comment

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

These checks appears to have been lost, perhaps in a rebase at some point?

@AaronVanGeffen
Copy link
Member

AaronVanGeffen commented May 23, 2018

@jensj12 It appears you're right. I was afraid that refunding mazes would be broken by the code removal, but it looks like demolishing them wasn't refunding any money without your code, either.

That removes my main objections to merging this. :)

@AaronVanGeffen AaronVanGeffen merged commit b763a32 into OpenRCT2:develop May 23, 2018
@AaronVanGeffen AaronVanGeffen removed pending review testing required PR needs to be tested before it is merged. labels May 23, 2018
@jensj12 jensj12 deleted the RenewRidesButton branch May 24, 2018 12:30
janisozaur added a commit that referenced this pull request Jun 10, 2018
- Feature: [#1417] Allow saving track designs for flat rides.
- Feature: [#1675] Auto-rotate shops to face footpaths.
- Feature: [#3473] Add button in ride window's maintainance tab to refurbish the ride.
- Feature: [#6510] Ability to select edges or a row of tiles by holding down Ctrl using the land tool.
- Feature: [#7187] Option for early scenario completion.
- Feature: [#7266] Make headless instances use an interactive terminal with access to the in-game console API.
- Feature: [#7267] Leverage more historical data in Finances window.
- Feature: [#7316] Cheat to allow freezing all staff
- Feature: [#7332] Keyboard shortcuts for view path issues and cutaway view.
- Feature: [#7348] Add large half loops to the Vertical Drop Roller Coaster.
- Feature: [#7459] Allow opening and closing of parks that use no money.
- Feature: [#7579] New horizontal +/- spinner widgets to make adjusting values easier.
- Fix: [#2053] When clearance checks are disabled, a track piece ghost can remove non-ghost track pieces.
- Fix: [#2611] Some objects show (undefined string) instead of a description in Korean.
- Fix: [#3596] Saving parks, landscapes and tracks with a period in the filenames don't get their extension.
- Fix: [#5210] Default system dialog not accessible from saving landscape window.
- Fix: [#6134] Scenarios incorrectly categorised when using Polish version of RCT2.
- Fix: [#6141] CSS50.dat is never loaded.
- Fix: [#6647] Changelog window causes FPS drop.
- Fix: [#6938] Banner do not correctly capitalise non-ASCII characters.
- Fix: [#7176] Mechanics sometimes fall down from rides.
- Fix: [#7303] Visual glitch with virtual floor near map edges.
- Fix: [#7313] Loading an invalid path with openrct2 produces results different than expected.
- Fix: [#7327] Abstract scenery and stations don't get fully See-Through when hiding them (original bug).
- Fix: [#7331] Invention list in scenario editor crashes upon removing previously-enabled ride/stall entries.
- Fix: [#7341] Staff may auto-spawn on guests walking outside of paths.
- Fix: [#7354] Cut-away view does not draw tile elements that have been moved down on the list.
- Fix: [#7358] Peeps and staff entering rides still have the slope speed penalty set.
- Fix: [#7382] Opening the mini-map reverts the size of the land tool to 1x1, regardless of what was selected before.
- Fix: [#7402] Edges of neigbouring footpaths stay connected after removing a path that's underneath a ride entrance.
- Fix: [#7405] Rides can be covered by placing scenery underneath them.
- Fix: [#7418] Staff walk off paths with a connection but no adjacent path.
- Fix: [#7434] Diagonal ride segments cannot be deleted if they are isolated.
- Fix: [#7436] Only the first 32 vehicles of a train can be painted.
- Fix: [#7480] Graphs skip values of 0.
- Fix: [#7505] Game crashes when trying to make path over map edge while having clearance checks disabled.
- Fix: [#7528] In park entrance pricing tab, switching tabs happens on mouse-down instead of mouse-up
- Fix: [#7544] Starting a headless server with no arguments causes the game to freeze.
- Fix: [#7571] Hovering a ride design over scenery or tracks will give tons of money.
- Improved: [#2989] Multiplayer window now changes title when tab changes.
- Improved: [#5339] Change eyedropper icon to actual eyedropper and change cursor to crosshair.
- Improved: [#5832] Resize tile inspector automatically when selecting a tile element.
- Improved: [#6221] The scenario editor's invention list is now resizeable.
- Improved: [#7069] The arbitrary ride type selection dropdown is now sorted orthographically, and has its spinners removed.
- Improved: [#7302] Raising land near the map edge makes the affected area smaller instead of showing an 'off edge map' error.
- Improved: [#7435] Object indexing now supports multi-threading.
- Improved: [#7510] Add horizontal clipping to cut-away view options.
- Improved: [#7531] "Save track design" dropdown now stays open.
- Improved: [#7548] Ctrl-clicking with the tile inspector open now directly selects an element and its tile.
- Improved: [#7555] Allow setting the Twitch API URL, allowing custom API servers.
- Improved: [#7567] Improve the performance of loading parks and the title sequence.
- Improved: [#7577] Allow fine-tuning the virtual floor style.
- Improved: [#7608] The vehicle selection dropdown is now sorted orthographically.
- Improved: [#7613] Resizing the staff window now resizes the name and action columns too.
- Improved: [#7627] Allow scrolling up and down on spinners to change their values.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
network version Network version needs updating - double check before merging!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet