From 6a6a0e8017109fbf1256bccf878277cf6e75403e Mon Sep 17 00:00:00 2001 From: Christoph Oelckers Date: Sun, 25 Sep 2016 01:28:27 +0200 Subject: [PATCH 1/3] - removed some more hubtravel related player start fudging. * do not skip the player_t init when travelling in a hub. The old player may still be needed in some edge cases. This applies only to singleplayer for now. The multiplayer version still needs reviewing. I left it alone because it may shuffle players around which is not wanted when doing hub travelling. * do not spawn two temp players in G_FinishTravel. Instead handle the case where no player_t::mo can be found gracefully by adding a few nullptr checks. This temp player served no real purpose except for having a valid pointer. The actual start position was retrieved from somewhere else. --- src/g_level.cpp | 29 +++++++++++++---------------- src/p_saveg.cpp | 11 ++++------- 2 files changed, 17 insertions(+), 23 deletions(-) diff --git a/src/g_level.cpp b/src/g_level.cpp index dcf97ba4fe..90b31138d2 100644 --- a/src/g_level.cpp +++ b/src/g_level.cpp @@ -1207,29 +1207,23 @@ void G_FinishTravel () pnum = int(pawn->player - players); pawn->ChangeStatNum (STAT_PLAYER); pawndup = pawn->player->mo; - start = NULL; assert (pawn != pawndup); - if (pawndup == NULL) - { // Oh no! there was no start for this player! - start = G_PickPlayerStart(pnum, PPS_FORCERANDOM); - if (start != NULL) pawndup = P_SpawnPlayer(start, pnum, (level.flags2 & LEVEL2_PRERAISEWEAPON) ? SPF_WEAPONFULLYUP : 0); - if (pawndup == NULL) - { - pawn->flags |= MF_NOSECTOR | MF_NOBLOCKMAP; - pawn->Destroy(); - continue; - } - } + start = G_PickPlayerStart(pnum, 0); if (start == NULL) { - start = G_PickPlayerStart(pnum, 0); - if (start == NULL) + if (pawndup != nullptr) { Printf(TEXTCOLOR_RED "No player %d start to travel to!\n", pnum + 1); // Move to the coordinates this player had when they left the level. pawn->SetXYZ(pawndup->Pos()); } + else + { + // Could not find a start for this player at all. This really should never happen but if it does, let's better abort. + DThinker::DestroyThinkersInList(STAT_TRAVELLING); + I_Error ("No player %d start to travel to!\n", pnum + 1); + } } oldpawn = pawndup; @@ -1266,8 +1260,11 @@ void G_FinishTravel () pawn->player->camera = pawn; pawn->player->viewheight = pawn->ViewHeight; pawn->flags2 &= ~MF2_BLASTED; - DObject::StaticPointerSubstitution (oldpawn, pawn); - oldpawn->Destroy(); + if (oldpawn != nullptr) + { + DObject::StaticPointerSubstitution (oldpawn, pawn); + oldpawn->Destroy(); + } if (pawndup != NULL) { pawndup->Destroy(); diff --git a/src/p_saveg.cpp b/src/p_saveg.cpp index 4657601374..2382d3ede5 100644 --- a/src/p_saveg.cpp +++ b/src/p_saveg.cpp @@ -537,7 +537,7 @@ void P_SerializeSounds(FSerializer &arc) //========================================================================== void CopyPlayer(player_t *dst, player_t *src, const char *name); -static void ReadOnePlayer(FSerializer &arc, bool skipload); +static void ReadOnePlayer(FSerializer &arc); static void ReadMultiplePlayers(FSerializer &arc, int numPlayers, int numPlayersNow, bool skipload); static void SpawnExtraPlayers(); @@ -594,7 +594,7 @@ void P_SerializePlayers(FSerializer &arc, bool skipload) // first player present, no matter what their name. if (numPlayers == 1) { - ReadOnePlayer(arc, skipload); + ReadOnePlayer(arc); } else { @@ -617,7 +617,7 @@ void P_SerializePlayers(FSerializer &arc, bool skipload) // //========================================================================== -static void ReadOnePlayer(FSerializer &arc, bool skipload) +static void ReadOnePlayer(FSerializer &arc) { int i; const char *name = NULL; @@ -636,10 +636,7 @@ static void ReadOnePlayer(FSerializer &arc, bool skipload) didIt = true; player_t playerTemp; playerTemp.Serialize(arc); - if (!skipload) - { - CopyPlayer(&players[i], &playerTemp, name); - } + CopyPlayer(&players[i], &playerTemp, name); } else { From 92d0043a81705cb4fa6d1a78552ba50f16b7e207 Mon Sep 17 00:00:00 2001 From: Christoph Oelckers Date: Sun, 25 Sep 2016 09:23:44 +0200 Subject: [PATCH 2/3] - undid part of the last commit and hopefully corrected it for good now. We have to be extremely careful with the player data, because there's just too much code littered around that has certain expectations about what needs to be present and what not. Obviously, when travelling in a hub, the player_t should be retained from the previous level. But we still have to set player_t::mo to the PlayerPawn from the savegame so that G_UnsnapshotLevel doesn't prematurely delete it and all associated voodoo dolls, because it checks player_t::mo to decide whether a player is valid or not. The actual deletion of this redundant PlayerPawn should only be done in G_FinishTravel, after the actual player has been fully set up --- src/p_saveg.cpp | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/src/p_saveg.cpp b/src/p_saveg.cpp index 2382d3ede5..9bb9f82e8e 100644 --- a/src/p_saveg.cpp +++ b/src/p_saveg.cpp @@ -537,7 +537,7 @@ void P_SerializeSounds(FSerializer &arc) //========================================================================== void CopyPlayer(player_t *dst, player_t *src, const char *name); -static void ReadOnePlayer(FSerializer &arc); +static void ReadOnePlayer(FSerializer &arc, bool skipload); static void ReadMultiplePlayers(FSerializer &arc, int numPlayers, int numPlayersNow, bool skipload); static void SpawnExtraPlayers(); @@ -594,7 +594,7 @@ void P_SerializePlayers(FSerializer &arc, bool skipload) // first player present, no matter what their name. if (numPlayers == 1) { - ReadOnePlayer(arc); + ReadOnePlayer(arc, skipload); } else { @@ -617,7 +617,7 @@ void P_SerializePlayers(FSerializer &arc, bool skipload) // //========================================================================== -static void ReadOnePlayer(FSerializer &arc) +static void ReadOnePlayer(FSerializer &arc, bool skipload) { int i; const char *name = NULL; @@ -636,7 +636,15 @@ static void ReadOnePlayer(FSerializer &arc) didIt = true; player_t playerTemp; playerTemp.Serialize(arc); - CopyPlayer(&players[i], &playerTemp, name); + if (!skipload) + { + CopyPlayer(&players[i], &playerTemp, name); + } + else + { + // we need the player actor, so that G_FinishTravel can destroy it later. + players[i].mo = playerTemp.mo; + } } else { @@ -750,6 +758,13 @@ static void ReadMultiplePlayers(FSerializer &arc, int numPlayers, int numPlayers } } } + else + { + for (i = 0; i < MAXPLAYERS; ++i) + { + players[i].mo = playertemp[i].mo; + } + } delete[] tempPlayerUsed; delete[] playertemp; From f4462a7b93312a9b50e8011a68ef0967326d08ed Mon Sep 17 00:00:00 2001 From: Christoph Oelckers Date: Sun, 25 Sep 2016 09:43:17 +0200 Subject: [PATCH 3/3] - fixed: Any DSectorEffect thinker must be placed into STAT_SECTOREFFECT. The slot had been there forever to address this same problem but only one of the two constructors actually set it, too bad that it was the wrong one... This is something that normally won't be noticed. But if some actor is spawned on a moving platform, with both thinkers on the same statnum it means that the order of execution is not correct with the platform being done first, resulting in the actor to 'jump' while the platform is moving. To prevent this it is necessary that all sector movers only tick after all actors have completed their thinking turn. --- src/dsectoreffect.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/dsectoreffect.cpp b/src/dsectoreffect.cpp index 78e00e6b04..815b49dc47 100644 --- a/src/dsectoreffect.cpp +++ b/src/dsectoreffect.cpp @@ -60,6 +60,7 @@ void DSectorEffect::Destroy() } DSectorEffect::DSectorEffect (sector_t *sector) + : DThinker(STAT_SECTOREFFECT) { m_Sector = sector; }