-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Calculate and display Elo change on server #7960
Conversation
const ladderUpdatePromise = LoginServer.request('ladderupdate', { | ||
p1: p1name, | ||
p2: p2name, | ||
score: p1score, | ||
format: formatid, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this just be moved down to where await ladderUpdatePromise
is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows the loginserver's processing to happen concurrently with the server running and displaying the Elo changes. It's admittedly a small optimization, given that the Elo calc doesn't take much time; up to you if you want to change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see! I think this is fine.
* Calculates Elo for quick display, matching the formula on loginserver | ||
*/ | ||
// see lib/ntbb-ladder.lib.php in the pokemon-showdown-client repo for the login server implementation | ||
// *intentionally* different from calculation in ladders-local, due to the high activity on the main server |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, in hindsight I think the only difference is because of ladder decay, which isn't implemented here.
Is the only difference here the 1300
vs the 1350
? Maybe we should merge them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not the only difference; the numbers for scaling when elo < 1200 are also different.
ladders-local:
if (score < 0.5) {
K = 10 + (previousUserElo - 1000) * 40 / 200;
} else if (score > 0.5) {
K = 90 - (previousUserElo - 1000) * 40 / 200;
}
ladders-remote:
if (score < 0.5) {
K = 20 + (previousUserElo - 1000) * 30 / 100;
} else if (score > 0.5) {
K = 80 - (previousUserElo - 1000) * 30 / 100;
}
ladders-remote uses 20/80 instead of 10/90, and multiplies by 30 instead of 40.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting! We probably want to investigate the formulas more, later. But that's probably a job for a different PR.
Co-authored-by: Guangcong Luo <[email protected]>
This is great! Thanks! |
This reverts commit b87dd71.
…ogon#8014) This reverts commit b87dd71.
Caused by smogon#8015 incorrectly copy/pasting only parts of smogon#7960. I also cleaned up the ladder fast-update code while I was working on this.
ladders-local.ts
already had the calculations for Elo change; I factored that out into a separate file that's called from bothladders-local
andladders-remote
.ladders-remote
now calculates Elo change immediately and displays it while waiting for the login server to return from updating the ladder.Fixes relevant bullet point on #2444.