Environment cleanup

* Move client list to ServerEnvironment and use RemotePlayer members instead of Player
* ClientEnvironment only use setLocalPlayer to specify the current player
* Remove ClientEnvironment dead code on player list (in fact other players are CAO not Player objects)
* Drop LocalPlayer::getPlayer(xxx) functions which aren't used.
* Improve a little bit performance by using const ref list for ClientEnvironment::getPlayerNames() & Client::getConnectedPlayerNames()
* Drop isLocal() function from (Local)Player which is not needed anymore because of previous changes

This change permits to cleanup shared client list which is very old code.
ClientEnvironment doesn't use player list anymore, it only contains the local player, as addPlayer is only called from Client constructor client side.
Clients are only CAO on client side, this cleanup permit to remove confusion about player list.

diff --git a/src/client.cpp b/src/client.cpp
index cd010e592f1da0ece8c80131ea172f9b5bbc9eed..62bd274aab557ff7f4d739ac92b7a48282623bf0 100644 (file)
--- a/src/client.cpp
+++ b/src/client.cpp
@@ -257,7 +257,7 @@ Client::Client(
        m_localdb(NULL)
 {
        // Add local player
-       m_env.addPlayer(new LocalPlayer(this, playername));
+       m_env.setLocalPlayer(new LocalPlayer(this, playername));
 
        m_mapper = new Mapper(device, this);
        m_cache_save_interval = g_settings->getU16("server_map_save_interval");
@@ -1418,8 +1418,9 @@ Inventory* Client::getInventory(const InventoryLocation &loc)
        break;
        case InventoryLocation::PLAYER:
        {
-               LocalPlayer *player = m_env.getPlayer(loc.name.c_str());
-               if(!player)
+               // Check if we are working with local player inventory
+               LocalPlayer *player = m_env.getLocalPlayer();
+               if (!player || strcmp(player->getName(), loc.name.c_str()) != 0)
                        return NULL;
                return &player->inventory;
        }
@@ -1500,11 +1501,6 @@ ClientActiveObject * Client::getSelectedActiveObject(
        return NULL;
 }
 
-std::list<std::string> Client::getConnectedPlayerNames()
-{
-       return m_env.getPlayerNames();
-}
-
 float Client::getAnimationTime()
 {
        return m_animation_time;
@@ -1664,7 +1660,7 @@ void Client::addUpdateMeshTaskForNode(v3s16 nodepos, bool ack_to_server, bool ur
 ClientEvent Client::getClientEvent()
 {
        ClientEvent event;
-       if(m_client_event_queue.size() == 0) {
+       if (m_client_event_queue.empty()) {
                event.type = CE_NONE;
        }
        else {

diff --git a/src/client.h b/src/client.h
index 72baac2d29b093afb1dc3564a78c663d2483bb15..6d24c0b1d27219bc45980dc952717c3c6332fd93 100644 (file)
--- a/src/client.h
+++ b/src/client.h
@@ -452,7 +452,10 @@ public:
                        core::line3d<f32> shootline_on_map
        );
 
-       std::list<std::string> getConnectedPlayerNames();
+       const std::list<std::string> &getConnectedPlayerNames()
+       {
+               return m_env.getPlayerNames();
+       }
 
        float getAnimationTime();
 

diff --git a/src/content_cao.cpp b/src/content_cao.cpp
index a5376814955a232f89daf751552875fd31e61fd9..00e17f328b32e2b4d972b00bc3124b71d1abe5ee 100644 (file)
--- a/src/content_cao.cpp
+++ b/src/content_cao.cpp
@@ -653,10 +653,10 @@ void GenericCAO::initialize(const std::string &data)
        pos_translator.init(m_position);
        updateNodePos();
 
-       if(m_is_player)
-       {
-               LocalPlayer *player = m_env->getPlayer(m_name.c_str());
-               if (player && player->isLocal()) {
+       if (m_is_player) {
+               // Check if it's the current player
+               LocalPlayer *player = m_env->getLocalPlayer();
+               if (player && strcmp(player->getName(), m_name.c_str()) == 0) {
                        m_is_local_player = true;
                        m_is_visible = false;
                        LocalPlayer* localplayer = player;

diff --git a/src/environment.cpp b/src/environment.cpp
index ff43ac516242f35125e94e37dbaf1e96bfbf2457..ceaa01d7a4d6dfeff723a906d983ae693c4e19a0 100644 (file)
--- a/src/environment.cpp
+++ b/src/environment.cpp
@@ -69,62 +69,6 @@ Environment::Environment():
 
 Environment::~Environment()
 {
-       // Deallocate players
-       for(std::vector<Player*>::iterator i = m_players.begin();
-                       i != m_players.end(); ++i) {
-               delete (*i);
-       }
-}
-
-void Environment::addPlayer(Player *player)
-{
-       DSTACK(FUNCTION_NAME);
-       /*
-               Check that peer_ids are unique.
-               Also check that names are unique.
-               Exception: there can be multiple players with peer_id=0
-       */
-       // If peer id is non-zero, it has to be unique.
-       if(player->peer_id != 0)
-               FATAL_ERROR_IF(getPlayer(player->peer_id) != NULL, "Peer id not unique");
-       // Name has to be unique.
-       FATAL_ERROR_IF(getPlayer(player->getName()) != NULL, "Player name not unique");
-       // Add.
-       m_players.push_back(player);
-}
-
-void Environment::removePlayer(Player* player)
-{
-       for (std::vector<Player*>::iterator it = m_players.begin();
-                       it != m_players.end(); ++it) {
-               if ((*it) == player) {
-                       delete *it;
-                       m_players.erase(it);
-                       return;
-               }
-       }
-}
-
-Player *Environment::getPlayer(u16 peer_id)
-{
-       for (std::vector<Player*>::iterator i = m_players.begin();
-                       i != m_players.end(); ++i) {
-               Player *player = *i;
-               if (player->peer_id == peer_id)
-                       return player;
-       }
-       return NULL;
-}
-
-Player *Environment::getPlayer(const char *name)
-{
-       for (std::vector<Player*>::iterator i = m_players.begin();
-                       i != m_players.end(); ++i) {
-               Player *player = *i;
-               if(strcmp(player->getName(), name) == 0)
-                       return player;
-       }
-       return NULL;
 }
 
 u32 Environment::getDayNightRatio()
@@ -545,10 +489,16 @@ ServerEnvironment::~ServerEnvironment()
        m_map->drop();
 
        // Delete ActiveBlockModifiers
-       for(std::vector<ABMWithState>::iterator
+       for (std::vector<ABMWithState>::iterator
                        i = m_abms.begin(); i != m_abms.end(); ++i){
                delete i->abm;
        }
+
+       // Deallocate players
+       for (std::vector<RemotePlayer *>::iterator i = m_players.begin();
+                       i != m_players.end(); ++i) {
+               delete (*i);
+       }
 }
 
 Map & ServerEnvironment::getMap()
@@ -563,12 +513,53 @@ ServerMap & ServerEnvironment::getServerMap()
 
 RemotePlayer *ServerEnvironment::getPlayer(const u16 peer_id)
 {
-       return dynamic_cast<RemotePlayer *>(Environment::getPlayer(peer_id));
+       for (std::vector<RemotePlayer *>::iterator i = m_players.begin();
+                       i != m_players.end(); ++i) {
+               RemotePlayer *player = *i;
+               if (player->peer_id == peer_id)
+                       return player;
+       }
+       return NULL;
 }
 
 RemotePlayer *ServerEnvironment::getPlayer(const char* name)
 {
-       return dynamic_cast<RemotePlayer *>(Environment::getPlayer(name));
+       for (std::vector<RemotePlayer *>::iterator i = m_players.begin();
+                       i != m_players.end(); ++i) {
+               RemotePlayer *player = *i;
+               if (strcmp(player->getName(), name) == 0)
+                       return player;
+       }
+       return NULL;
+}
+
+void ServerEnvironment::addPlayer(RemotePlayer *player)
+{
+       DSTACK(FUNCTION_NAME);
+       /*
+               Check that peer_ids are unique.
+               Also check that names are unique.
+               Exception: there can be multiple players with peer_id=0
+       */
+       // If peer id is non-zero, it has to be unique.
+       if (player->peer_id != 0)
+               FATAL_ERROR_IF(getPlayer(player->peer_id) != NULL, "Peer id not unique");
+       // Name has to be unique.
+       FATAL_ERROR_IF(getPlayer(player->getName()) != NULL, "Player name not unique");
+       // Add.
+       m_players.push_back(player);
+}
+
+void ServerEnvironment::removePlayer(RemotePlayer *player)
+{
+       for (std::vector<RemotePlayer *>::iterator it = m_players.begin();
+                       it != m_players.end(); ++it) {
+               if ((*it) == player) {
+                       delete *it;
+                       m_players.erase(it);
+                       return;
+               }
+       }
 }
 
 bool ServerEnvironment::line_of_sight(v3f pos1, v3f pos2, float stepsize, v3s16 *p)
@@ -601,7 +592,7 @@ bool ServerEnvironment::line_of_sight(v3f pos1, v3f pos2, float stepsize, v3s16
 void ServerEnvironment::kickAllPlayers(AccessDeniedCode reason,
                const std::string &str_reason, bool reconnect)
 {
-       for (std::vector<Player*>::iterator it = m_players.begin();
+       for (std::vector<RemotePlayer *>::iterator it = m_players.begin();
                        it != m_players.end(); ++it) {
                RemotePlayer *player = dynamic_cast<RemotePlayer *>(*it);
                ((Server*)m_gamedef)->DenyAccessVerCompliant(player->peer_id,
@@ -614,7 +605,7 @@ void ServerEnvironment::saveLoadedPlayers()
        std::string players_path = m_path_world + DIR_DELIM "players";
        fs::CreateDir(players_path);
 
-       for (std::vector<Player*>::iterator it = m_players.begin();
+       for (std::vector<RemotePlayer *>::iterator it = m_players.begin();
                        it != m_players.end();
                        ++it) {
                RemotePlayer *player = static_cast<RemotePlayer*>(*it);
@@ -1253,7 +1244,7 @@ void ServerEnvironment::step(float dtime)
        */
        {
                ScopeProfiler sp(g_profiler, "SEnv: handle players avg", SPT_AVG);
-               for (std::vector<Player*>::iterator i = m_players.begin();
+               for (std::vector<RemotePlayer *>::iterator i = m_players.begin();
                                i != m_players.end(); ++i) {
                        RemotePlayer *player = dynamic_cast<RemotePlayer *>(*i);
                        assert(player);
@@ -1276,7 +1267,7 @@ void ServerEnvironment::step(float dtime)
                        Get player block positions
                */
                std::vector<v3s16> players_blockpos;
-               for (std::vector<Player*>::iterator i = m_players.begin();
+               for (std::vector<RemotePlayer *>::iterator i = m_players.begin();
                                i != m_players.end(); ++i) {
                        RemotePlayer *player = dynamic_cast<RemotePlayer *>(*i);
                        assert(player);
@@ -2255,6 +2246,7 @@ ClientEnvironment::ClientEnvironment(ClientMap *map, scene::ISceneManager *smgr,
                ITextureSource *texturesource, IGameDef *gamedef,
                IrrlichtDevice *irr):
        m_map(map),
+       m_local_player(NULL),
        m_smgr(smgr),
        m_texturesource(texturesource),
        m_gamedef(gamedef),
@@ -2291,37 +2283,16 @@ ClientMap & ClientEnvironment::getClientMap()
        return *m_map;
 }
 
-LocalPlayer *ClientEnvironment::getPlayer(const u16 peer_id)
-{
-       return dynamic_cast<LocalPlayer *>(Environment::getPlayer(peer_id));
-}
-
-LocalPlayer *ClientEnvironment::getPlayer(const char* name)
-{
-       return dynamic_cast<LocalPlayer *>(Environment::getPlayer(name));
-}
-
-void ClientEnvironment::addPlayer(LocalPlayer *player)
+void ClientEnvironment::setLocalPlayer(LocalPlayer *player)
 {
        DSTACK(FUNCTION_NAME);
        /*
                It is a failure if already is a local player
        */
-       FATAL_ERROR_IF(getLocalPlayer() != NULL,
-                       "Player is local but there is already a local player");
-
-       Environment::addPlayer(player);
-}
+       FATAL_ERROR_IF(m_local_player != NULL,
+                       "Local player already allocated");
 
-LocalPlayer *ClientEnvironment::getLocalPlayer()
-{
-       for(std::vector<Player*>::iterator i = m_players.begin();
-                       i != m_players.end(); ++i) {
-               Player *player = *i;
-               if (player->isLocal())
-                       return (LocalPlayer*)player;
-       }
-       return NULL;
+       m_local_player = player;
 }
 
 void ClientEnvironment::step(float dtime)
@@ -2558,24 +2529,6 @@ void ClientEnvironment::step(float dtime)
                }
        }
 
-       /*
-               Stuff that can be done in an arbitarily large dtime
-       */
-       for (std::vector<Player*>::iterator i = m_players.begin();
-                       i != m_players.end(); ++i) {
-               Player *player = *i;
-               assert(player);
-
-               /*
-                       Handle non-local players
-               */
-               if (!player->isLocal()) {
-                       // Move
-                       player->move(dtime, this, 100*BS);
-
-               }
-       }
-
        // Update lighting on local player (used for wield item)
        u32 day_night_ratio = getDayNightRatio();
        {

diff --git a/src/environment.h b/src/environment.h
index f19708ad7e22346fa3e378729e6642b366ba3dc9..83ad69562375a76f00af988cfc8bd2105dee7f14 100644 (file)
--- a/src/environment.h
+++ b/src/environment.h
@@ -72,9 +72,6 @@ public:
 
        virtual Map & getMap() = 0;
 
-       virtual void addPlayer(Player *player);
-       void removePlayer(Player *player);
-
        u32 getDayNightRatio();
 
        // 0-23999
@@ -94,12 +91,6 @@ public:
        u32 m_added_objects;
 
 protected:
-       Player *getPlayer(u16 peer_id);
-       Player *getPlayer(const char *name);
-
-       // peer_ids in here should be unique, except that there may be many 0s
-       std::vector<Player*> m_players;
-
        GenericAtomic<float> m_time_of_day_speed;
 
        /*
@@ -325,6 +316,8 @@ public:
        void saveLoadedPlayers();
        void savePlayer(RemotePlayer *player);
        RemotePlayer *loadPlayer(const std::string &playername);
+       void addPlayer(RemotePlayer *player);
+       void removePlayer(RemotePlayer *player);
 
        /*
                Save and load time of day and game timer
@@ -520,6 +513,9 @@ private:
        // Can raise to high values like 15s with eg. map generation mods.
        float m_max_lag_estimate;
 
+       // peer_ids in here should be unique, except that there may be many 0s
+       std::vector<RemotePlayer*> m_players;
+
        // Particles
        IntervalLimiter m_particle_management_interval;
        UNORDERED_MAP<u32, float> m_particle_spawners;
@@ -579,8 +575,8 @@ public:
 
        void step(f32 dtime);
 
-       virtual void addPlayer(LocalPlayer *player);
-       LocalPlayer * getLocalPlayer();
+       virtual void setLocalPlayer(LocalPlayer *player);
+       LocalPlayer *getLocalPlayer() { return m_local_player; }
 
        /*
                ClientSimpleObjects
@@ -630,21 +626,15 @@ public:
 
        u16 attachement_parent_ids[USHRT_MAX + 1];
 
-       std::list<std::string> getPlayerNames()
-       { return m_player_names; }
-       void addPlayerName(std::string name)
-       { m_player_names.push_back(name); }
-       void removePlayerName(std::string name)
-       { m_player_names.remove(name); }
+       const std::list<std::string> &getPlayerNames() { return m_player_names; }
+       void addPlayerName(const std::string &name) { m_player_names.push_back(name); }
+       void removePlayerName(const std::string &name) { m_player_names.remove(name); }
        void updateCameraOffset(v3s16 camera_offset)
        { m_camera_offset = camera_offset; }
        v3s16 getCameraOffset() const { return m_camera_offset; }
-
-       LocalPlayer *getPlayer(const u16 peer_id);
-       LocalPlayer *getPlayer(const char* name);
-
 private:
        ClientMap *m_map;
+       LocalPlayer *m_local_player;
        scene::ISceneManager *m_smgr;
        ITextureSource *m_texturesource;
        IGameDef *m_gamedef;

diff --git a/src/localplayer.h b/src/localplayer.h
index 9d43128aab8ca57b4aa2e288d24d06b360fc4f83..eb727d7e3452da2c080939c993dae7ca86b9147b 100644 (file)
--- a/src/localplayer.h
+++ b/src/localplayer.h
@@ -38,11 +38,6 @@ public:
        LocalPlayer(Client *gamedef, const char *name);
        virtual ~LocalPlayer();
 
-       bool isLocal() const
-       {
-               return true;
-       }
-
        ClientActiveObject *parent;
 
        bool got_teleported;

diff --git a/src/player.h b/src/player.h
index 3c945b100eb56033ba04a23ac68eb85fa5bd24fb..6ac5dfe65ba4e53ef38929b9d46915ab346a2deb 100644 (file)
--- a/src/player.h
+++ b/src/player.h
@@ -183,22 +183,6 @@ public:
                return size;
        }
 
-       void setLocalAnimations(v2s32 frames[4], float frame_speed)
-       {
-               for (int i = 0; i < 4; i++)
-                       local_animations[i] = frames[i];
-               local_animation_speed = frame_speed;
-       }
-
-       void getLocalAnimations(v2s32 *frames, float *frame_speed)
-       {
-               for (int i = 0; i < 4; i++)
-                       frames[i] = local_animations[i];
-               *frame_speed = local_animation_speed;
-       }
-
-       virtual bool isLocal() const { return false; }
-
        bool camera_barely_in_ceiling;
        v3f eye_offset_first;
        v3f eye_offset_third;

diff --git a/src/remoteplayer.h b/src/remoteplayer.h
index f6c70b0e93eab3ea693f4c08f83df16623035804..1b1a90de3d7d6205bca20d19bea55d00ae246291 100644 (file)
--- a/src/remoteplayer.h
+++ b/src/remoteplayer.h
@@ -142,6 +142,20 @@ public:
                Player::setYaw(yaw);
        }
 
+       void setLocalAnimations(v2s32 frames[4], float frame_speed)
+       {
+               for (int i = 0; i < 4; i++)
+                       local_animations[i] = frames[i];
+               local_animation_speed = frame_speed;
+       }
+
+       void getLocalAnimations(v2s32 *frames, float *frame_speed)
+       {
+               for (int i = 0; i < 4; i++)
+                       frames[i] = local_animations[i];
+               *frame_speed = local_animation_speed;
+       }
+
        u16 protocol_version;
 private:
        /*