[1.12.2] Packet problems when updating capability data

[Alekseyev]

Hi all,

 

after quite some time away I finally got back to fixing a long-standing issue within my mod in progress. (See here)

 

I was advised to send the capability data to the server via a packet, and therefore set out to that. I came across this tutorial: https://wiki.mcjty.eu/modding/index.php?title=Networking-1.12 and used that to test out how packets work. The example function that was provided works flawlessly. However, when I tried to use another packet to update the needed data, I received a crash.

 

Here's the crash log.

 

And here are my relevant classes:

 

Event Subscriber

	@SideOnly(Side.CLIENT)
	@SubscribeEvent(priority=EventPriority.NORMAL, receiveCanceled=true)
	public static void onKeyInput(InputEvent.KeyInputEvent event)
	{
		System.out.println("Key Input Event");
		KeyBinding[] keyBindings = ClientProxy.keyBindings;

		// crashes
		if (keyBindings[KEY_TEST].isPressed())
		{
			System.out.println("Key binding =" + keyBindings[KEY_TEST].getKeyDescription());
			PacketHandler.INSTANCE.sendToServer(new PacketUpdateElement(BioElements.WATER));
		}

      	//Works fine
		if (keyBindings[KEY_GET_BLOCK].isPressed())
		{
			System.out.println("Key binding =" + keyBindings[KEY_GET_BLOCK].getKeyDescription());
			PacketHandler.INSTANCE.sendToServer(new PacketSendKey()); 
		}
	}

 

Packet

public class PacketUpdateElement implements IMessage
{
	private BioElements element;

	@Override
	public void fromBytes(ByteBuf buf)
	{
		try
		{
			byte[] bytes = new byte[buf.readableBytes()];
			buf.readBytes(bytes);
			element = BioElements.valueOf(new String(bytes));
		}
		catch (IllegalArgumentException e) // If no matching String is found in the enum, an IllegalArgumentException will be thrown
		{
			System.out.println("An error occurred: " + e.getMessage());
			element = BioElements.NONE;
			System.out.println("Element was set to NONE.");
		}
	}

	@Override
	public void toBytes(ByteBuf buf)
	{
		buf.writeBytes(element.name().getBytes());
	}

	public PacketUpdateElement(BioElements element)
	{
		this.element = element;
	}

	public static class Handler implements IMessageHandler<PacketUpdateElement, IMessage>
	{
		@Override
		public IMessage onMessage(PacketUpdateElement message, MessageContext context)
		{
			FMLCommonHandler.instance().getWorldThread(context.netHandler).addScheduledTask(() -> handle(message, context));
			return null;
		}

		private void handle(PacketUpdateElement message, MessageContext context)
		{
			EntityPlayerMP playerEntity = context.getServerHandler().player;

			if (playerEntity.hasCapability(CAPABILITY_BIO_PLAYER_DATA, null))
			{
				playerEntity.getCapability(CAPABILITY_BIO_PLAYER_DATA, null).setElement(message.element);
			}

			playerEntity.sendStatusMessage(new TextComponentString(TextFormatting.GREEN + "Selected element from packet: " + message.element.name()), false);
		}
	}
}

 

Packet handler

public class PacketHandler
{
	private static int packetId = 0;

	public static SimpleNetworkWrapper INSTANCE = null;

	public PacketHandler()
	{
	}

	public static int nextID()
	{
		return packetId++;
	}

	public static void registerMessages(String channelName)
	{
		INSTANCE = NetworkRegistry.INSTANCE.newSimpleChannel(channelName);
		registerMessages();
	}

	public static void registerMessages()
	{
		// Register messages which are sent from the client to the server here:
		INSTANCE.registerMessage(PacketSendKey.Handler.class, PacketSendKey.class, nextID(), Side.SERVER);
		INSTANCE.registerMessage(PacketUpdateElement.Handler.class, PacketUpdateElement.class, nextID(), Side.SERVER);
	}
}

PacketHandler.registerMessages("biomod");  is fired in the FMLInitializationEvent event subscriber.

 

I tried figuring out the issue on my own, but had no luck so far.

[V0idWa1k3r]

Your packet class MUST have an explicitly defined parameterless constructor.

 

15 minutes ago, Alekseyev said:

@SideOnly(Side.CLIENT)

Don't abuse SideOnly, modders are not even meant to use it in the first place. If your event handlers are client-side only then only register them on the client.

 

Don't send a packet to the server every time a key is pressed regardless of anything. Perform the checks as to whether the client should even send the packet first, receive the packet on the server, perform your checks for whatever it is you need to do, etc. Don't spam the server with packets, but don't trust the client either.

 

17 minutes ago, Alekseyev said:

byte[] bytes = new byte[buf.readableBytes()]; buf.readBytes(bytes); element = BioElements.valueOf(new String(bytes));

Why are you doing this? Just store the ordinal as a byte or something in the buffer, then read the enum constant through said enum's array.

If you don't like this approach then you can have an index field in your enums and get it by the index. There is no reason to send a string.

Also if you ever do need to send a string use ByteBufUtils.

 

 

[Alekseyev]

2 minutes ago, V0idWa1k3r said:

Your packet class MUST have an explicitly defined parameterless constructor.

How do I supply the info to the packet, then?

 

---------------------------------------------------

1 minute ago, V0idWa1k3r said:

Don't send a packet to the server every time a key is pressed regardless of anything. Perform the checks as to whether the client should even send the packet first, receive the packet on the server, perform your checks for whatever it is you need to do, etc. Don't spam the server with packets, but don't trust the client either.

Thanks for the heads up, I will keep it in mind for the future. This particular case was just for easy testing anyway and will be removed afterwards.

3 minutes ago, V0idWa1k3r said:

Why are you doing this? Just store the ordinal as a byte or something in the buffer, then read the enum constant through said enum's array.

If you don't like this approach then you can have an index field in your enums and get it by the index. There is no reason to send a string. 

Also if you ever do need to send a string use ByteBufUtils.

I thought about simply using an ordinal and a single byte, since the amount of enum values will never exceed 256 anyway. I will look into fixing this as soon as the packet sending itself works.

[V0idWa1k3r]

5 minutes ago, Alekseyev said:

How do I supply the info to the packet, then?

 

I didn't say that it must ONLY have the parameterless constructor, i said that you must INCLUDE it. You can still keep your current constructor and use it just fine.

 

Also there are a ton of ways to pass data to an object without involving the constructor, like a setter for example. But this is basic java.

 

 

[Alekseyev]

Just now, V0idWa1k3r said:

Also there are a ton of ways to pass data to an object without involving the constructor, like a setter for example. But this is basic java

Sorry, sometimes I'm dumb. I was assuming that if there has to be a parameterless constructor, that one would be used in the sending process somehow and you would be unable to use your pre-created object.

 

I would dare to say that I know basic java pretty well, it's using someone else's hooks and libraries and whatnot (in this case Forge) what gets me all the time. :s

[Draco18s]

3 hours ago, Alekseyev said:

Sorry, sometimes I'm dumb. I was assuming that if there has to be a parameterless constructor, that one would be used in the sending process somehow and you would be unable to use your pre-created object.

Server creates the packet by calling new YourPacket(some, params), right?

 

What do you think the client does, it doesn't know what some, params are, but it still needs to create the packet instance so that it can take the buffer it does have and pipe it through fromBytes(buf). And the only way that even makes sense is to call a parameterless constructor.

[Cadiboo]

8 hours ago, Draco18s said:

And the only way that even makes sense is to call a parameterless constructor.

A factory would be nicer if vanilla did it that way