Improvements to Sync Service #500
Conversation
… to private because they shouldn't be accessible to outsiders. And most importantly, improved the sync service's performance by giving it only players' entities.
broxen
added
enhancement
|
Smart pointers would probably be a good idea, yes. 👍 |
| ACE_Guard<ACE_Thread_Mutex> guard_buffer(ref_entity_mgr.getEntitiesMutex()); | ||
|
|
||
| for(Entity * e : ref_entity_mgr.m_live_entlist) | ||
| for(int i = 0; i < m_entities.size(); ++i) |
There was a problem hiding this comment.
Nit: Why not a range based for-loop anymore?
There was a problem hiding this comment.
i < size/length/count whatever is what i nearly always use :P
|
Not at this point though, all Entity instances are be 'owned' only at a single point, so making those shared_ptr would muddle things a bit, I would even consider making most of |
| EntityManager& ref_entity_mgr; | ||
| uint8_t m_game_server_id; | ||
| int m_update_interval = 5; | ||
| QVector<Entity *> m_entities; |
There was a problem hiding this comment.
I'd very much prefer we don't store Entity pointers here, since they can change ( underlying memory is owned by EntityManager, and is part of a std::vector)
|
So I suppose we should make this EntityHandle class that uses the EntityManager's m_store... That makes sense because we'd want the entity to be put into m_free_entries back like some sort of 'entities pool'. The way I look at it, just having access to m_store is not enough, so should we make EntityHandler have a reference instead of a pointer to Entity, and if we have pointers, would the memory no longer be shared between it and EntityManager? |
MapInstance does the serialization part SyncService did, SyncService simply reposts the message to the db handler. Added a function in ClientManager.h to get all active sessions inside it.
|
Ready for merge. Testing is okay with 2 characters on map. Seems I have to re-create the message in sync service because the original message probably got 'muddied' by being sent as a SEGSEvent to the sync service, and then casted back to a Character/PlayerUpdateMessage. |
|
Works great!
|
|
@nemerle I'll leave this to you to merge when you've had a chance to review. |
| vClients get_active_sessions() | ||
| { | ||
| return m_active_sessions; | ||
| } |
There was a problem hiding this comment.
If the users of this function do not need to modify the active sessions ( the result is only used to find things ), I'd prefer us to use this form here:
const vClients &get_active_sessions() const { return m_active_sessions; }
| void MapInstance::on_update_entities() | ||
| { | ||
| std::vector<MapClientSession *> active_sessions = m_session_store.get_active_sessions(); | ||
|
|
There was a problem hiding this comment.
no need to copy the active_sessions vector here, if get_active_sessions returns reference:
const std::vector<MapClientSession *> &active_sessions(m_session_store.get_active_sessions())
| // all active sessions are for player, so we don't need to verify if db_id != 0 | ||
| for (size_t i = 0; i < active_sessions.size(); ++i) | ||
| { | ||
| Entity *e = active_sessions[i]->m_ent; |
There was a problem hiding this comment.
We could be using range-for here ?
There was a problem hiding this comment.
this is my huge preference for normal for-loops at work here :P
There was a problem hiding this comment.
Just a side-note, for performance work, compilers can have trouble optimizing plain for loops if the condition part has a function call in it ( like size() call above )
There was a problem hiding this comment.
I see... I'll keep that in mind in the future. Thanks for the heads-up.
On another note, I just made the changes as advised. Should be okay now.
| @@ -63,6 +63,11 @@ mutable std::mutex m_store_mutex; | |||
| } | |||
|
|
|||
| public: | |||
| const vClients get_active_sessions() | |||
| { | |||
| return m_active_sessions; | |||
There was a problem hiding this comment.
const vClients & plz, this still will return a copy of the vector :)
There was a problem hiding this comment.
Oops I forgot to look at the ampersand, updated again
Closes #498 .
Removed unused variables and functions, moved some of the public ones to private because they shouldn't be accessible to outsiders. And most importantly, improved the sync service's performance by giving it only players' entities.
So far it seems alright in testing, but as always - test again to ensure it's good for merging.
Some note: I cannot delete the pointer to entity because entity's destructor is private. Perhaps we should use a vector of smart pointers instead?