We found that when we call to AGameModeBase::RestartPlayer the event OnPossessedPawnChanged is dispatched with NewPawn equals to nullptr.
We think that it should been two symmetrical events OnPossessedPawnChanged, one from the current Pawn to nullptr and one from nullptr to the current Pawn, or no event at all.
The problem is that AGameModeBase::RestartPlayer ends calling AGameModeBase::FinishRestartPlayer that calls Possess with the Pawn we already possess:
// If the Pawn is destroyed as part of possession we have to abort
if (!IsValid(NewPlayer->GetPawn()))
…`
When you call this the flow is as follows:
AController::Possess calls OnPosses.
APlayerController::OnPossess checks that its the same pawn and it does NOT call to AController::UnPossess.
APlayerController::OnPossess checks that the Pawn has a controller and calls to AController::UnPossess.
AController::UnPossess calls OnUnPossess.
APlayerController::OnUnPossess sets the Pawn to nullptr.
AController::UnPossess checks that the Pawn has hanged and dispaches the event OnPossessedPawnChanged with NewPawn equals to nullptr and OldPawn equals to the current Pawn.
APlayerController::OnPossess sets the Pawn to the pawn it has to possess (the same it had at the begining).
AController::Possess checks that the Pawn has not changed and bNotificationRequired is false and does NOT dispaches the event OnPossessedPawnChanged with NewPawn equals to the current Pawn and OldPawn equals to the current Pawn.
At the end the event OnPossessedPawnChanged has being dispatched once indicating that the possession has changed and that the NewPaw is nullptr but this is false. The PlayerController is possessing the same Pawn that it was possessing at the begining of the process.
We think that a possible solution would be to modify APlayerController::OnPossess to do not call to AController::UnPossess if the controller of the Pawn is not null and it is not the same controller that it is trying to possess it:
Steps to Reproduce
Call to Possess from a PlayerController passing the Pawn of that PlayerController. There is such call already in the engine in AGameModeBase::FinishRestartPlayer:
NewPlayer->Possess(NewPlayer->GetPawn());
To reproduce it call AGameModeBase::RestartPlayer more than once.
Thanks for the explanation and the proposed solution.
I’ve reviewed the issue and passed it along to Epic for internal evaluation. It does make sense that the delegate does nos trigger when NewPawn is null, even when the pawn hasn’t actually changed.
From my review, I don’t see any side effects that could affect the engine. However, since this touches core logic that could have other side effect, we’ll need to wait for Epic input on this. I’ll let you know once I get a response.
Let us know if you found any issue with your implementation so I can comunicate it to Epic.
Hey there, a miscommunication took place and I’m taking ownership of this case now.
Regarding your change to APlayerController::OnPossess: this is a tricky topic. In my opinion it’s a safe fix for your project, but things are not clear-cut as to whether this should be in the engine. The issue is backwards compatibility and consistency:
Game projects may depend on APlayerController::(On)UnPossess being called to do cleanup when RestartPlayer() is called.
It’s confusing when pawns get redundant PossessedBy() calls, but controllers don’t get redundant OnPossessedPawnChanged calls.
For an engine fix I’d prefer to broadcast the delegate one more time (pawn -> null, null -> pawn) rather than one less time (no broadcasts). However, in AController::Possess() it’s difficult to detect whether the call to OnPossess(InPawn) will internally broadcast OnPossessedPawnChanged because OnPossess is virtual. Still, I prefer making that assumption over removing a broadcast. Here is what my suggested engine change would look like in AController::Possess():
// A notification is required when the current assigned pawn is not possessed (i.e. pawn assigned before calling Possess). // Since default engine behavior is to unpossess the current pawn in OnPossess() triggering OnPossessedPawnChanged(CurrentPawn -> nullptr) // even if we're re-possessing the same pawn, also remember to broadcast OnPossessedPawnChanged in that case. const bool bNotificationRequired = (CurrentPawn != nullptr) && (CurrentPawn->GetController() == nullptr || CurrentPawn->GetController() == this);I’ll gather team thoughts before submitting an engine fix.
Thanks for you answer. We have tried this solution and it fixes our problem. Broadcasting the delegate one more time it is not a problem in our game, we will use this as it seem safer.