Skip to content

Conversation

@Cryptite
Copy link
Contributor

This exposes most recent ping in milliseconds on a Player. The default Player.getPing() represents an average over time, but it is very nice to be able to actually see their most recent ping.

@Cryptite Cryptite requested a review from a team as a code owner January 15, 2026 03:24
@github-project-automation github-project-automation bot moved this to Awaiting review in Paper PR Queue Jan 15, 2026
@SirYwell
Copy link
Contributor

Would it make sense to return Duration instead? And also keep the nanosecond resolution then, as Duration makes it easy to convert to milliseconds as needed.

@Cryptite
Copy link
Contributor Author

Cryptite commented Jan 15, 2026

It would seem weird for that to be a Duration while getPing() right next to it is in ms, plus ms seems to be the generally accepted resolution on "the internets." But it's easy enough to change if there's more demand for it I suppose.

I just assume that even if it was a Duration, the absolute majority use-case would be people converting it to milliseconds.

Copy link
Contributor

@lynxplay lynxplay left a comment

Choose a reason for hiding this comment

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

We hardforked, no need for comments.

@lynxplay lynxplay moved this from Awaiting review to Waiting For Author in Paper PR Queue Jan 16, 2026
Copy link
Member

@Owen1212055 Owen1212055 left a comment

Choose a reason for hiding this comment

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

This should not be on Player, rather the PlayerCommonConnection.
Also include some of the other ping stuff.

@github-project-automation github-project-automation bot moved this from Waiting For Author to Changes required in Paper PR Queue Jan 16, 2026
@Cryptite
Copy link
Contributor Author

This should not be on Player, rather the PlayerCommonConnection. Also include some of the other ping stuff.

Doing a cursory investigation of this this morning, moving the existing latency/keepalive stuff here would turn this into a not-very-trivial PR. If this is quite desired I can try it but to relocate the existing ping stuff down into PaperCommonConnection requires moving quite a bunch of stuff around which I can do but am less comfortable handling.

@electronicboy
Copy link
Member

What you mean is that you probably want to rebase onto the files PR to do the change provisionally against the vanilla keepalive impl, and then reapply all of the feature patches and make the tweak to the keepalive improvement patch

@Cryptite
Copy link
Contributor Author

What you mean is that you probably want to rebase onto the files PR to do the change provisionally against the vanilla keepalive impl, and then reapply all of the feature patches and make the tweak to the keepalive improvement patch

This was done already with Owen/Lynxplay's help yesterday, I was just referring to Owen's request to also relocate ping related logic.

@electronicboy
Copy link
Member

electronicboy commented Jan 17, 2026

They mean to promote the few existing ping related methods in Player up to the connection API class, not sure if there is some invisible complexity (not looked at specifics) there or you misread into something?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Changes required

Development

Successfully merging this pull request may close these issues.

5 participants