From d83972dfbbfd48deefa803880821d6d668c332b4 Mon Sep 17 00:00:00 2001 From: Kp Date: Sat, 8 Dec 2018 19:16:01 +0000 Subject: [PATCH] Hide game window when advancing a dead player to the next level Commit 88b5e616a9d1f88d2eab29f863a22c5bcf2d303c ("Replace calls to window_set_visible in DoPlayerDead() with stop/start_time()") switched from hiding the game window to only stopping time, and that only if hiding the window would have stopped time. In multiplayer, this allows time to continue running. This introduced a crash if the player dies during the mine countdown. When the player dies, the game checks if the reactor has been destroyed. If so, the game immediately advances to the next level. That advance will try to draw the game window if it is visible. When the window is drawn, if time is not stopped, then game logic runs. Some of that logic is not prepared to deal with the inconsistent state present when a new level is only partially loaded. That inconsistent state then causes a crash. Call stack: #11 slew_frame () at similar/main/slew.cpp:152 #12 in d2x::object_move_one () at similar/main/object.cpp:1758 #13 in d2x::object_move_all () at similar/main/object.cpp:1956 #14 in d2x::GameProcessFrame () at similar/main/game.cpp:1848 #15 d2x::game_handler () at similar/main/game.cpp:1615 #16 in dcx::window_send_event () at common/include/window.h:116 #17 dcx::event_process () at common/arch/sdl/event.cpp:176 #18 in newmenu_do2 () at similar/main/newmenu.cpp:498 #19 in newmenu_do2 () at common/main/newmenu.h:184 #20 newmenu_do () at common/main/newmenu.h:190 #21 newmenu_do<1ul, dcx::unused_newmenu_userdata_t const> () at common/main/newmenu.h:196 #22 net_udp_wait_for_requests () at similar/main/net_udp.cpp:4563 #23 net_udp_level_sync () at similar/main/net_udp.cpp:4607 #24 in multi_level_sync () at similar/main/multi.cpp:3458 #25 in d2x::StartNewLevelSub () at similar/main/gameseq.cpp:1803 #26 in d2x::StartNewLevel () at similar/main/gameseq.cpp:2018 #27 in d2x::AdvanceLevel () at similar/main/gameseq.cpp:1648 #28 in d2x::DoPlayerDead () at similar/main/gameseq.cpp:1721 The root cause of this is the layering violation: - Killing the player can have the side effect of advancing the level - Advancing the level can have the side effect of calling multiplayer code while the level data is in an inconsistent state - Calling multiplayer code can cause the event system to redraw the game - Redrawing the game can cause game logic to run Hack around this by restoring the logic that hides the game window, so that the window is not redrawn and the game logic is not run. This does not fix the layering problem, but prevents crashing affected users. To avoid undoing the feature from the breaking commit, hide the window only when advancing to a new level, rather than unconditionally. A player advancing to a new level already lacks the move-at-cold-start capability even on successfully escaping the mine, so no functionality is lost with this change. Players who are dead and do not advance to a new level retain that capability. Fixes: 88b5e616a9d1f88d2eab29f863a22c5bcf2d303c ("Replace calls to window_set_visible in DoPlayerDead() with stop/start_time()") Reported-by: Ninjared --- similar/main/gameseq.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/similar/main/gameseq.cpp b/similar/main/gameseq.cpp index 70f3619e3..00e831f1e 100644 --- a/similar/main/gameseq.cpp +++ b/similar/main/gameseq.cpp @@ -1717,10 +1717,14 @@ window_event_result DoPlayerDead() #endif { + if (Game_wind) + window_set_visible(Game_wind, 0); result = AdvanceLevel(0); //if finished, go on to next level init_player_stats_new_ship(Player_num); last_drawn_cockpit = -1; + if (Game_wind) + window_set_visible(Game_wind, 1); } #if defined(DXX_BUILD_DESCENT_II) } else if (Current_level_num < 0) {