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

Add support for PaperServerListPingEvent #880

Merged
merged 8 commits into from Apr 18, 2018

Conversation

Projects
None yet
6 participants
@Minecrell
Contributor

Minecrell commented Mar 26, 2018

PaperServerListPingEvent is an extended version of Bukkit's standard ServerListPingEvent, but allows full control over the status response. This makes it possible to change the player sample, modify the server version and many other things.

Until now, this was only possible by using ProtocolLib to modify the packets directly. With this event, it can be also done on Glowstone where ProtocolLib isn't working (yet).

TODO before merging

Right now, the PR doesn't compile anymore as-is. The remaining changes (unrelated to the new event) need to be made by someone more familiar with the Glowstone implementation:

  • Needs an upstream merge in Glowkit, depends on GlowstoneMC/Glowkit#18
  • Glowstone needs to be updated for new methods added in Bukkit/Spigot/Paper. There seem to be several new methods in the Paper API that need to be implemented.
  • Depends on #881

Test plugin

The new event is supported in the development builds of ServerListPlus.
Download ServerListPlus-3.4.9-SNAPSHOT-Universal.jar from Jenkins, all features should be working.

public InetSocketAddress getVirtualHost() {
return session.getVirtualHost();
}

This comment has been minimized.

@momothereal

momothereal Mar 26, 2018

Member

Remove whitespace

import org.bukkit.util.CachedServerIcon;
import org.json.simple.JSONArray;
import org.json.simple.JSONObject;

public final class StatusRequestHandler implements
MessageHandler<GlowSession, StatusRequestMessage> {

private static final UUID FAKE_UUID = new UUID(0, 0);

This comment has been minimized.

@momothereal

momothereal Mar 26, 2018

Member

Change to BLANK_UUID

// Add player sample list
List<Player> playerList = new ArrayList<>(server.getOnlinePlayers());
int count = Math.min(playerList.size(), server.getPlayerSampleCount());
Collections.shuffle(playerList);

This comment has been minimized.

@momothereal

momothereal Mar 26, 2018

Member

Is shuffling really necessary? I understand if that's Vanilla behavior, but it seems like to me like an unnecessary performance hit (especially with a large player count)

This comment has been minimized.

@Minecrell

Minecrell Mar 26, 2018

Contributor

The main purpose here is to avoid showing the same players all the time. This could be simplified more, but then it would be less random. Up to you.

This comment has been minimized.

@Pr0methean

Pr0methean Apr 7, 2018

Contributor

Implement a shuffle that stops choosing players when you have count.

This comment has been minimized.

@Minecrell

Minecrell Apr 8, 2018

Contributor

What does "when you have count" mean?

This comment has been minimized.

@Pr0methean

Pr0methean Apr 9, 2018

Contributor

I mean, if server.getPlayerSampleCount() < playerList.size(), then only choose (and only copy) server.getPlayerSampleCount() random elements and then stop building the shuffled copy. That way, the running time, and the memory the shuffled copy consumes, will stop increasing linearly with playerList.size() once they reach the sample size.

Add support for PaperServerListPingEvent
PaperServerListPingEvent is an extended version of Bukkit's standard
ServerListPingEvent, but allows full control over the status response.
This makes it possible to change the player sample, modify the server
version and many other things.

Until now, this was only possible by using ProtocolLib to modify
the packets directly. With this event, it can be also done on
Glowstone where ProtocolLib isn't working (yet).
@mastercoms

This comment has been minimized.

Member

mastercoms commented Apr 6, 2018

I'll try to update Glowkit soon, but I would appreciate if someone else could do it as I'll be pretty busy for the next week or so.

// Add player sample list
List<Player> playerList = new ArrayList<>(server.getOnlinePlayers());
int count = Math.min(playerList.size(), server.getPlayerSampleCount());
Collections.shuffle(playerList);

This comment has been minimized.

@Pr0methean

Pr0methean Apr 7, 2018

Contributor

Implement a shuffle that stops choosing players when you have count.

@Minecrell

This comment has been minimized.

Contributor

Minecrell commented Apr 10, 2018

I've optimized the code a bit to avoid shuffling the entire list if only a few players are needed. However, the copy of the entire player list is still needed because we need to swap entries and access them by their index.

@smartboyathome

This comment has been minimized.

Contributor

smartboyathome commented Apr 10, 2018

Unfortunately, the algorithm in your most recent change seems to bias against the beginning of the list, since the swap happens to the first k members, where k is the number of players you are selecting. In addition, if the number of players is less than k, you don't need to randomize them (since the whole point of the randomization is to try to return a different subset).

You should look into using something like the Reservoir Sampling here. This algorithm should allow you to randomly select items from the list without using any form of shuffle. Instead, it weights probabilities over the iteration of the players list, running in O(n) time (where n is the number of players). It can be done without an array copy due to this.

@Minecrell

This comment has been minimized.

Contributor

Minecrell commented Apr 10, 2018

Unfortunately, the algorithm in your most recent change seems to bias against the beginning of the list, since the swap happens to the first k members, where k is the number of players you are selecting.

How does the swap to the first k members bias it to the beginning of the list?

The algorithm moves the randomly chosen entries to the beginning of the list, and chooses further entries only from the remaining list to avoid duplicates. The entire list is considered when choosing entries.

Essentially, it is a partial iteration of the Fisher-Yates shuffle, stopping when we have chosen enough entries.

In addition, if the number of players is less than k, you don't need to randomize them (since the whole point of the randomization is to try to return a different subset).

Technically not, but IMO it would be a bit weird if the players are randomly sorted when players.size() > sampleCount but not if you have less players online. The overhead in that case is probably rather low anyway.

You should look into using something like the Reservoir Sampling here. This algorithm should allow you to randomly select items from the list without using any form of shuffle. Instead, it weights probabilities over the iteration of the players list, running in O(n) time (where n is the number of players). It can be done without an array copy due to this.

I'm not too concerned about the copy of the players list personally. It won't be that large usually. If you'd like to change it, feel free to add more commits here or make a PR on my fork.

@Pr0methean

Please make sure CircleCI passes before submitting! It looks like we're still using a version of Paper that doesn't define PaperServerListPingEvent; you'll probably need to update the version in pom.xml.

@Pr0methean

This comment has been minimized.

Contributor

Pr0methean commented Apr 10, 2018

@smartboyathome Minecrell is right: his algorithm is unbiased, and its running time is linear in the sample size.

@Minecrell Since GlowServer.getOnlinePlayers already makes a defensive copy, we can just use Arrays.asList.

Pr0methean added some commits Apr 10, 2018

private static final UUID BLANK_UUID = new UUID(0, 0);

private static void choosePlayerSample(GlowServer server, PaperServerListPingEvent event) {
ThreadLocalRandom random = ThreadLocalRandom.current();

This comment has been minimized.

@Pr0methean

Pr0methean Apr 10, 2018

Contributor

Check shouldHidePlayers() first?

This comment has been minimized.

@Minecrell

Minecrell Apr 10, 2018

Contributor

This would be nice, but this method is run before the event is called so it would be never set at this point. The only way to optimize this would be to override getPlayerSample() on the event and lazily initialize the sample list, however shouldHidePlayers() will likely not be used often so this isn't quite worth optimizing.

@Minecrell

This comment has been minimized.

Contributor

Minecrell commented Apr 10, 2018

Please make sure CircleCI passes before submitting! It looks like we're still using a version of Paper that doesn't define PaperServerListPingEvent; you'll probably need to update the version in pom.xml.

Yeah, this is mentioned in the PR description. Updating Glowkit needs to be done separately before this PR can be merged. The reason I haven't included that in this PR is that there were many new methods added unrelated to the event implementation (e.g. in the PlayerProfile API). To test my changes locally, I have only added the stub methods to get everything to compile.

If it helps I could submit a PR for Glowkit to merge the Glowkit patches on top of the current Paper API. However, unless you want to merge the stub methods someone else with more experience with the Glowstone implementation will need to implement them.

private static void choosePlayerSample(GlowServer server, PaperServerListPingEvent event) {
ThreadLocalRandom random = ThreadLocalRandom.current();

List<Player> players = Arrays.asList(server.getOnlinePlayers());

This comment has been minimized.

@Minecrell

Minecrell Apr 10, 2018

Contributor

In newer Bukkit versions (since 1.7.10 I think), Server.getOnlinePlayers() no longer returns an array but rather a view of the online players. Therefore, this will cause a compile error.

Revert "Use Arrays.asList"
This reverts commit 0c3a50a.
@Pr0methean

This comment has been minimized.

Contributor

Pr0methean commented Apr 10, 2018

The Glowkit PR sounds fine to me. Could you please put all the stubs in a separate commit, and once they're merged, open an issue linking to the commit?

@Minecrell Minecrell referenced this pull request Apr 11, 2018

Merged

Upstream merge #18

@Minecrell

This comment has been minimized.

Contributor

Minecrell commented Apr 11, 2018

@Pr0methean Done. CircleCI should compile and might pass the tests now once GlowstoneMC/Glowkit#18 is merged (otherwise it can't find the updated Glowkit artifact). For some reasons some of the tests are failing with unrelated NPEs locally (even on the main dev branch) so I'm not entirely sure.

@Minecrell

This comment has been minimized.

Contributor

Minecrell commented Apr 11, 2018

@Pr0methean Build seems to pass now, yay! :)

@Override
public UUID getId() {
return uniqueId == null ? null : uniqueId.getNow(null);
}

@Override
public UUID setId(@Nullable UUID uuid) {

This comment has been minimized.

@yannicklamprecht

yannicklamprecht Apr 16, 2018

I wouldn’t recommend setting the UUID after GameProfile creation. That could lead to unpleasant behavior.

Player#getGameProfile#setId could be called at any time. Blame me if I’m wrong.

This comment has been minimized.

@yannicklamprecht

yannicklamprecht Apr 16, 2018

The gameprofile is like an identity card. Setting the UUID is comparable to changing the ID of your IDC after creation.

This comment has been minimized.

@Minecrell

Minecrell Apr 16, 2018

Contributor

This is a method added upstream by Paper: https://ci.destroystokyo.com/userContent/apidocs/com/destroystokyo/paper/profile/PlayerProfile.html#setId-java.util.UUID-

If I remember correctly, the Paper implementation usually returns copies to avoid this problem. It doesn't matter yet because this is just a stub, and won't be implemented as part of this PR. (I've only added it so the code compiles correctly)


@Override
public void setPlayerProfile(PlayerProfile playerProfile) {
throw new UnsupportedOperationException("Not implemented yet.");

This comment has been minimized.

@yannicklamprecht

yannicklamprecht Apr 16, 2018

This method should ensure that only GameProfiles with the same UUID can be set, Else throw an exception.
Modification of the players UUID shouldn’t be allowed after creation.

This comment has been minimized.

@Minecrell

Minecrell Apr 16, 2018

Contributor

Again unrelated to this PR (since this is just a stub), but it is in fact designed to make it easy to change a player's identity. It is specifically designed to make it possible to change the identity of the player, or something like that. (Not entirely sure, see https://ci.destroystokyo.com/userContent/apidocs/org/bukkit/entity/Player.html#setPlayerProfile-com.destroystokyo.paper.profile.PlayerProfile-)

@Pr0methean Pr0methean merged commit 45c4a5e into GlowstoneMC:dev Apr 18, 2018

2 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
license/cla Contributor License Agreement is signed.
Details

@Minecrell Minecrell deleted the Minecrell:paper-slp-event branch Apr 19, 2018

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