Feature/matchmaking#59
Conversation
…gnment and merging. - Refined types in custom-lobby-types.ts to match API requirements. - Added detailed matchmaking and character loadout types in vhs-the-game-types.ts. - Corrected lobby management logic in lobby-manager.ts for better consistency with matchmaking system."
…gnment and merging. - Refined types in custom-lobby-types.ts to match API requirements. - Added detailed matchmaking and character loadout types in vhs-the-game-types.ts. - Corrected lobby management logic in lobby-manager.ts for better consistency with matchmaking system."
…rashes. - Async Wrapper for Nedb (MongDB will come other time) - Forgotten Timestamp declaration. - Enum called LobbyAction to replace string literals for action types, reduces the risk of errors. - Improved logging messages for better debugging and monitoring.
LuisMayo
left a comment
LuisMayo
left a comment
There was a problem hiding this comment.
Hi!
Thanks a lot for the PR, those are a lot of good changes this project desperately needed. Also advancing a lot in the matchmaking feature
I've left some comments I'll like to address before merging.
Again, thanks a lot for your contributions, and sorry for taking so long to review.
Thanks.
| } catch(e) { | ||
| msg = 'Error while removing user ' + e; | ||
| // Method for Nedb | ||
| collection.remove({ [DBConstants.userIdField]: req.body.userId }, { multi: false }, (err, numRemoved) => { |
There was a problem hiding this comment.
Is there any reason for changing this from the async/await syntax into the callback one?
There was a problem hiding this comment.
The primary reason for switching to callback syntax is due to NeDB's native callback-based design, avoiding the extra overhead of wrapping those methods with promises unless necessary. With callbacks, the execution is immediate, and the function typically returns as soon as the operation is complete. (we are getting closer to VHS standarts)
There was a problem hiding this comment.
Giving this is a REST API (i.e: most of the overhead is the HTTP+TCP handshake) I doubt a microtask queuing (that's all the overhead a promise gives) is really relevant but it is an improvement, so thanks for doing it and for your patience explaining it
| interface GameSession { | ||
| sessionId: string; | ||
| players: string[]; // Array of player IDs in the session | ||
| state: string; // e.g., 'waiting', 'active', 'completed' |
There was a problem hiding this comment.
Not required for merging, but this could be an ENUM
There was a problem hiding this comment.
Yes, you're correct, I will make it as enum's
| // Initialize the matchmaking queue if necessary | ||
| const queue = await collection.findOneAsync({}); | ||
| if (!queue) { | ||
| await collection.insertAsync({ players: [] }); // Empty initial queue |
There was a problem hiding this comment.
I'm not sure I understand the design here, do we only have a queue?
There was a problem hiding this comment.
Initial testing for me, we can do more here like:
queueId: string; // Unique ID for the queue (game mode, region, etc.)
gameMode: string; // The game mode for the queue, e.g., 'ranked', 'casual'
region: string; // Region, e.g., 'NA', 'EU'
combineRegions: regions; // Combine NA and EU for more players
players: string[]; // Players waiting in the queue
This part can be deleted, because I was testing...
There was a problem hiding this comment.
Ok, knowing it's a test, it's good to merge. About your suggestion, we're not gonna use regions yet, also, a single region may have several queues (for instance, several monsters queue). I was hoping to have Matchmaking Sessions in the Array instead of players themselves
There was a problem hiding this comment.
Thinking more about this, I just don't think I want the matchmaking data on the database. It doesn't make sense because if a server restarts we're losing the websocket connections anyway, so all matchmaking info is irrelevant.
This is not blocking for merging the pull request because it can be deleted anyway
| return response.send({ | ||
| log: { logSuccessful: true }, | ||
| data: { | ||
| // TODO return the truth instead of this |
There was a problem hiding this comment.
Is there a reason to remove this comment?
There was a problem hiding this comment.
I mean I can't remember removing this ..
There was a problem hiding this comment.
Ok so if you agree I'll add it back
|
This will be good to merge once I redo the removed comment. Thanks again! |
|
Ok this branch doesn't compile so it won't be merge until compile errors are fixed |
2 participants
Here is all the changes made: