OpenRCT2
Refactor Ferris Wheel code
Part of an effort to make the Ferris Wheel logic better. Should have zero impact on anything but I've updated network just in case. ~~The messy commit history was a Github Desktop goof up since it gets confused when squashing sometimes, I accidentally forgot to change a != with a == in 3f8414c19ae9cf3fd18df182fd4cc40db1096725 but did fix it later in e8875c49fccf0a324ebc2c1350685bda9c892802.~~
So, I let CI run for 313c80e23164e54d9a97d98b799eeafe958de961 in my fork and all was good, so when I updated the network version I skipped CI since I had already confirmed everything was OK. When I created this PR, I thought it would at least run CI for the aforementioned commit, but it's only taking in account the last one I made (Update NetworkBase.cpp). I'll try to amend the last commit to see if I can force it to run, I apologize for the mistake.
I disagree with most of this, I don’t think the ternary conditionals improve legibility (quite the opposite) and neither does removing the early return.
No objections on ternary but definitely on the early returns. Also why the network bump? The logic should remain the same and if the logic did not change then there is no need to raise that version, it is only raised it when there is a clear deviation in the simulation which would cause a desync if two clients were running different versions.
Thanks for the feedback. So, this is actually split up from a bigger branch I have that documents the Ferris Wheel code and gives more readable names for the vars. Since it was a quite big and hard to understand diff, I thought it would be better to separate the refactor and documenting parts into two branches for upstream. The condition if (ferris_wheel_var_0 == -8) is actually checking if the Ferris Wheel has slowed down enough for guests to get on/off, in which case it updates the ride's substate (which for the Ferris actually indicates the last animation frame it stopped at for guests) and sets the state to Arriving. In the documentation branch I felt it was easier to grasp without the early return, but I will take out this change from this branch.
The ternary attributions are simply incrementing/decrementing the image index of the Ferris Wheel animation.
The network version was also updated because even though it should have zero effect on anything, I thought it would be better to be safe since this is code that dictates how a ride behaves.
The network version was also updated because even though it should have zero effect on anything, I thought it would be better to be safe since this is code that dictates how a ride behaves.
This does not justify it, it either works exactly the same way or it does not, if the behavior changed then we do want to know about that. The implication of raising the network version is that servers also have to update so that clients with a newer version can join which leaves out clients that have to yet update, unless there is a clear deviation in the logic don't touch that number.
Alright, but I had already done that in https://github.com/OpenRCT2/OpenRCT2/pull/25391 and didn't receive any objections which is why I thought it was OK.
The implication of raising the network version is that servers also have to update so that clients with a newer version can join which leaves out clients that have to yet update, unless there is a clear deviation in the logic don't touch that number.
I'm aware, I believe it was you that explained that to me on Discord some time ago.
Alright, but I had already done that in #25391 and didn't receive any objections which is why I thought it was OK.
This should not have happened, looks like an oversight. It isn't a breaking change but it is rather unpleasant for the community to keep up with this.
it is rather unpleasant for the community to keep up with this.
You mean for the players who have to update so they can play online, right? Or for the developers?...
it is rather unpleasant for the community to keep up with this.
You mean for the players who have to update so they can play online, right? Or for the developers?...
People who host the servers, a network bump means clients with a higher version can not join so they also have to update the servers which is typically more effort than just using the launcher.
Edit: I see I left that comment earlier. I hope it doesn’t discourage you from making more PRs, but I'm afraid I'm going to close this PR as I think the changes negatively impact code readability.
Edit: I see I left that comment earlier. I hope it doesn’t discourage you from making more PRs, but I'm afraid I'm going to close this PR as I think the changes negatively impact code readability.
No problem, but this was actually split off from a bigger Ferris Wheel refactor branch so it would be easier to review. I guess this ended up making it harder to review because it's much more obtuse to understand what's going on without the renamed variables and comments I added in that branch, the downside being that the diff will be quite bigger than it should.