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

Refactor some network Packets that affect the game state to be tick bound. #5684

Closed
ZehMatt opened this issue Jun 22, 2017 · 6 comments
Closed
Labels
refactor A task that will improve code readability, without changing outcome.

Comments

@ZehMatt
Copy link
Member

ZehMatt commented Jun 22, 2017

Currently networking the player list is not bound to ticks where it should since the client always lags behind the server. We should perhaps refactor some packets to use game commands instead of being a simple network packets.

Example with PLAYERLIST packet:
Player (3) is currently placing tracks, issuing game commands at client Tick 33331
Player (1) kicks Player (3) at client Tick 33332
Server executes place track at Tick 33333, send game command back to clients.
Server executes kick at Tick 33333 and sends the new player list to all clients, player is removed at this point.
Player (2) receives place track from Player (3) at client Tick 33332, queues the command.
Player (2) receives PLAYERLIST, executed at point of arrival which removes the player if hes missing in the list received.
Player (2) executes at Tick 33333 the place track command, however the player does not exist anymore.

It should be clear that we can not remove players from the game state earlier than when the Server did it, the client has to always catch up otherwise this will cause faulty logic in the game state.

In some cases this is not a issue if the client is usually able to catch up fast enough however with higher latency this might be not the case.

I would like to hear some suggestions and ideas on how we should process with such specific things. One suggestion is, we could also include in the packet header the tick by default and tell each client if he has to wait or not for the tick to be reached, however this requires a common queue to keep order in tact.

@Gymnasiast
Copy link
Member

To me, it looks reasonable. The thing is I know next to nothing about the network layer, so I can't really comment. This is something for @IntelOrca @janisozaur and @zsilencer (if he is active again).

@janisozaur
Copy link
Member

We could have server remove kicked player's actions from the queue or if it fails to do so (e.g. the action has been already sent for the tick), defer kicking until earliest next tick.

@duncanspumpkin
Copy link
Contributor

Why not just delay the kick for a nominal amount of ticks and disallow any future commands after the kick is in the queue.

@ZehMatt
Copy link
Member Author

ZehMatt commented Jun 22, 2017

@janisozaur On the server client actions are never queued they are directly processed and then sent to the clients, theres no chance catching it before messages go out, it would be really rare to be on the same Tick where kick and place track happens from two different clients.

Clients should always do the same as the server in the same order and the same time, manipulating anything between seems error prone.

@duncanspumpkin The kick command is already bound to a tick, the real issue is that the playerlist is not and so the order of things is messed up like prematurely removing the player, it would be no issue if playerlist was a command since the server first sends the kick and then the playerlist.

@zsilencer
Copy link
Contributor

zsilencer commented Jun 22, 2017

PLAYERLIST does not affect game state.

Game commands are separate, they are not tied to the player list at all. Clients will still run the command even if the player doesn't exist. The only reason kicking is a game command is to make use of the client->server, permission thats already there etc. You don't remember but there were game commands before there was multiplayer, they are the only thing that can change game state.

Lets say you actually did this, when someone paused the game causing no more ticks to be processed, if someone joined the game or disconnected they would not even appear until it was unpaused.

@ZehMatt
Copy link
Member Author

ZehMatt commented Jun 22, 2017

Thats how pausing usually works when theres a round trip involved. The other way would be to simply have a separate list for the player list to show but the actual client cant vanish not before the kick command is executed on the client.

Edit: Also PLAYERLIST does affect the game state by removing a player, if it wouldn't remove them then its fine, I'm not sure if we are on the same definition when we speak about game state, game states usually contain all players because the game state is synchronized. So removing the player too early will cause some things to fail once the client gets to execute commands from the player before he got kicked like placing objects, now the client doesn't even know who placed them.

@Gymnasiast Gymnasiast added the refactor A task that will improve code readability, without changing outcome. label Oct 4, 2017
ZehMatt added a commit to ZehMatt/OpenRCT2 that referenced this issue Dec 9, 2018
ZehMatt added a commit to ZehMatt/OpenRCT2 that referenced this issue Dec 9, 2018
ZehMatt added a commit to ZehMatt/OpenRCT2 that referenced this issue Dec 9, 2018
Gymnasiast added a commit that referenced this issue Dec 15, 2018
janisozaur added a commit that referenced this issue Mar 13, 2019
- Feature: [#4418] Allow steep slopes on the side-friction roller coaster.
- Feature: [#7726] Add shortcut to advance one tick.
- Feature: [#7956, #7964] Add sprite font glyphs for Hungarian and some Czech letters.
- Feature: [#7971] Toolbox option to open custom content folder.
- Feature: [#7980] Allow data path for RCT1 to be specified by a command line argument.
- Feature: [#8073] Auto-upload minidumps to backtrace.io (optional, MSVC/Windows only)
- Feature: [#8078] Add save_park command to in-game console.
- Feature: [#8080] New console variable "current_rotation" to get or set view rotation.
- Feature: [#8098] Glyph for Russian rouble sign.
- Feature: [#8099] Add Powered Launch mode to Inverted RC (for RCT1 parity).
- Feature: [#8190] Allow building footpaths on 'corner down' terrain.
- Feature: [#8191] Allow building on-ride photos and water S-bends on the Water Coaster.
- Feature: [#8259] Add say command to in-game console.
- Feature: [#8374] Add replay system.
- Feature: [#8377] Add option to adjust amount of autosaves to keep.
- Feature: [#8458] Add sprite sorting benchmark.
- Feature: [#8583] Add boosters to water coaster.
- Feature: [#8648] Add optional chat button to top toolbar in multiplayer games.
- Feature: [#8652] Add network window including a graph for data usage visualisation.
- Feature: [#8670] Add ability to download missing objects when loading a park.
- Change: [#7961] Add new object types: station, terrain surface, and terrain edge.
- Change: [#8222] The climate setting has been moved from objective options to scenario options.
- Change: [#8718] Allow TARMAC object to be removed when running the `remove_unused_objects` command.
- Change: [#8718] No longer require the generic scenery groups and tarmac footpath to be checked when creating a scenario.
- Change: [#8734] Disable kick button in multiplayer window when unable to use it.
- Fix: [#3832] Changing the colour scheme of track pieces does not work in multiplayer.
- Fix: [#4094] Coasters with long flat-to-steep pieces offer them in diagonal mode (original bug).
- Fix: [#5684] Player list can desync between clients and server and can crash.
- Fix: [#6191] OpenRCT2 fails to run when the path has an emoji in it.
- Fix: [#7439] Placement messages have mixed strings
- Fix: [#7473] Disabling sound effects also disables "Disable audio on focus loss".
- Fix: [#7536] Android builds fail to start.
- Fix: [#7689] Deleting 0-tile maze gives a MONEY32_UNDEFINED (negative) refund.
- Fix: [#7828] Copied entrances and exits stay when demolishing ride.
- Fix: [#7945] Client IP address is logged as `(null)` in server logs.
- Fix: [#7952] Performance drop caused by code refactor.
- Fix: [#7954] Key validation fails on Windows due to non-ASCII user / player name.
- Fix: [#7975] Inspection flag not cleared for rides which are set to never be inspected (original bug).
- Fix: [#7985] Giant Screenshot ignores 'Map rendering' settings.
- Fix: [#7987] Broken track designs increase money by MONEY32_UNDEFINED.
- Fix: [#7991] Scenery and footpaths on Construction Rights tiles can be deleted using Clear Scenery.
- Fix: [#8034] Vanilla sprites are broken when making screenshots from command line.
- Fix: [#8045] Crash when switching between languages.
- Fix: [#8062] In multiplayer warnings for unstable cheats are shown when disabling them.
- Fix: [#8090] Maze designs saved incorrectly.
- Fix: [#8101] Title sequences window flashes after opening.
- Fix: [#8120] Crash trying to place peep spawn outside of map.
- Fix: [#8121] Crash Renaming park with server logging enabled.
- Fix: [#8139] Buying land costs money when the game is in "no money" mode.
- Fix: [#8141] Attempting to build entrance/exit on station 2 does not work.
- Fix: [#8142] Reliability of mazes and crooked houses can go below 100%.
- Fix: [#8187] Cannot set land ownership over ride entrances or exits in sandbox mode.
- Fix: [#8200] Incorrect behaviour when removing entrances and exits that are on the same tile.
- Fix: [#8204] Crash when tile element has no surface elements.
- Fix: [#8264] Rides and scenery placeable outside of map with ZC and Sandbox mode enabled.
- Fix: [#8335] Rides with arbitrary ride types can crash the game when they break down.
- Fix: [#8358] Infinite loop when changing vehicle count on stopped ride.
- Fix: [#8402] Crash closing a window in some cases.
- Fix: [#8431] Crash when game action logging is enabled.
- Fix: [#8433] Crash if master server response is not valid JSON.
- Fix: [#8434] Crash if curl_easy_init fails.
- Fix: [#8443] Crash when selecting the current vehicle for ride that has none available.
- Fix: [#8456] Junior booster track piece doesn't connect properly.
- Fix: [#8464] Crash on game shutdown.
- Fix: [#8469] Crash modifying colour on hacked rides.
- Fix: [#8508] Underground roto-drop is not going up.
- Fix: [#8555] Multiplayer window text limits are not computed properly.
- Fix: [#8572] Steel Twister track pieces ID 64 and 65 drawn incorrectly.
- Fix: [#8585] Part of track missing on air powered vertical coaster.
- Fix: [#8588] Guest list scrolling breaks above ~2000 guests.
- Fix: [#8591] Game loop does not run at a consistent tick rate of 40 Hz.
- Fix: [#8647] Marketing campaigns check for entry fees below £1 (original bug).
- Fix: [#8653] Crash when peeps attempt to enter a ride with no vehicles.
- Fix: [#8720] Desync due to boats colliding with ghost pieces.
- Fix: [#8739] Savegame from original game crashes when cruising through map.
- Fix: [#8742] Access violation in vehicle_update_sound_params.
- Fix: [#8804] Raising water shows money effect at the bottom rather than new height.
- Fix: [#8811] Some fields in the sv6 save file not being copied correctly.
- Fix: [#8824] Invalid read in footpath_chain_ride_queue.
- Improved: [#2940] Allow mouse-dragging to set patrol area (Singleplayer only).
- Improved: [#7730] Draw extreme vertical and lateral Gs red in the ride window's graph tab.
- Improved: [#7930] Automatically create folders for custom content.
- Improved: [#7980] Show the full path of the scenario in the scenario select window.
- Improved: [#7993] Allow assigning a keyboard shortcut for opening the tile inspector.
- Improved: [#8107] Support Discord release of RCT2.
- Improved: [#8491] Highlight entrance and exit with different colours in track design previews.
- Improved: Almost completely new Hungarian translation.
- Removed: [#7929] Support for scenario text objects.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor A task that will improve code readability, without changing outcome.
Projects
None yet
Development

No branches or pull requests

5 participants