Fixed few multiplayer desync issues. #5578

ZehMatt · 2017-06-10T06:14:44Z

Fixed peeps trying to watch ghost rides in multiplayer.
Fixed using the wrong tick number for commands, client was ahead one tick in reality.
Added some logging for scenario_rand when DEBUG_DESYNC is defined.

Fixed peeps trying to watch ghost rides in multiplayer. Fixed using the wrong tick number for commands, client as ahead one tick in reality. Added some logging scenario_rand when DEBUG_DESYNC is defined.

Gymnasiast · 2017-06-10T08:12:42Z

src/openrct2/world/Climate.cpp

@@ -181,7 +181,7 @@ extern "C"
        else if (gClimateCurrentWeatherEffect == 2)
        {
            // Create new thunder and lightning
-            uint32 randomNumber = util_rand();
+            uint32 randomNumber = scenario_rand();


Is thunder not completely client-side?

Its still driven by a prng which is at least sync so every client should see the thunder at the same time.

There is an option to disable thunders. This may cause a client UI layer not to call this function and it would desync this client, that's why it is client side.

More precisely: there is an option to disable lightning flashes. You'll still hear thunder.

I need to double check this, but the thunder and lightning effects should be purely client side and aesthetic.

After sleeping over it I just realized that, altho it should still use the same prng stream however not affect it so at least all clients will run the same effects.

Margen67 · 2017-06-10T18:16:49Z

Once this is merged https://github.com/OpenRCT2/OpenRCT2/wiki/Multiplayer-issues will have to be updated.

janisozaur · 2017-06-10T20:23:22Z

@ZehM4tt thanks for this PR. While it looks like it indeed solves some desyncs, the map you provided for testing: aaa.sv6.txt still exhibits some.

It reproduces quite reliably for me under these conditions:

Arch Linux + GCC 7
No title sequence
server: ./openrct2 host aaa.sv6
client: ./openrct2 join 127.0.0.1
using rct1 theme
on client: spam the close/open button of the Autoskooter 1 ride.

…ew order.

janisozaur · 2017-06-10T21:15:27Z

src/openrct2/scenario/scenario.c

    if (!gInUpdateCode) {
        log_warning("scenario_rand called from outside game update");
-        assert(false);
+        //assert(false);


@ZehM4tt any particular reason this is commented out? I mentioned on gitter I hit this condition at times, but the PRNG looks fine regardless. Are there known good paths leading to this point?

Can you either provide justification and remove this line or leave the assert in?

Its only active with DEBUG_DESYNC and it was triggering even tho its fine to be called at the given time. I would just leave it commented in case someone requires to stop there to investigate the stack or memory.

I'm a bit nervous about this. I do not know enough about network to say why it got in, but an assertion is never put in for nothing.

There are false positives, its just annoying to have it there while investigating.

janisozaur · 2017-06-10T21:16:25Z

src/openrct2/world/map.c

@@ -3659,7 +3659,8 @@ void map_update_tiles()
        }

        gGrassSceneryTileLoopPosition++;
-        gGrassSceneryTileLoopPosition &= 0xFFFF;
+        // Type is uint16, so why do we need this exactly?
+        //gGrassSceneryTileLoopPosition &= 0xFFFF;


Is this needed? Looks redundant, so perhaps should be part of another PR?

Perhaps leave ZehM4tt's changes intact and create an issue for it?

janisozaur · 2017-06-10T21:17:43Z

src/openrct2/network/network.cpp

@@ -1124,7 +1131,7 @@ void Network::Server_Send_TICK()
    // but debug version can check more often.
    static sint32 checksum_counter = 0;
    checksum_counter++;
-    if (checksum_counter >= 100) {
+    if (checksum_counter >= 5) {


Back this out, such low value is only needed for testing

janisozaur · 2017-06-10T21:22:22Z

src/openrct2/peep/peep.c

@@ -174,6 +174,12 @@ static void peep_update_ride_inspected(sint32 rideIndex);

 bool loc_690FD0(rct_peep *peep, uint8 *rideToView, uint8 *rideSeatToView, rct_map_element *esi);

+#ifdef DEBUG_DESYNC 
+#define peep_rand() scenario_rand_data(peep)


peep may not be as useful as part of logging, as it's id: https://github.com/OpenRCT2/OpenRCT2/blob/develop/src/openrct2/peep/peep.h#L469

Sweet-Tan · 2017-06-10T21:42:55Z

Crash while on ed06a22
4a0b6f13-24a0-4d41-a80e-0992e4973316.zip

janisozaur · 2017-06-11T05:47:44Z

@ZehM4tt it may not be related directly to your PR, as I don't recall any new places where it could crash, but perhaps I missed something. Can you please take a look at this dump?

ZehMatt · 2017-06-11T06:40:10Z

Its a sound crash, where mixer is NULL.
Stack trace:

openrct2.dll!Mixer_Play_Music(int pathId=3, int loop=-1, int streaming=0) Line 150 C++
openrct2.dll!peep_update_crowd_noise(...) Line 6983 C
openrct2.dll!game_logic_update(...) Line 421 C
openrct2.dll!game_update(...) Line 318 C

janisozaur · 2017-06-11T07:19:28Z

@ZehM4tt can you please post the stack trace as text? Images are not searchable nor they add any value. It also looks like this is irrelevant to your PR and something that hasn't been reported yet, so can you please file a new issue with all the information there?

ZehMatt · 2017-06-11T08:49:14Z

I'm not the one who found it to begin with and I have no idea how to trigger it, etc. The picture was merely some indication that it doesn't belong here.

Gymnasiast

I left a few line notes to address.

I'm not sure the debug stuff should be in this PR, but I'm not going to obstruct this if I'm the only one.

I do not know enough game_update() either to say anything about that, I'll leave that to IntelOrca, Janisozaur and Duncan.

Gymnasiast · 2017-06-11T19:19:45Z

src/openrct2/peep/peep.c

@@ -11344,6 +11350,12 @@ static void peep_easter_egg_peep_interactions(rct_peep *peep)
 */
 static bool peep_should_watch_ride(rct_map_element *mapElement) {
    rct_ride *ride = get_ride(mapElement->properties.track.ride_index);
+
+    // In case of upcoming references we also have the check in here.


I find this comment a bit vague. It isn't clear what it's trying to prevent, nor does it say why (I know it's to prevents desyncs, but the comment should really say that).

Gymnasiast · 2017-06-11T19:20:43Z

src/openrct2/peep/peep.c

@@ -11394,6 +11406,9 @@ static bool peep_find_ride_to_look_at(rct_peep *peep, uint8 edge, uint8 *rideToV

    mapElement = surfaceElement;
    do {
+        if (network_get_mode() != NETWORK_MODE_NONE) {


Please put a comment explaining this is to prevent desyncs from interaction with ghost elements.

Gymnasiast · 2017-06-11T19:22:17Z

src/openrct2/scenario/scenario.c

    if (!gInUpdateCode) {
        log_warning("scenario_rand called from outside game update");
-        assert(false);
+        //assert(false);


I'm a bit nervous about this. I do not know enough about network to say why it got in, but an assertion is never put in for nothing.

Gymnasiast · 2017-06-11T19:24:14Z

src/openrct2/world/map.c

@@ -3659,7 +3659,8 @@ void map_update_tiles()
        }

        gGrassSceneryTileLoopPosition++;
-        gGrassSceneryTileLoopPosition &= 0xFFFF;
+        // Type is uint16, so why do we need this exactly?
+        //gGrassSceneryTileLoopPosition &= 0xFFFF;


Perhaps leave ZehM4tt's changes intact and create an issue for it?

Gymnasiast · 2017-06-11T19:24:27Z

src/openrct2/world/map.c

@@ -3719,6 +3720,9 @@ static void map_update_grass_length(sint32 x, sint32 y, rct_map_element *mapElem
            mapElementAbove++;
            if (map_element_get_type(mapElementAbove) == MAP_ELEMENT_TYPE_WALL)
                continue;
+            // Grass should not be affected by ghost elements.


Well spotted.

janisozaur · 2017-06-11T20:26:05Z

@Gymnasiast I addressed your comments. I'm not sure about the one that wants to have comment next to ghost checks, there are quite a few and having a comment next to each one would introduce a lot of clutter, so I'm not exactly for it.

@duncanspumpkin expressed concerns about passing the peep data to logging, but I concur. I think I could use such data and even if not, it's a good example of how to use the little debug features we have.

[ci skip]

janisozaur · 2017-06-11T20:31:13Z

@Gymnasiast I addressed all your comment now.

duncanspumpkin · 2017-06-12T05:45:01Z

My concern with the peep debug Rand was. does logging the Id/peep give any additional useful information? For debugging a desync it's rare that you would care about an individual sprite and if you did know which sprite caused the desync how would that help.

Gymnasiast

My notes are addressed. I'll leave the game_update() bits and related stuff to @janisozaur and @duncanspumpkin .

ZehMatt · 2017-06-12T06:18:17Z

@duncanspumpkin It did help determine if the order is correct not necessarily the peep in question. If peep A comes before peep B on server then peep A must do all decision making on client before peep B as well. Since all allocation seems to be deterministic on my Windows 10 machine with debug heap this can be quite helpful, but you are right about the data its self, its quite useless. However this is only enabled if DEBUG_DESYNC is defined, its probably best to use their object id for logging.

This addresses some of the desync causes: * `vehicle_create_car` was using `scenario_rand` when it shouldn't have * ghost elements affected grass growth * ghost elements affecting peep logic[1] It also adds some desync debug facilities, enabled at compile time. It also reverts part of change introduced in OpenRCT2#5185, namely reorder of desync check vs call to `ProcessGameCommandQueue();` [1] It is not ideal to have this check in multiple locations, it is prone to human error. We already have `map_remove_provisional_elements`, but it is possible it does not work as well as it should. This needs further investigation.

ZehMatt added 3 commits June 10, 2017 08:11

Fixed ghost rides affecting grass.

8767a00

Fixed peeps trying to watch ghost rides in multiplayer. Fixed using the wrong tick number for commands, client as ahead one tick in reality. Added some logging scenario_rand when DEBUG_DESYNC is defined.

Okay.

d707483

Tabs to spaced.

74f9aa8

ZehMatt changed the title ~~Fixed ghost rides affecting grass.~~ Fixed few multiplayer desync issues. Jun 10, 2017

Use scenario_rand instead of local util_rand which is local to time.

ae61849

Gymnasiast reviewed Jun 10, 2017

View reviewed changes

Pull back scenario_rand to util_rand for weather.

ddaff4f

ZehMatt mentioned this pull request Jun 10, 2017

Network desync immediately after connecting #5579

Closed

Fixed network desync check in some cases being wrong because of the n…

e86bc67

…ew order.

janisozaur reviewed Jun 10, 2017

View reviewed changes

Undone testing value, was only there for testing purposes.

62e8aba

janisozaur reviewed Jun 10, 2017

View reviewed changes

janisozaur requested a review from Gymnasiast June 10, 2017 21:24

janisozaur added 3 commits June 11, 2017 21:22

Revert change irrelevant to this PR

0fc0fd0

Restore commented out assert

621a873

Improve comment

7073c0d

Gymnasiast requested changes Jun 11, 2017

View reviewed changes

Gymnasiast requested a review from duncanspumpkin June 11, 2017 19:28

janisozaur added the network version Network version needs updating - double check before merging! label Jun 11, 2017

Improve comments

0dde369

[ci skip]

janisozaur force-pushed the issue-5058 branch from de00130 to 0dde369 Compare June 11, 2017 20:29

Gymnasiast approved these changes Jun 12, 2017

View reviewed changes

janisozaur merged commit 60bf508 into OpenRCT2:develop Jun 12, 2017

This was referenced Jun 12, 2017

Guests stop and watch ride preview "ghost pieces", causing desyncs in multiplayer #5058

Closed

Crash on new assert from #5587 #5602

Closed

ZehMatt deleted the issue-5058 branch July 19, 2017 21:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed few multiplayer desync issues. #5578

Fixed few multiplayer desync issues. #5578

ZehMatt commented Jun 10, 2017

Gymnasiast Jun 10, 2017

ZehMatt Jun 10, 2017

janisozaur Jun 10, 2017

Gymnasiast Jun 10, 2017

IntelOrca Jun 10, 2017

ZehMatt Jun 10, 2017 •

edited

Margen67 commented Jun 10, 2017

janisozaur commented Jun 10, 2017

janisozaur Jun 10, 2017

ZehMatt Jun 10, 2017

Gymnasiast Jun 11, 2017

ZehMatt Jun 12, 2017

janisozaur Jun 10, 2017

ZehMatt Jun 10, 2017

Gymnasiast Jun 11, 2017

janisozaur Jun 10, 2017

ZehMatt Jun 10, 2017

janisozaur Jun 10, 2017

Sweet-Tan commented Jun 10, 2017

janisozaur commented Jun 11, 2017

ZehMatt commented Jun 11, 2017 •

edited

janisozaur commented Jun 11, 2017

ZehMatt commented Jun 11, 2017

Gymnasiast left a comment

Gymnasiast Jun 11, 2017

Gymnasiast Jun 11, 2017

Gymnasiast Jun 11, 2017

Gymnasiast Jun 11, 2017

Gymnasiast Jun 11, 2017

janisozaur commented Jun 11, 2017

janisozaur commented Jun 11, 2017

duncanspumpkin commented Jun 12, 2017

Gymnasiast left a comment

ZehMatt commented Jun 12, 2017

Fixed few multiplayer desync issues. #5578

Fixed few multiplayer desync issues. #5578

Conversation

ZehMatt commented Jun 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZehMatt Jun 10, 2017 • edited

Choose a reason for hiding this comment

Margen67 commented Jun 10, 2017

janisozaur commented Jun 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sweet-Tan commented Jun 10, 2017

janisozaur commented Jun 11, 2017

ZehMatt commented Jun 11, 2017 • edited

janisozaur commented Jun 11, 2017

ZehMatt commented Jun 11, 2017

Gymnasiast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janisozaur commented Jun 11, 2017

janisozaur commented Jun 11, 2017

duncanspumpkin commented Jun 12, 2017

Gymnasiast left a comment

Choose a reason for hiding this comment

ZehMatt commented Jun 12, 2017

ZehMatt Jun 10, 2017 •

edited

ZehMatt commented Jun 11, 2017 •

edited