Mc/matchmaking poc1 #339
Mc/matchmaking poc1 #339
Conversation
Please, remove pubnub-matchmaking-kotlinOld module after you rewrite logic that is inside of it.
…currency is handled by coroutines.
Last message in a user-related channel will be pointing to current status.
| import com.pubnub.kmp.PNFuture | ||
|
|
||
| // todo add kdoc | ||
| interface User { |
There was a problem hiding this comment.
it's concerning that we're copying and creating yet another implementation of User, this time in com.pubnub.matchmaking. If someone uses Chat SDK are they going to have two incompatible User classes? Should this SDK be an additon to Chat SDK? Or maybe User and Channel should be in some base module, used by both Chat and Matchmaking?
this could use some more thought to find the best solution
| package com.pubnub.matchmaking.entities | ||
|
|
||
| enum class MatchmakingStatus { | ||
| IN_QUEUE, |
There was a problem hiding this comment.
what can a user do with all those statuses?
for example, why do we have an UNKNOWN status? we control this code, so shouldn't all of them be known?
what's the difference between IN_QUEUE and RE_ADDED_TO_QUEUE (from user's perspective)?
can we figure out the minimum list of statuses to make this work?
| package com.pubnub.matchmaking.entities | ||
|
|
||
| class FindMatchResult( | ||
| val result: String, |
There was a problem hiding this comment.
String? what's inside?
|
|
||
| fun deleteUser(id: String, soft: Boolean = false): PNFuture<User?> | ||
|
|
||
| fun findMatch(userId: String): PNFuture<String> |
There was a problem hiding this comment.
this seems overly complicated, couldn't the whole API be just one method -
@Throws(MatchMakingException::class)
fun findMatch(forUserId: String): PNFuture<MatchResult>
where MatchResult contains the set of users that were matched or the ID of the game or some other means of starting the game or confirming the match?
in case of error you would get an exception with details of the error, in case of success you get the result
why would I want to observe statuses like IN_QUEUE, RE_ADDED_TO_QUEUE etc, it's not useful?
| PubNubException("Id is required").asFuture() | ||
| } else { | ||
| PNFuture { callback -> | ||
| CoroutineScope(Dispatchers.Default).launch { |
There was a problem hiding this comment.
don't create a new scope, just use the scope from the MatchMakingRestService object
| try { | ||
| while (true) { | ||
| processMatchmakingQueue() | ||
| delay(5000L) // Wait 5 seconds between processing |
There was a problem hiding this comment.
why the 5 second delay? and why 5 seconds?
| delay(5000L) // Wait 5 seconds between processing | ||
| } | ||
| } finally { | ||
| processingQueueInProgress = false |
There was a problem hiding this comment.
this will never execute because of the above while loop, but if it did it's not guarded by Mutex (all other access to this variable is guarded)
| } | ||
|
|
||
| // Create all possible pairs (only including allowed pairs) | ||
| private fun createAllPairs(users: List<User>): List<Triple<Int, Int, Double>> { |
There was a problem hiding this comment.
this List<Triple<Int,Int,Double>> is unreadable (it's difficult to say what these ints and doubles represent), consider a better return type
|
|
||
| val eloDifference = abs(eloA - eloB) | ||
| if (eloDifference > maxEloGap) { | ||
| return Double.POSITIVE_INFINITY |
There was a problem hiding this comment.
wouldn't null instead of Double.POSITIVE_INFINITY be more appropriate in this case?
|
|
||
| // Greedy pairing: sort by score and select pairs without conflicts. | ||
| // Instead of returning MatchmakingPair, we create a MatchGroup that holds a set of users. | ||
| private fun pairUsersBySkill(users: List<User>): MatchmakingResult { |
There was a problem hiding this comment.
in general I think this algorithm is wasteful:
- why create all pairs when we're only going to use some of them?
- and why do it every 5 seconds, even if nothing has changed (no new users are requesting a match)?
instead of running a loop that checks all possibilities every X seconds, I'd explore something more event-based, like:
- there is some
List<MatchGroup>that is initially empty - a user with some
userIdandskillcallsfindMatch() findMatchiterates over the list of open match groups (i.e. matches that have not started because they don't have the required number of players) and if theskillmatches the group'sskilllevel (due to some rules), add the user to the group- if the group has the necessary number of players, inform all users in the group that they have a match (using the callback, or in REST by returning HTTP 200)
- if there is no group that matches the player's skill, open a new group with this user and add it to the list of open matches (HTTP is in progress until the group is complete)
So everything would happen in response to a new player requesting a match. The only timer that I'd add would be to support some sort of timeout if the player has been waiting for too long to return with a "no match found" error or something like that.
…otlinx.coroutines.channels.Channel receive/send functionality
| } | ||
| if (groupToJoin != null) { | ||
| // Join the found group. | ||
| groupToJoin!!.users.add(user) |
There was a problem hiding this comment.
reorganize the code to avoid using "!!", for example:
remove the groupToJoin variable, do instead: openMatchGroups.firstOrNull { ... }?.let { groupToJoin -> ... } ?: run { // group not found }
There was a problem hiding this comment.
or just make it a val instead of var so you can use smart cast
| private val openMatchGroups = mutableListOf<OpenMatchGroup>() | ||
| private val groupsMutex = Mutex() | ||
|
|
||
| @Throws(MatchMakingException::class) |
There was a problem hiding this comment.
this doesn't need @throws if it returns PNFuture because the exception is delivered inside the future
No description provided.