From 66bb2954362748c4722d366d0df490ad51a591a2 Mon Sep 17 00:00:00 2001
From: Ner'zhul <nerzhul@users.noreply.github.com>
Date: Sat, 5 Nov 2016 10:25:30 +0100
Subject: [PATCH] PlayerSAO saving fix (#4734)

PlayerSAO::disconnected() function was historical and remove the link between SAO and RemotePlayer session. With previous attributes linked to RemotePlayer saving was working. But now attributes are read from SAO not RemotePlayer and the current serialize function verify SAO exists to save the player attributes.

Because PlayerSAO::disconnected marks playersao for removal, only mark playerSAO for removal and let PlayerSAO::removingFromEnvironment do the correct saving behaviour and all the disconnection process instead of doing a partial removal and let the server loop doing the RemotePlayer cleanup and remove some saved attributes...
---
 src/content_sao.cpp  | 20 +++++++++++---------
 src/content_sao.h    |  1 +
 src/remoteplayer.cpp | 14 +++++++-------
 src/server.cpp       |  2 +-
 4 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/src/content_sao.cpp b/src/content_sao.cpp
index 5fb8f936e..dff222f42 100644
--- a/src/content_sao.cpp
+++ b/src/content_sao.cpp
@@ -838,10 +838,7 @@ void PlayerSAO::removingFromEnvironment()
 {
 	ServerActiveObject::removingFromEnvironment();
 	if (m_player->getPlayerSAO() == this) {
-		m_player->peer_id = 0;
-		m_env->savePlayer(m_player);
-		m_player->setPlayerSAO(NULL);
-		m_env->removePlayer(m_player);
+		unlinkPlayerSessionAndSave();
 		for (UNORDERED_SET<u32>::iterator it = m_attached_particle_spawners.begin();
 			it != m_attached_particle_spawners.end(); ++it) {
 			m_env->deleteParticleSpawner(*it, false);
@@ -1340,15 +1337,20 @@ void PlayerSAO::setWieldIndex(int i)
 	}
 }
 
+// Erase the peer id and make the object for removal
 void PlayerSAO::disconnected()
 {
 	m_peer_id = 0;
 	m_removed = true;
-	if(m_player->getPlayerSAO() == this)
-	{
-		m_player->setPlayerSAO(NULL);
-		m_player->peer_id = 0;
-	}
+}
+
+void PlayerSAO::unlinkPlayerSessionAndSave()
+{
+	assert(m_player->getPlayerSAO() == this);
+	m_player->peer_id = 0;
+	m_env->savePlayer(m_player);
+	m_player->setPlayerSAO(NULL);
+	m_env->removePlayer(m_player);
 }
 
 std::string PlayerSAO::getPropertyPacket()
diff --git a/src/content_sao.h b/src/content_sao.h
index 5d837a466..f58c7dadb 100644
--- a/src/content_sao.h
+++ b/src/content_sao.h
@@ -318,6 +318,7 @@ public:
 
 private:
 	std::string getPropertyPacket();
+	void unlinkPlayerSessionAndSave();
 
 	RemotePlayer *m_player;
 	u16 m_peer_id;
diff --git a/src/remoteplayer.cpp b/src/remoteplayer.cpp
index f4a79dd08..67ab89113 100644
--- a/src/remoteplayer.cpp
+++ b/src/remoteplayer.cpp
@@ -177,13 +177,13 @@ void RemotePlayer::serialize(std::ostream &os)
 	args.set("name", m_name);
 	//args.set("password", m_password);
 
-	if (m_sao) {
-		args.setS32("hp", m_sao->getHP());
-		args.setV3F("position", m_sao->getBasePosition());
-		args.setFloat("pitch", m_sao->getPitch());
-		args.setFloat("yaw", m_sao->getYaw());
-		args.setS32("breath", m_sao->getBreath());
-	}
+	// This should not happen
+	assert(m_sao);
+	args.setS32("hp", m_sao->getHP());
+	args.setV3F("position", m_sao->getBasePosition());
+	args.setFloat("pitch", m_sao->getPitch());
+	args.setFloat("yaw", m_sao->getYaw());
+	args.setS32("breath", m_sao->getBreath());
 
 	args.writeLines(os);
 
diff --git a/src/server.cpp b/src/server.cpp
index cd526ad77..48331e4f8 100644
--- a/src/server.cpp
+++ b/src/server.cpp
@@ -2654,7 +2654,7 @@ void Server::DeleteClient(u16 peer_id, ClientDeletionReason reason)
 		RemotePlayer *player = m_env->getPlayer(peer_id);
 
 		/* Run scripts and remove from environment */
-		if(player != NULL) {
+		if (player != NULL) {
 			PlayerSAO *playersao = player->getPlayerSAO();
 			assert(playersao);