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

Server response to legacy ping (fe and fe01) packets #334

Merged
merged 7 commits into from
Feb 1, 2016

Conversation

deathcap
Copy link
Member

Implements server-side support for #332 FE legacy ping support

Supports type 0 (fe) and type 1 (fe01) pings, testing with https://github.com/deathcap/node-minecraft-ping:

node-minecraft-protocol $ NODE_DEBUG=mc-proto node examples/server/server.js
node-minecraft-ping $   node demo.js localhost 25565
received ping_fe01fa { pingVersion: 1,
  protocolVersion: 47,
  gameVersion: '1.8.8',
  motd: 'Vox Industries',
  playersOnline: 0,
  maxPlayers: 12 }
received ping_fe { pingVersion: 0,
  motd: 'Vox Industries',
  playersOnline: 0,
  maxPlayers: 12 }
received ping_fe01 { pingVersion: 1,
  protocolVersion: 47,
  gameVersion: '1.8.8',
  motd: 'Vox Industries',
  playersOnline: 0,
  maxPlayers: 12 }

Some notes about issues/limitations with this pull request:

  • The deserializer cannot parse legacy ping packets, because of the missing varint packet type header, so I worked around by having the splitter prepend a varint packet id. Ideally minecraft-data could specify how to parse these packets: Legacy ping protocol minecraft-data#95 but idk how
  • minecraft-data specifies server_legacy_list_ping as requiring a ubyte payload, but for ping type 0 it is not, so I fill in a \0 in the ping payload to avoid deserialization errors. Also would need changes to Legacy ping protocol minecraft-data#95
  • Serializer is not used to write the ping responses (or the ping requests, which aren't supported in this PR) for the same reason; client.writeRaw() is also unsuitable because it prepends length, so I use client.socket.write(). Maybe this could be refactored further, a new "legacy" packet state, which doesn't have length varints? Could be revisited later if/once pre-Netty protocol support is added.
  • Node.js has utf16le support in Buffer but not utf16be (UCS2 big endian encoding for buffer nodejs/node-v0.x-archive#1684); encoded hackishly updated to use native utf16le + swap with https://github.com/substack/endian-toggle, works well
  • No client support for sending legacy ping requests or parsing legacy ping responses (can use n-mc-ping, but would be nice to have in nmp)

but it is functional

@deathcap deathcap changed the title [WIP] Server response to legacy ping (fe and fe01) packets Server response to legacy ping (fe and fe01) packets Jan 31, 2016
deathcap added a commit to deathcap/node-minecraft-protocols that referenced this pull request Feb 1, 2016
PrismarineJS/node-minecraft-protocol#334 Server response to legacy ping (fe and fe01) packets
@@ -51,6 +51,7 @@ class Client extends EventEmitter
this.deserializer = createDeserializer({ isServer:this.isServer, version:this.version, state: state, packetsToParse:
this.packetsToParse});

this.splitter.recognizeLegacyPing = state === states.HANDSHAKING;
Copy link
Member

Choose a reason for hiding this comment

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

isn't state === states.HANDSHAKING always true at that point ?

edit:nevermind

@rom1504
Copy link
Member

rom1504 commented Feb 1, 2016

This is kind of a hack, but well, so is that packet. It would be ideal to handle this kind of thing properly directly in protodef, but let's do that later in #332

rom1504 added a commit that referenced this pull request Feb 1, 2016
Server response to legacy ping (fe and fe01) packets
@rom1504 rom1504 merged commit bf029d3 into PrismarineJS:master Feb 1, 2016
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.

2 participants