Various improvements #71
Conversation
added `EntitySoundEffectPacket`, `ParticlePacket`, and `SoundEffectPacket`
Since the returned Task of AsyncEvent.Dispatch now always completes after all subscribers finished I inspected the places where await was used on such tasks. Removed the await in the most cases since that how it was before.
…provements - added IVersionAwareSerializable to extend ISerializable with version specific logic like in the packets - moved some core element serialization methods in their classes fixes psu-de#68
|
Hi, However I get some exceptions when running the integration tests. |
| /// </summary> | ||
| /// <param name="MessageId">The message ID + 1, used for validating message signature.</param> | ||
| /// <param name="Signature">The previous message's signature. Always 256 bytes and not length-prefixed.</param> | ||
| public sealed record DeleteMessagePacket(int MessageId, byte[]? Signature) : IPacket |
There was a problem hiding this comment.
DeleteMessagePacket is a little bit different in 1.19.2
| /// </summary> | ||
| /// <param name="Position">The position of the scoreboard.</param> | ||
| /// <param name="ScoreName">The unique name for the scoreboard to be displayed.</param> | ||
| public sealed record DisplayObjectivePacket(int Position, string ScoreName) : IPacket |
There was a problem hiding this comment.
In 1.20 and before, the Position field is sent as a sbyte
| /// End Combat packet | ||
| /// </summary> | ||
| /// <param name="Duration">Length of the combat in ticks</param> | ||
| public sealed record EndCombatPacket(int Duration) : IPacket |
There was a problem hiding this comment.
In 1.19.4 and before, this packet had an additional entityId field
|
|
||
| namespace MineSharp.Protocol.Packets.Clientbound.Play; | ||
| #pragma warning disable CS1591 | ||
| public sealed record EntitySoundEffectPacket( |
There was a problem hiding this comment.
EntitySoundEffect packet changed in 1.19 and again in 1.19.3
There was a problem hiding this comment.
With all that differences between versions how should we deal with that?
-
We could make the packet class able to handle all the versions. But that often means that the paket gets fields that are not used in other versions. So we need to make them nullable. And also need to document when (for which version) these nullable fields are used.
-
Or alternatively we could create a implementation of that packet class for every version that changed something in that packet. This makes using the packet classes more straitforward since you really get all the fields it hat. But that would mean that for different versions difference PacketHandlers are required (and need to be registered with
On<PACKET>).
I like option 2 more. What do you think?
There was a problem hiding this comment.
Yeah, i've been struggeling with this problem ever since I started this project...
I'd also go with Option 2. Not too long ago I did something similar with SystemChatMessagePacket. I used an abstract base class so the different versions don't have to be registered themselves.
There was a problem hiding this comment.
btw. don't worry if you don't want to bother implementing all different packet versions. I'll do it at some point when I'm bored...
There was a problem hiding this comment.
I think that we need an even more abstract/universal approach than that currently used in SystemChatMessagePacket. A system that is consistent between all packets.
I mean I would also do this with using inheritance of the base packet class. But all the possible version specific sub classes need to implement a common interface and need to be registered in a lookup table (per packet).
We should probably create an issue regarding that topic.
There was a problem hiding this comment.
Hi,
sorry it took a while....
I've updated this Issue, and tried to write down my thoughts.
| /// <param name="Experience">Total experience for this villager (always 0 for the wandering trader).</param> | ||
| /// <param name="IsRegularVillager">True if this is a regular villager; false for the wandering trader.</param> | ||
| /// <param name="CanRestock">True for regular villagers and false for the wandering trader.</param> | ||
| public sealed record MerchantOffersPacket(int WindowId, int Size, Trade[] Trades, int VillagerLevel, int Experience, bool IsRegularVillager, bool CanRestock) : IPacket |
There was a problem hiding this comment.
in 1.18, Size is a byte, not varint
| /// </summary> | ||
| /// <param name="Location">The position of the sign</param> | ||
| /// <param name="IsFrontText">Whether the opened editor is for the front or on the back of the sign</param> | ||
| public sealed record OpenSignEditorPacket(Position Location, bool IsFrontText) : IPacket |
There was a problem hiding this comment.
IsFrontText doesn't exist in 1.19.4
| /// Data depends on the particle id. | ||
| /// Will be an empty buffer for most particles. | ||
| /// </param> | ||
| public sealed record ParticlePacket( |
There was a problem hiding this comment.
ParticleId was sent as a VarInt before 1.19
| /// </summary> | ||
| /// <param name="EntityId">The entity ID</param> | ||
| /// <param name="EffectId">The effect ID</param> | ||
| public sealed record RemoveEntityEffectPacket(int EntityId, int EffectId) : IPacket |
There was a problem hiding this comment.
EffectId was sent as a byte before 1.19
| /// <param name="HasIcon">Indicates if the server has an icon.</param> | ||
| /// <param name="Icon">Optional icon bytes in the PNG format.</param> | ||
| /// <param name="EnforcesSecureChat">Indicates if the server enforces secure chat.</param> | ||
| public sealed record ServerDataPacket(Chat Motd, bool HasIcon, byte[]? Icon, bool EnforcesSecureChat) : IPacket |
There was a problem hiding this comment.
Packet was different in 1.18, and changed in 1.19, 1.19.2, 1.19.3 and 1.19.4
| /// <param name="ExperienceBar">The experience bar value between 0 and 1</param> | ||
| /// <param name="Level">The experience level</param> | ||
| /// <param name="TotalExperience">The total experience points</param> | ||
| public sealed record SetExperiencePacket(float ExperienceBar, int Level, int TotalExperience) : IPacket |
There was a problem hiding this comment.
for some reason, the order in which ExperienceBar and Level are sent changed in 1.19.2 and then again in 1.20.2
|
|
||
| namespace MineSharp.Protocol.Packets.Clientbound.Play; | ||
| #pragma warning disable CS1591 | ||
| public sealed record SoundEffectPacket( |
There was a problem hiding this comment.
Packet was different in 1.18 and changed in .19.2 and 1.19.3
| /// <param name="EmptyBlockLightMask">BitSet for empty block light sections</param> | ||
| /// <param name="SkyLightArrays">Array of sky light data</param> | ||
| /// <param name="BlockLightArrays">Array of block light data</param> | ||
| public sealed record UpdateLightPacket( |
There was a problem hiding this comment.
Until 1.19.4, there was a TrustEdges field
| /// <param name="Value">The score to be displayed next to the entry</param> | ||
| /// <param name="DisplayName">The custom display name</param> | ||
| /// <param name="NumberFormat">The number format for the score</param> | ||
| public sealed record UpdateScorePacket( |
Components/MineSharp.Protocol/Packets/Handlers/HandshakePacketHandler.cs
Outdated
Show resolved
Hide resolved
|
I am currently having a problem with the players health. Upon joining the server sends the |
Hm no... Did your last commit fix it? |
Yes in that it now works without triggering my bot to auto disconnect (because I wait for the |
|
I added a lot of missing packets but there are still some missing. I will slowly add them as I find time to do so |
Hi, I am using your repo to build my Minecraft bot and while doing so I continued developing your project.
I am doing this in my forked repo.
This pull request is a friendly note that I am doing this and you can take my changes from that fork anytime.
So you can merge this pull request or just cherry pick changes you like.