Revert changes to ItemStack handling, fix StreamSerializer

ProtocolLib/src/main/java/com/comphenix/protocol/utility/MinecraftReflection.java

@@ -49,6 +49,7 @@ import com.comphenix.protocol.ProtocolLibrary;
 import com.comphenix.protocol.error.ErrorReporter;
 import com.comphenix.protocol.error.Report;
 import com.comphenix.protocol.error.ReportType;
+import com.comphenix.protocol.injector.BukkitUnwrapper;
 import com.comphenix.protocol.injector.packet.PacketRegistry;
 import com.comphenix.protocol.reflect.ClassAnalyser;
 import com.comphenix.protocol.reflect.ClassAnalyser.AsmMethod;
@@ -126,6 +127,15 @@ public class MinecraftReflection {
 	// The NMS version
 	private static String packageVersion;
 
+	// Item stacks
+	private static Method craftNMSMethod;
+	private static Method craftBukkitNMS;
+	private static Method craftBukkitOBC;
+	private static boolean craftItemStackFailed;
+
+	private static Constructor<?> craftNMSConstructor;
+	private static Constructor<?> craftBukkitConstructor;
+
 	// net.minecraft.server
 	private static Class<?> itemStackArrayClass;
 
@@ -1746,50 +1756,119 @@ public class MinecraftReflection {
 		return getArrayClass(getMultiBlockChangeInfoClass());
 	}
 
-	private static MethodAccessor asCraftMirror;
-	private static MethodAccessor asCraftCopy;
-	private static MethodAccessor asNMSCopy;
-
-	/**
-	 * Retrieve a CraftItemStack from a given NMS ItemStack.
-	 * 
-	 * @param nmsItem - the NMS ItemStack to convert.
-	 * @return A CraftItemStack as a NMS ItemStack.
-	 */
-	public static ItemStack getBukkitItemStack(Object nmsItem) {
-		if (asCraftMirror == null) {
-			asCraftMirror = Accessors.getMethodAccessor(getCraftItemStackClass(), "asCraftMirror", getItemStackClass());
-		}
-
-		return (ItemStack) asCraftMirror.invoke(null, nmsItem);
-	}
-
 	/**
 	 * Retrieve a CraftItemStack from a given ItemStack.
-	 * 
-	 * @param stack - the Bukkit ItemStack to convert.
+	 * @param bukkitItemStack - the Bukkit ItemStack to convert.
 	 * @return A CraftItemStack as an ItemStack.
 	 */
-	public static ItemStack getCraftItemStack(ItemStack stack) {
-		if (asCraftCopy == null) {
-			asCraftCopy = Accessors.getMethodAccessor(getCraftItemStackClass(), "asCraftCopy", ItemStack.class);
+	public static ItemStack getBukkitItemStack(ItemStack bukkitItemStack) {
+		// Delegate this task to the method that can execute it
+		if (craftBukkitNMS != null)
+			return getBukkitItemByMethod(bukkitItemStack);
+
+		if (craftBukkitConstructor == null) {
+			try {
+				craftBukkitConstructor = getCraftItemStackClass().getConstructor(ItemStack.class);
+			} catch (Exception e) {
+				// See if this method works
+				if (!craftItemStackFailed)
+					return getBukkitItemByMethod(bukkitItemStack);
+
+				throw new RuntimeException("Cannot find CraftItemStack(org.bukkit.inventory.ItemStack).", e);
+			}
 		}
 
-		return (ItemStack) asCraftCopy.invoke(null, stack);
+		// Try to create the CraftItemStack
+		try {
+			return (ItemStack) craftBukkitConstructor.newInstance(bukkitItemStack);
+		} catch (Exception e) {
+			throw new RuntimeException("Cannot construct CraftItemStack.", e);
+		}
+	}
+
+	private static ItemStack getBukkitItemByMethod(ItemStack bukkitItemStack) {
+		if (craftBukkitNMS == null) {
+			try {
+				craftBukkitNMS = getCraftItemStackClass().getMethod("asNMSCopy", ItemStack.class);
+				craftBukkitOBC = getCraftItemStackClass().getMethod("asCraftMirror", MinecraftReflection.getItemStackClass());
+			} catch (Exception e) {
+				craftItemStackFailed = true;
+				throw new RuntimeException("Cannot find CraftItemStack.asCraftCopy(org.bukkit.inventory.ItemStack).", e);
+			}
+		}
+
+		// Next, construct it
+		try {
+			Object nmsItemStack = craftBukkitNMS.invoke(null, bukkitItemStack);
+			return (ItemStack) craftBukkitOBC.invoke(null, nmsItemStack);
+		} catch (Exception e) {
+			throw new RuntimeException("Cannot construct CraftItemStack.", e);
+		}
 	}
 
 	/**
-	 * Retrieve the NMS ItemStack from a given ItemStack.
-	 * 
-	 * @param stack - the ItemStack to convert.
-	 * @return The NMS ItemStack.
+	 * Retrieve the Bukkit ItemStack from a given net.minecraft.server ItemStack.
+	 * @param minecraftItemStack - the NMS ItemStack to wrap.
+	 * @return The wrapped ItemStack.
 	 */
-	public static Object getMinecraftItemStack(ItemStack stack) {
-		if (asNMSCopy == null) {
-			asNMSCopy = Accessors.getMethodAccessor(getCraftItemStackClass(), "asNMSCopy", ItemStack.class);
+	public static ItemStack getBukkitItemStack(Object minecraftItemStack) {
+		// Delegate this task to the method that can execute it
+		if (craftNMSMethod != null)
+			return getBukkitItemByMethod(minecraftItemStack);
+
+		if (craftNMSConstructor == null) {
+			try {
+				craftNMSConstructor = getCraftItemStackClass().getConstructor(minecraftItemStack.getClass());
+			} catch (Exception e) {
+				// Give it a try
+				if (!craftItemStackFailed)
+					return getBukkitItemByMethod(minecraftItemStack);
+
+				throw new RuntimeException("Cannot find CraftItemStack(net.minecraft.server.ItemStack).", e);
+			}
 		}
 
-		return asNMSCopy.invoke(null, stack);
+		// Try to create the CraftItemStack
+		try {
+			return (ItemStack) craftNMSConstructor.newInstance(minecraftItemStack);
+		} catch (Exception e) {
+			throw new RuntimeException("Cannot construct CraftItemStack.", e);
+		}
+	}
+
+	private static ItemStack getBukkitItemByMethod(Object minecraftItemStack) {
+		if (craftNMSMethod == null) {
+			try {
+				craftNMSMethod = getCraftItemStackClass().getMethod("asCraftMirror", minecraftItemStack.getClass());
+			} catch (Exception e) {
+				craftItemStackFailed = true;
+				throw new RuntimeException("Cannot find CraftItemStack.asCraftMirror(net.minecraft.server.ItemStack).", e);
+			}
+		}
+
+		// Next, construct it
+		try {
+			return (ItemStack) craftNMSMethod.invoke(null, minecraftItemStack);
+		} catch (Exception e) {
+			throw new RuntimeException("Cannot construct CraftItemStack.", e);
+		}
+	}
+
+	/**
+	 * Retrieve the net.minecraft.server ItemStack from a Bukkit ItemStack.
+	 * <p>
+	 * By convention, item stacks that contain air are usually represented as NULL.
+	 * 
+	 * @param stack - the Bukkit ItemStack to convert.
+	 * @return The NMS ItemStack, or NULL if the stack represents air.
+	 */
+	public static Object getMinecraftItemStack(ItemStack stack) {
+		// Make sure this is a CraftItemStack
+		if (!isCraftItemStack(stack))
+			stack = getBukkitItemStack(stack);
+
+		BukkitUnwrapper unwrapper = new BukkitUnwrapper();
+		return unwrapper.unwrapItem(stack);
 	}
 
 	/**

ProtocolLib/src/main/java/com/comphenix/protocol/utility/StreamSerializer.java

@@ -14,12 +14,14 @@ import org.bukkit.inventory.ItemStack;
 import org.yaml.snakeyaml.external.biz.base64Coder.Base64Coder;
 
 import com.comphenix.protocol.compat.netty.Netty;
+import com.comphenix.protocol.compat.netty.WrappedByteBuf;
 import com.comphenix.protocol.reflect.FuzzyReflection;
 import com.comphenix.protocol.reflect.accessors.Accessors;
 import com.comphenix.protocol.reflect.accessors.MethodAccessor;
 import com.comphenix.protocol.reflect.fuzzy.FuzzyMethodContract;
 import com.comphenix.protocol.wrappers.nbt.NbtCompound;
 import com.comphenix.protocol.wrappers.nbt.NbtFactory;
+import com.comphenix.protocol.wrappers.nbt.NbtType;
 import com.google.common.base.Preconditions;
 
 /**
@@ -308,7 +310,10 @@ public class StreamSerializer {
 				);
 			}
 
-			WRITE_NBT_METHOD.invoke(Netty.packetWriter(output).getHandle(), handle);
+			WrappedByteBuf buf = Netty.packetWriter(output);
+			buf.writeByte(NbtType.TAG_COMPOUND.getRawID());
+
+			WRITE_NBT_METHOD.invoke(buf.getHandle(), handle);
 		} else {
 			if (WRITE_NBT_METHOD == null) {
 				WRITE_NBT_METHOD = Accessors.getMethodAccessor(

ProtocolLib/src/test/java/com/comphenix/protocol/utility/StreamSerializerTest.java

@@ -18,11 +18,14 @@ import org.junit.runner.RunWith;
 import org.powermock.core.classloader.annotations.PowerMockIgnore;
 
 import com.comphenix.protocol.BukkitInitialization;
+import com.comphenix.protocol.wrappers.nbt.NbtCompound;
+import com.comphenix.protocol.wrappers.nbt.NbtFactory;
 
 @RunWith(org.powermock.modules.junit4.PowerMockRunner.class)
 @PowerMockIgnore({ "org.apache.log4j.*", "org.apache.logging.*", "org.bukkit.craftbukkit.libs.jline.*" })
 //@PrepareForTest(CraftItemFactory.class)
 public class StreamSerializerTest {
+
 	@BeforeClass
 	public static void initializeBukkit() throws IllegalAccessException {
 		BukkitInitialization.initializeItemMeta();
@@ -61,17 +64,15 @@ public class StreamSerializerTest {
 		assertEquals(initial, deserialized);
 	}
 
-	// TODO This is an actual issue, probably need to fix the NBT API
 	// For future reference, items are saved in the ChunkRegionLoader and TileEntityChest
 
-	/* @Test
+	@Test
 	public void testCompound() throws IOException {
 		StreamSerializer serializer = new StreamSerializer();
 		NbtCompound initial = NbtFactory.ofCompound("tag");
 		initial.put("name", "Ole");
 		initial.put("age", 20);
-		
-		// Buffer
+
 		ByteArrayOutputStream buffer = new ByteArrayOutputStream();
 		serializer.serializeCompound(new DataOutputStream(buffer), initial);
 		
@@ -80,5 +81,20 @@ public class StreamSerializerTest {
 		NbtCompound deserialized = serializer.deserializeCompound(input);
 		
 		assertEquals(initial, deserialized);
-	} */
+	}
+
+	@Test
+	public void testItems() throws IOException {
+		StreamSerializer serializer = new StreamSerializer();
+		ItemStack initial = new ItemStack(Material.STRING);
+
+		ByteArrayOutputStream buffer = new ByteArrayOutputStream();
+		serializer.serializeItemStack(new DataOutputStream(buffer), initial);
+		
+		DataInputStream input = new DataInputStream(
+				new ByteArrayInputStream(buffer.toByteArray()));
+		ItemStack deserialized = serializer.deserializeItemStack(input);
+		
+		assertEquals(initial, deserialized);
+	}
 }

ProtocolLib/src/test/java/com/comphenix/protocol/wrappers/nbt/NbtFactoryTest.java

@@ -26,12 +26,16 @@ import java.io.DataInputStream;
 import java.io.DataOutput;
 import java.io.DataOutputStream;
 
+import net.minecraft.server.v1_8_R3.ItemStack;
+import net.minecraft.server.v1_8_R3.Items;
+
 import org.junit.BeforeClass;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.powermock.core.classloader.annotations.PowerMockIgnore;
 
 import com.comphenix.protocol.BukkitInitialization;
+import com.comphenix.protocol.utility.MinecraftReflection;
 import com.comphenix.protocol.wrappers.nbt.io.NbtBinarySerializer;
 
 @RunWith(org.powermock.modules.junit4.PowerMockRunner.class)
@@ -65,9 +69,7 @@ public class NbtFactoryTest {
 		assertEquals(compound.getList("nicknames"), cloned.getList("nicknames"));
 	}
 
-	// TODO See StreamSerializerTest
-	
-	/* @Test
+	@Test
 	public void testItemTag() {
 		ItemStack test = new ItemStack(Items.GOLDEN_AXE);
 		org.bukkit.inventory.ItemStack craftTest = MinecraftReflection.getBukkitItemStack(test);
@@ -79,5 +81,5 @@ public class NbtFactoryTest {
 		NbtFactory.setItemTag(craftTest, compound);
 		
 		assertEquals(compound, NbtFactory.fromItemTag(craftTest));
-	} */
+	}
 }