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

Packet API #352

Closed
wants to merge 4 commits into from
Closed

Packet API #352

wants to merge 4 commits into from

Conversation

aramperes
Copy link
Member

@aramperes aramperes commented Aug 23, 2016

This is the new packet API for Glowstone, based on two events: PacketReceiveEvent and PacketSendEvent. This plays a role with the future compatibility with ProtocolLib.

Example plugin with the usage of PacketReceiveEvent with packet deconstruction/construction (when you type 'redstone', it will show a redstone block under you):

public class SamplePlugin extends JavaPlugin implements Listener {

    @Override
    public void onEnable() {
        this.getServer().getPluginManager().registerEvents(this, this);
    }

    @EventHandler
    public void onPacketReceive(PacketReceiveEvent event) {
        try {
            if (event.getPacket() == GlowPacket.Play.In.Chat) {
                PacketDeconstructor deconstructor = event.deconstruct();
                String text = (String) deconstructor.getField("text");
                if (!text.equals("redstone")) {
                    return;
                }
                Location location = event.getPlayer().getLocation();
                PacketConstructor constructor = new PacketConstructor(GlowPacket.Play.Out.BlockChange)
                        .intField(location.getBlockX())
                        .intField(location.getBlockY() - 1)
                        .intField(location.getBlockZ())
                        .intField(Material.REDSTONE_BLOCK.getId())
                        .intField(0);
                event.getSession().send(constructor.build());
            }
        } catch (PacketDeconstructor.PacketDeconstructionException | PacketConstructor.PacketConstructException e) {
            e.printStackTrace();
        }
    }
}

API Reference

PacketConstructor and PacketDeconstructor

To build a packet without using the actual class constructor, you can use the PacketConstructor tool:

PacketConstructor constructor = new PacketConstructor(GlowPacket.Play.Out.Health) // Create a new Health update packet
        .floatField(5.0F)   // field 0 "health"
        .intField(17)       // field 1 "food"
        .floatField(0.5F);  // field 2 "saturation"
Message packet = constructor.build(); // Build the packet

To read data from a generic message type, you can use PacketDeconstructor:

Message message = [...]  // The generic message
PacketDeconstructor deconstructor = new PacketDeconstructor(GlowPacket.Play.In.Chat, message); // Initialize deconstructor for inbound Chat packet
String text = (String) deconstructor.getField("text");  // Get the value of the "text" field
deconstructor.setField("text", text + "!"); // Append an exclamation point to the text

Alternatively, if you are in a PacketReceiveEvent/PacketSendEvent handler, you can use event.deconstruct() to initialize the PacketDeconstructor directly.

Cancelling sending and receiving

You can cancel or ignore incoming and outgoing packets using the events.
Ignoring a received inbound packet:

@EventHandler
public void onPacketReceive(PacketReceiveEvent event) {
    event.setIgnored(true); // Ignore the packet, it will not be handled by the server
}

Cancelling an outgoing packet:

@EventHandler
public void onPacketSend(PacketSendEvent event) {
    event.setCancelled(true); // Cancel the packet, it will not be sent.
}

Queuing packets

When you send a new packet inside an event handler, it will be sent before handling/sending the current packet.

@EventHandler
public void onPacketSend(PacketSendEvent event) {
    event.getSession().send(new TimePacket(-1, 1000)); // Will be sent *before* event.getPacket()!
}

To circumvent this (and sending a packet afterwards), you can queue the packets.

@EventHandler
public void onPacketSend(PacketSendEvent event) {
    event.queue(new TimePacket(-1, 1000)); // Will be sent *after* event.getPacket()!
}

To-Do

  • Document all the things
  • Add mutability for other packets (right now only for InboundChatPacket)
  • Registering handlers for specific packets

Rejoice!

@Paulomart
Copy link
Member

I very much like this idea. But I think it would be better to expose the Message class itself to avoid using reflection. Also I did not see if it's possible to register a listener for only some packet types.

@aramperes
Copy link
Member Author

aramperes commented Aug 23, 2016

@Paulomart You can always use the Packet classes directly, like such:

@EventHandler
public void onPacketSend(PacketSendEvent event) {
    // Check packet type beforehand!!
    OutboundChatPacket packet = (OutboundChatPacket) event.getPacketObject();
}

It's just a matter of remembering the class names, you don't need to with PacketConstructor/Deconstructor.

@Paulomart
Copy link
Member

Ah okay then, what about registering a listener for some types only?

@aramperes
Copy link
Member Author

aramperes commented Aug 23, 2016

@Paulomart I didn't commit that yet, but this is an example with a custom Packet listener (already functional, but looking for feedback): http://hastebin.com/loreqileyu.java (3rd argument of registerPacketListener is vararg for multiple packets)

@mastercoms mastercoms self-assigned this Aug 26, 2016
@aramperes aramperes removed this from the 2016Q3 milestone Sep 16, 2016
@mastercoms
Copy link
Member

mastercoms commented Sep 19, 2016

Assuming this PR is bug free, would everyone be ok with accepting it into master so we have this feature, and build on it later?

@Kalkulu
Copy link

Kalkulu commented Sep 19, 2016

I personally think that this should be exposed to Glowkit but implemented in Glowstone.

@mastercoms
Copy link
Member

Packets and networking are server specific, though.

@Kalkulu
Copy link

Kalkulu commented Sep 19, 2016

It is, however, what would be the point of Glowkit if you could just use the implementation directly?

@mastercoms
Copy link
Member

Glowkit is for things that are accessible in the exact same way across all Bukkit servers. How a certain server networks with the client is not up to the API to define.

@Kalkulu
Copy link

Kalkulu commented Sep 19, 2016

Then the packet API might as well not exist and Glowkit is practically pointless. No one should access the implementation of the server to use a feature. This is why craftbukkit's package is versioned.

@mastercoms
Copy link
Member

Why is Glowkit pointless if we are not adding one feature to it?

Glowstone and CraftBukkit are very different and this comparison is not valid. There is no reason against using Glowstone's methods. For CraftBukkit, it relied heavily on decompiled Minecraft code which was not designed to be accessed by plugins and were subject to instability and breakage between versions.

@aramperes
Copy link
Member Author

aramperes commented Sep 19, 2016

@Kalkulu
As I have stated multiple times, exposing this to Glowkit is practically impossible, since the API uses flow. Unless we add flow to Glowkit (which would be pointless), the packets should stay declared in Glowstone.

@Kalkulu
Copy link

Kalkulu commented Sep 19, 2016

And yet that's exactly what should be done. Maybe it's just me, however, the internals of the server should not be exposed, however things like this that are APIs intended for public use should definitely be exposed instead of having them import the internals of Glowstone. Remember, you are creating an implementation for an api. Not an api in an implementation.

@mastercoms
Copy link
Member

Why is having public facing methods in Glowstone so wrong to you?

Glowstone specific APIs go in Glowstone. It's as simple as that. For a general Minecraft server API like Bukkit to have a Packet API that is specific to only a certain implementation of Bukkit just doesn't make sense.

@olliestanley
Copy link
Contributor

Agreed with @mastercoms and @Momothereal gotta remember that other people should be able to implement Glowkit - e.g in future Sponge servers may wish to support Glowkit plugins - and having implementation details in an API doesn't conform to this.

@Kalkulu
Copy link

Kalkulu commented Sep 19, 2016

@DziNeIT yet this is exactly what I'm getting at. Extending GlowKit to support the packet api and implementing it in Glowstone.

@mastercoms
Copy link
Member

But the packet API is a Glowstone specific thing.

You still haven't provided a reason for why to move it there in the first place.

@Kalkulu
Copy link

Kalkulu commented Sep 19, 2016

The reason is the same reason as to why the bukkit api was forked in the first place. Why don't you put all of these enhancement patches inside glowstone instead and have users just use Glowstone instead? Because it does not belong there.

@aramperes
Copy link
Member Author

aramperes commented Sep 19, 2016

@Kalkulu I agree with what you say in some parts and not with some others.

It would be great to allow it to work with Glowkit, but due to how Flow works (having 1 handler for each packet type), it's not feasible (at least not as easy as doing it how this PR does with Glowstone).

About Glowkit, I suggest that plugins based on Glowkit should be relatable, but not necessarily compatible to Bukkit-based software (like CB or Spigot). To me, Bukkit plugin developers are used to making a switch from the API to the implementation when working with internals (like Packets). It's not really complicated to assume you need to use the Server JAR as a dependency for working with network internals: that's how Bukkit implementations, like us, have been doing in the past. The exception to this is ProtocolLib, which might come later.

I think that this should act as a bare-bone to packet handling in Glowstone, since right now it is impossible to register new handlers for certain packets. Maybe there could be an API level in the future, but it's not really my original point of making this. The goal of this was actually to make Linkstone possible, but I demonstrated the usage of this API with plugins as an easier mean to communicate its usage.

I hope you get what I mean by this: It's not really supposed to be an alternative to what ProtocolLib is, it's more about opening the gate to further developments of that kind.

Also I'd like to add that using Bukkit's even handler is probably not suitable for this. I have a custom handling system in the works that should be committed in the near future. That way, this API will be independent to the API to avoid confusion.

e: Also, I'd like to say to not merge this yet @mastercoms, since that packet handler registration system I mentioned is not complete yet.

@PaulBGD
Copy link
Contributor

PaulBGD commented Oct 19, 2016

My thoughts on a packet API is that it should be backwards compatible. As in, we create a wrapper around the actual glowstone packets so that they don't have to use reflection or assume a variable exists.

@kamcio96
Copy link
Member

@PaulBGD Glowstone dosen't have version in package, so you can use packets from main project. What do you mean "backwards compatible"? if mojang change packets, Glowstone will change it too (sometimes we can add some default or ignore fields in constructor) 😉

@PaulBGD
Copy link
Contributor

PaulBGD commented Oct 19, 2016

@kamcio96 Well the reason that this API uses reflection is because the underlying API might change. What I'm saying is that we make an API on top of glowstone packets that doesn't change (or changes incrementally.)

@Paulomart
Copy link
Member

I would prefer non breaking, but I think it's not always possible. In the case a packet may contain a short instead of a byte it would be easy to keep support. But if they add a third hand it may get difficult.

@PaulBGD
Copy link
Contributor

PaulBGD commented Oct 20, 2016

@Paulomart What I'm saying is if we wrap a public API around the packet API, we get the ability to make things that don't break. When they do you can either make workarounds (see when Bukkit switched health from int -> float/double) and/or deprecate.

@Paulomart
Copy link
Member

Yeah we are talking about the same thing

@PaulBGD
Copy link
Contributor

PaulBGD commented Oct 20, 2016

Sorry @Paulomart I thought you were the other guy :p

@aramperes
Copy link
Member Author

Closing this as it probably needs more planning, and because it's pretty outdated. I feel we need a more general consensus on how to implement a Packet API.

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

Successfully merging this pull request may close these issues.

None yet

7 participants