-
Notifications
You must be signed in to change notification settings - Fork 184
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 more packets (and minor fixes) #68
Conversation
…f.data is negative
Thanks a lot for your work! Will review this when I'm free, and I'd appreciate a look from you as well @joodicator |
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.
Looks really good overall, thanks again for your work.
minecraft/networking/connection.py
Outdated
packet_data.send(decompressed_packet) | ||
packet_data.reset_cursor() | ||
rawPacketData = None | ||
try: |
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.
I'm not convinced this is the right way to handle this, can you provide more information about when this problem happens? What server with what plugin configuration?
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.
Unfortunately I don't know anything in specific about the plugins used nor the server version (afaik they use 1.8.8).
In general this faulty behaviour can be reproduced on play(.)saicopvp(.)com
I did some further investigation and the server claims to support the following protocol versions:
[47, 107, 108, 109, 110, 210, 301, 302, 303, 304, 305, 306, 307, 308, 309, 310, 311, 312, 313, 314, 315, 316, 317, 318, 319, 320, 321, 322, 323, 324, 325, 326, 327, 328, 329, 330, 331, 332, 333, 334, 335, 336, 337, 338]
However when connecting with self.context.protocol_version = max(self.allowed_proto_versions)
the client doesn't do anything.
After some debugging I found out that the server sends the following: 0x1A 0x1A DisconnectPacketPlayState {'json_data': '"Unsupported protocol version 335"'}
or similar depending on the version the client connects with.
In general this is messed up from the start already. The server claims to support this specific version and when connecting it sends a DisconnectPacketPlayState-Packet.
Regardless for my testing I adjusted the way how the version is determined by using:
self.context.protocol_version = min(self.allowed_proto_versions)
this will result in:
File "~/pyCraft/minecraft/networking/connection.py", line 440, in read_packet decompressed_packet = decompress(packet_data.read()) zlib.error: Error -5 while decompressing data: incomplete or truncated stream
If I add output about the decompressed_size
its a non-zero value and dynamic for each packet. this it will try to decompress the packet and fail with the previous error.
I can't reproduce this in any way with any paper spigot / spigot server with ViaVersion.
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.
Firstly, it does indeed appear that the saicopvp server is misconfigured or running a multiple-protocol-support plugin with bugs. It advertises the capability to use protocol 338 (1.12.1), but as you say rejects with an error message. It does accept connections using protocol 316 (1.11.2) and earlier. My suggested workaround would be to configure pyCraft to connect using that specific version, until their issues are fixed, like so:
from minecraft.networking.connection import Connection
conn = Connection(..., allowed_versions={'1.11.2'}) # allowed_versions={316} would also be accepted.
conn.connect()
Secondly, it seems to me that the zlib errors are due to improperly encoded compressed packets being sent by the server. Examining the compressed sections with infgen shows that they consist of a zlib header, followed by an incomplete deflate stream, and not including a zlib footer which would be used to perform error-checking. Despite this, it is possible to obtain a partial output by decoding this whose length matches the decompressed length indicated in each packet, and constituting a valid Minecraft packet, and it seems that this is what Mojang's client does, as it seems to successfully read some packets (Player List Item) that made pyCraft crash at the time of this pull request.
On the other hand, treating the data which failed to compress as an uncompressed packet, as this pull request implements, does not solve the problem, as the result cannot be parsed as a Minecraft packet. It might appear to solve the problem, as the byte giving the packet ID is unrecognised and silently ignored by pyCraft, but this means that the packet is lost, and could cause an exception if this byte happens to be a recognised packet ID.
Considering the behaviour of Mojang's client, I think it would be reasonable for pyCraft to decode the compressed packets in a more lenient fashion and accept truncated zlib streams as long as the decompressed output has the correct length. If the stream is not truncated, error-checking will still occur, so nothing much is lost. It can be implemented like this, using a zlib.decompressobj
instead of zlib.decompress()
:
import zlib
# ...
if self.connection.options.compression_enabled:
decompressed_size = VarInt.read(packet_data)
if decompressed_size > 0:
decompressor = zlib.decompressobj()
decompressed_packet = decompressor.decompress(packet_data.read())
# decompressor.eof may be False, if we have read a truncated stream,
# but we will allow it provided that the following assertion holds.
assert len(decompressed_packet) == decompressed_size, \
'decompressed length %d, but expected %d' % \
(len(decompressed_packet), decompressed_size)
packet_data.reset()
packet_data.send(decompressed_packet)
packet_data.reset_cursor()
minecraft/networking/types.py
Outdated
return {'x': x, 'y': y, 'z': z} | ||
|
||
@staticmethod | ||
def send(value, socket): |
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.
I would suggest changing this to pass either the x, y, z as arguments or passing a dict containing x, y, z. The types classes are supposed to abstract away the python types and sending them over the network and calling an encode method before sending breaks that a little.
minecraft/networking/packets.py
Outdated
self.blockId = (blockData >> 4) | ||
self.blockMeta = (blockData & 0xF) | ||
|
||
def write(self, socket, compression_threshold=None): |
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.
I would suggest implementing this so this is easier to test. See this commit for an example: d686b64
minecraft/networking/packets.py
Outdated
% type_id) | ||
return subcls | ||
|
||
class Type_Boat(Type): |
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.
I think you followed the example from the PlayerListItemPacket
for this, the reason it has this subclassing is because the packet varies depending on the type of Action. In this case, the type doesn't vary so you can forego this and simply introduce an enum like thing that looks like:
class EntityType:
BOAT = 1
ITEM_STACK = 2
# ...
by_id = {id: entity for (entity, id) in EntityType.__dict__.items() if entity.isupper()}
@staticmethod
def get_entitity_type_by_id(id):
return self.by_id[id]
@joodicator So looking at this especially, I think |
@ammaraskar thanks for the feedback / input and I'll def look into this further. What I also would suggest is some sort of common pattern for client bound packets and server bound packets. |
…to abstract away the python types and sending them over the network and calling an encode method before sending breaks that a little.
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.
Thank you for your contribution. In addition to my other comments, I have some general points:
-
The packet IDs of Entity Velocity (0x3E), Update Health (0x41), Combat Event (0x2D), and Client Status (0x03) changed in protocol 336 (snapshot 17w31a), but are not reflected in this pull request. Since pyCraft is declared to support up to Minecraft 1.12.1, those packet IDs should be included.
I have been working on a web page that displays packet IDs scraped from wiki.vg that you might find useful for seeing per-version changes at a glance: https://joodicator.github.io/mc-dev-data/. I can't guarantee that it is completely free of errors -- either my own or those existing on the wiki -- so due caution is recommended (and please open an issue if you notice any errors).
-
I appreciate the need for a better naming scheme for packets. Hopefully we may agree on such a scheme in relation to my next point below. But, in any case, the names of the packets implemented in this pull request need to be consistent with the other packet names, to avoid confusion and to be more forward-compatible. Currently, they all end in
Packet
, possibly followed byClientbound
,Serverbound
orPlayState
. Therefore,ServerClientStatus
would properly beClientStatusPacketServerbound
, for example. -
@ammaraskar Perhaps we could separate the packets into sub-modules based on whether they are serverbound or clientbound, and in what connection state they belong. This may also allow us to deal with the problem of packet names not being clear about this. It could be structured like this:
minecraft.networking.packets
clientbound
handshake
login
play
status
serverbound
handshake
login
play
status
# packets/clientbound/login.py
from minecraft.networking.packets import Packet
# Formerly known as state_login_clientbound.
def get_packets(context):
packets = {
DisconnectPacket,
# ...
}
return packets
class DisconnectPacket(Packet):
# ...
# ...
# packets/clientbound/play/__init__.py
from minecraft.networking.packets import Packet
# Formerly known as state_playing_clientbound.
def get_packets(context):
packets = {
DisconnectPacket,
ChatPacket,
KeepAlivePacket,
MapPacket,
# ...
}
return packets
# Any sufficiently large packets can be defined in their own module and re-exported here:
from .map_packet import MapPacket
# But smaller packets can be defined directly here:
class ChatPacket(Packet):
# ...
# ...
# packets/clientbound/play/map_packet.py
from minecraft.networking.packets import Packet
class MapPacket(Packet):
# ...
# packets/__init__.py
class Packet(object):
# ...
# For backward compatibility, re-export any old names from before the change:
from .clientbound.handshake import get_packets as state_handshake_clientbound
from .clientbound.login import get_packets as state_login_clientbound
# etc...
from .clientbound.login import DisconnectPacket
from .clientbound.play import DisconnectPacket as DisconnectPacketPlayState
from .clientbount.play import ChatPacket as ChatMessagePacket
from .serverbound.play import ChatPacket
from .clientbound.play import KeepAlivePacket as KeepAlivePacketClientbound
from .serverbound.play import KeepAlivePacket as KeepAlivePacketServerbound
from .clientbound.play import MapPacket
# etc...
minecraft/networking/packets.py
Outdated
{'yaw': Float}, | ||
{'pitch': Float}, | ||
{'current_item': Short} if context.protocol_version <= 49 else {} | ||
]) |
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.
I note that this definition of Spawn Player omits the entity metadata section at the end. This isn't a problem, as the packet definition will still work properly for users not interested in the metadata; but perhaps there should be a comment along the lines of TODO: read entity metadata
to make it clear that the definition is incomplete.
minecraft/networking/packets.py
Outdated
|
||
packet_name = 'combat event' | ||
|
||
class EventTypes(object): |
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.
The name of this class should be the singular EntityType
, as instances represent a single event rather than multiple events.
minecraft/networking/packets.py
Outdated
|
||
packet_name = 'explosion' | ||
|
||
class Record(object): |
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 is a reasonable implementation, but I thought I might note that the Record
class can be more succinctly defined by:
from collections import namedtuple
# ...
class ClientExplosion(Packet):
# ...
Record = namedtuple('ClientExplosion.Record', ('x', 'y', 'z'))
with the additional advantage that instances can be treated like ordinary tuples for most purposes, including destructuring assignment - for example:
(x, y, z) = ClientExplosion.Record(1, 2, 3)
would bind x
, y
and z
to 1
, 2
and 3
, respectively.
minecraft/networking/packets.py
Outdated
__slots__ = 'x', 'y', 'z', 'blockId', 'blockMeta' | ||
|
||
def __init__(self, horizontal_position, y_coordinate, blockData): | ||
self.x = (horizontal_position & 0xF0) |
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.
I think this should be:
self.x = (horizontal_position & 0xF0) >> 4
minecraft/networking/packets.py
Outdated
{'action_id': VarInt}]) | ||
|
||
RESPAWN = 0 | ||
REQUEST_STATS = 1 |
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.
Prior to 1.12, there was also an action ID of 2
, representing open inventory. Because pyCraft might connect to a pre-1.12 server, it should be able to handle such an action. For now, it will probably suffice to add
OPEN_INVENTORY = 2
to the class and leave it to the user to determine when it's sensible to expect this value to occur with this meaning.
If another action ID with value 2
and a different meaning is added in the future, it might be necessary to do something more complicated using the context
.
minecraft/networking/types.py
Outdated
if z >= pow(2, 25): | ||
z -= pow(2, 26) | ||
|
||
return {'x': x, 'y': y, 'z': z} |
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.
Further to @ammaraskar's comments on this, which I agree with, I would propose representing the return value as a named tuple rather than a dictionary, as dictionaries might be a bit unwieldy (and memory-inefficient) for this purpose.
You could make Position
a subclass of both Type
and a namedtuple
type, and use a scheme like the following:
from collections import namedtuple
# ...
class Type(object):
__slots__ = () # Ensure that no instance dictionary is allocated.
# ...
# ...
class Position(Type, namedtuple('Position', ('x', 'y', 'z'))):
__slots__ = () # Ensure that no instance dictionary is allocated.
@staticmethod
def read(file_object):
# ...
return Position(x=x, y=y, z=z)
Now the return value of read
can be:
- Treated as an ordinary tuple:
(x, y, z) = Position.read(file) Position.send(x, y, z+1, socket)
- Treated as a named tuple:
pos = Position.read(file) pos = pos._replace(z = pos.z + 1) Position.send(*pos, socket=socket) # The last argument must be a keyword argument in Python 2.
- Treated as a dictionary:
pos = Position.read(file)._asdict() pos['z'] += 1 Position.send(socket=socket, **pos) # The keyword argument must precede the ** in Python 2.
- And automatically printed in a nice format:
>>> Position(1,2,3) Position(x=1, y=2, z=3)
And as long as you make proper use of __slots__
, each Position
will have a compact memory representation suitable for working with a large number of instances.
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.
Creating a Position
class would have the additional benefit that it contains all the fields required for ClientExplosion.Record
, so that could simply subclass Position and pass.
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.
Creating a Position class would have the additional benefit that it contains all the fields required for ClientExplosion.Record, so that could simply subclass Position and pass.
If that is to be done, then it might be better for the named tuple to be a common superclass of both types.Position
and packets.ClientExplosion.Record
, so that the latter class does not include the read
and send
static methods. Note thatClientExplosion.Record
are encoded as an array of 3 bytes, while the fields in a Position
are encoded as 26 + 12 + 26 bits packed into an unsigned long integer, so the two are best not confused.
minecraft/networking/connection.py
Outdated
packet_data.send(decompressed_packet) | ||
packet_data.reset_cursor() | ||
rawPacketData = None | ||
try: |
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.
Firstly, it does indeed appear that the saicopvp server is misconfigured or running a multiple-protocol-support plugin with bugs. It advertises the capability to use protocol 338 (1.12.1), but as you say rejects with an error message. It does accept connections using protocol 316 (1.11.2) and earlier. My suggested workaround would be to configure pyCraft to connect using that specific version, until their issues are fixed, like so:
from minecraft.networking.connection import Connection
conn = Connection(..., allowed_versions={'1.11.2'}) # allowed_versions={316} would also be accepted.
conn.connect()
Secondly, it seems to me that the zlib errors are due to improperly encoded compressed packets being sent by the server. Examining the compressed sections with infgen shows that they consist of a zlib header, followed by an incomplete deflate stream, and not including a zlib footer which would be used to perform error-checking. Despite this, it is possible to obtain a partial output by decoding this whose length matches the decompressed length indicated in each packet, and constituting a valid Minecraft packet, and it seems that this is what Mojang's client does, as it seems to successfully read some packets (Player List Item) that made pyCraft crash at the time of this pull request.
On the other hand, treating the data which failed to compress as an uncompressed packet, as this pull request implements, does not solve the problem, as the result cannot be parsed as a Minecraft packet. It might appear to solve the problem, as the byte giving the packet ID is unrecognised and silently ignored by pyCraft, but this means that the packet is lost, and could cause an exception if this byte happens to be a recognised packet ID.
Considering the behaviour of Mojang's client, I think it would be reasonable for pyCraft to decode the compressed packets in a more lenient fashion and accept truncated zlib streams as long as the decompressed output has the correct length. If the stream is not truncated, error-checking will still occur, so nothing much is lost. It can be implemented like this, using a zlib.decompressobj
instead of zlib.decompress()
:
import zlib
# ...
if self.connection.options.compression_enabled:
decompressed_size = VarInt.read(packet_data)
if decompressed_size > 0:
decompressor = zlib.decompressobj()
decompressed_packet = decompressor.decompress(packet_data.read())
# decompressor.eof may be False, if we have read a truncated stream,
# but we will allow it provided that the following assertion holds.
assert len(decompressed_packet) == decompressed_size, \
'decompressed length %d, but expected %d' % \
(len(decompressed_packet), decompressed_size)
packet_data.reset()
packet_data.send(decompressed_packet)
packet_data.reset_cursor()
That sounds like a solid plan, let's pull this with consistent Packet names and then implement these style of modules. |
Thanks for the feedback guys! |
…t Event (0x2D), and Client Status (0x03) changed in [protocol 336 (snapshot 17w31a)](http://wiki.vg/index.php?title=Pre-release_protocol&oldid=13265)
This reverts commit 357340e.
… the packets module since this packet just got added before the structure changed
I added all the requested changes. The only thing I'm not really happy about is |
A better fix for the flake8 issue would be to add from .clientbound.handshake import get_packets as state_handshake_clientbound
__all__ = ['state_handshake_clientbound', ...] to show that the import is being used as an export |
Nice work! There are just a few packet names still containing some redundant information that I would recommend changing:
The last one may be arguable. It introduces more inconsistency between the old and new names, but IMO it is worth it for the new names to be more consistent among themselves. (I also considered PongPacket, which would be in line with the names now used on wiki.vg, but I don't think that's a good idea, as the Other than that, this seems good to go to me. |
Actually, on that same point, perhaps ChatMessagePacket should be left as it is, because its |
@@ -4,6 +4,7 @@ | |||
""" | |||
import struct | |||
import uuid | |||
from collections import namedtuple | |||
|
|||
|
|||
class Type(object): |
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, and I almost forgot: the class Type
here needs to also have __slots__ = ()
, otherwise every instance of Position
will have an unnecessary empty __dict__
allocated.
…ance of 'Position' will have an unnecessary empty '__dict__' allocated
Thanks for the input! |
@TheSnoozer When implementing this change to the global palette of block state IDs used by the Block Change and Multi Block Change packets, which were added in this PR, I felt the need to make some backward-incompatible changes to these packets, The issue is that the block data can no longer be split into a block ID and a state ID just using bitwise operations, so I felt it was best to leave it as an opaque block state ID for library users to deal with as they see fit. I would be interested to know if this is a helpful change for any use cases of the Block Change and Multi Block Change packets you might have. I'd be pleased to hear any suggestions as to how the implementation might be improved, since I'm not sure my solution is the best one. Edit: incorporate fix from |
Hi @joodicator, For me the main use-case is to be able to determine what type of block we have. If you are interested in a specific use-case feel free to drop me a mail (git log should tell you ;-)). |
@TheSnoozer Each block may be represented by multiple block state IDs. For example, In your use case, if you want to check if a block is an My question is: bearing in mind how you'd have to modify your program to make it work with these changes, is there anything that could be changed about pyCraft's API that would make it less painful a change to deal with? |
@joodicator thanks for the clarification...this makes much more sense now. In terms what feels the 'best' would be potentially a lookup (prop an enum) inside pyCraft's API that tells me what type of block we have. However I fear maintaining that for multiple different versions would be a pain in the ***. Just for reference I originally used the MCProtocolLib (java implementation). I changed cause it was just killing my RAM. Looking at the code (https://github.com/Steveice10/MCProtocolLib/blob/master/src/main/java/com/github/steveice10/mc/protocol/packet/ingame/server/world/ServerMultiBlockChangePacket.java and https://github.com/Steveice10/MCProtocolLib/blob/master/src/main/java/com/github/steveice10/mc/protocol/data/game/world/block/BlockState.java) the implementation just gives you a blockid. That somewhat reflects the code I had written there where I build a client side wrapper to translate the hard-coded values to something more readable (enum). Just a side node: The servers I use the client on do not support 12.X so to that extend I'm not even affected by this change (plus I connect with a hard coded version so my hard coded block ids are guaranteed not changing). Regardless of that it would be convenient to have a lookup. The benefit would be that the client code do not really to bother about different versions (since it would be reflected inside the API's lookup) plus any hard coded (somewhat unreadable magic) can be replaced with something more readable. As you said we currently don't know how the protocol will change in the feature so this would be the ideal world. TLDR:
|
@TheSnoozer I see. I agree that it would be a good feature for pyCraft to be aware of block data, and to allow library users to work with tokens like |
Ok sounds fair for me. If there is anything I see fitting I'd open up a MR or create an issue. |
First of all: thanks for this awesome project!
I decided to play around with it and added some packets to the python implementation (including respawning which seems to be the first actual client command that is not being dealt on server side).
I also added some workaround that deals with faulty compression that for some reason gets introduced when using a specific plugin that supports the play to join a server with multiple versions. Not 100% what happens but based on the packets the server claims to have compression enabled but in fact it is not.
I have tested with the following:
even though I have tested it with almost every major release I can't guarantee its bug free...