Skip to content
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

Ladders: Fix crash #8015

Merged
merged 5 commits into from
Feb 14, 2021
Merged

Ladders: Fix crash #8015

merged 5 commits into from
Feb 14, 2021

Conversation

mia-pi-git
Copy link
Member

A user namechanging before the battle ended in any way would break this.

Comment on lines 73 to 74
const p1id = p1 ? p1.id : toID(p1name);
const p2id = p2 ? p2.id : toID(p2name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just always toID(p1name)?

server/ladders-remote.ts Outdated Show resolved Hide resolved
server/ladders-remote.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@AnnikaCodes AnnikaCodes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@AnnikaCodes AnnikaCodes merged commit 3d820cf into smogon:master Feb 14, 2021
Comment on lines +176 to +178
/**
* Calculates Elo based on a match result
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original PR had a much more useful comment which seems to have been ripped out for no reason

const p2Act = (p1score > 0.9 || p1score < 0 ? `losing` : (p1score < 0.1 ? `winning` : `tying`));
let p2Reasons = `${p2NewElo - p2OldElo} for ${p2Act}`;
if (!p2Reasons.startsWith('-')) p2Reasons = '+' + p2Reasons;
room.addRaw(Utils.html`${p2name}'s rating: ${p2OldElo} &rarr; <strong>${p2NewElo}</strong><br />(${p2Reasons})`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You forgot to remove the old rating messages when you added these new messages in, so now messages are appearing twice.

/**
* Calculates Elo based on a match result
*/
private calculateElo(previousUserElo: number, score: number, foeElo: number): number {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This formula is wrong; it's generating different results from ntbb-ladder.php:

Kennedy ⌐◼_◼'s rating: 1000 → 1064
(+64 for winning)
eaubot's rating: 1157 → 1128
(-29 for losing)
Kennedy ⌐◼_◼'s rating: 1000 → 1057
(+57 for winning)
eaubot's rating: 1157 → 1121
(-36 for losing)

(gen8challengecup1v1 battle)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your duplicate result bug at least allowed us to catch this...

Zarel added a commit that referenced this pull request Mar 18, 2021
Caused by #8015 incorrectly copy/pasting only parts of #7960.

I also cleaned up the ladder fast-update code while I was working
on this.
Quanyails pushed a commit to Quanyails/Pokemon-Showdown that referenced this pull request May 12, 2021
* Ladders: Fix crash

* OK

* Update ladders-remote.ts

* Update server/ladders-remote.ts

Co-authored-by: Guangcong Luo <[email protected]>

* Update server/ladders-remote.ts

Co-authored-by: Guangcong Luo <[email protected]>
Quanyails pushed a commit to Quanyails/Pokemon-Showdown that referenced this pull request May 12, 2021
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants