(#90) Pick block events re-done - close #86, close #89

src/main/java/net/fabricmc/fabric/api/event/client/player/ClientPickBlockApplyCallback.java

@@ -0,0 +1,47 @@
+/*
+ * Copyright (c) 2016, 2017, 2018 FabricMC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package net.fabricmc.fabric.api.event.client.player;
+
+import net.fabricmc.fabric.api.event.Event;
+import net.fabricmc.fabric.api.event.EventFactory;
+import net.minecraft.entity.player.PlayerEntity;
+import net.minecraft.item.ItemStack;
+import net.minecraft.util.hit.HitResult;
+
+/**
+ * This event is emitted during the block-picking process. It can be used to
+ * modify the returned ItemStack, as well as nullify it - returning an empty
+ * ItemStack will cause the event to leave, and no block to be picked.
+ */
+public interface ClientPickBlockApplyCallback {
+	public static final Event<ClientPickBlockApplyCallback> EVENT = EventFactory.createArrayBacked(ClientPickBlockApplyCallback.class,
+		(listeners) -> (player, result, _stack) -> {
+			ItemStack stack = _stack;
+
+			for (ClientPickBlockApplyCallback event : listeners) {
+				stack = event.pick(player, result, stack);
+				if (stack == ItemStack.EMPTY || stack.isEmpty()) {
+					return stack;
+				}
+			}
+
+			return stack;
+		}
+	);
+
+	ItemStack pick(PlayerEntity player, HitResult result, ItemStack stack);
+}

src/main/java/net/fabricmc/fabric/api/event/client/player/ClientPickBlockCallback.java

@@ -23,14 +23,15 @@ import net.minecraft.item.ItemStack;
 import net.minecraft.util.hit.HitResult;
 
 /**
- * This interaction event is called on the CLIENT SIDE ONLY when the player
- * attempts to pick up an item.
+ * This event handler has been deprecated due to not hooking nicely
+ * into the game. Please use the alternatives.
  *
- * Use {@link Container} to change the picked stack. Return true if you
- * wish for execution to continue, return false to cancel the item picking
- * operation (for example, if you want to route to the server side, etc.)
+ * @deprecated 0.3.0
  */
+@SuppressWarnings("DeprecatedIsStillUsed")
+@Deprecated
 public interface ClientPickBlockCallback {
+	@Deprecated
 	public static final class Container {
 		private ItemStack stack;
 
@@ -47,6 +48,7 @@ public interface ClientPickBlockCallback {
 		}
 	}
 
+	@Deprecated
 	public static final Event<ClientPickBlockCallback> EVENT = EventFactory.createArrayBacked(ClientPickBlockCallback.class,
 		(listeners) -> (player, result, container) -> {
 			for (ClientPickBlockCallback event : listeners) {

src/main/java/net/fabricmc/fabric/api/event/client/player/ClientPickBlockGatherCallback.java

@@ -0,0 +1,45 @@
+/*
+ * Copyright (c) 2016, 2017, 2018 FabricMC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package net.fabricmc.fabric.api.event.client.player;
+
+import net.fabricmc.fabric.api.event.Event;
+import net.fabricmc.fabric.api.event.EventFactory;
+import net.minecraft.entity.player.PlayerEntity;
+import net.minecraft.item.ItemStack;
+import net.minecraft.util.hit.HitResult;
+
+/**
+ * This event is emitted at the beginning of the block picking process in
+ * order to find any applicable ItemStack. The first non-empty ItemStack
+ * will be returned, overriding vanilla behaviour.
+ */
+public interface ClientPickBlockGatherCallback {
+	public static final Event<ClientPickBlockGatherCallback> EVENT = EventFactory.createArrayBacked(ClientPickBlockGatherCallback.class,
+		(listeners) -> (player, result) -> {
+			for (ClientPickBlockGatherCallback event : listeners) {
+				ItemStack stack = event.pick(player, result);
+				if (stack != ItemStack.EMPTY && !stack.isEmpty()) {
+					return stack;
+				}
+			}
+
+			return ItemStack.EMPTY;
+		}
+	);
+
+	ItemStack pick(PlayerEntity player, HitResult result);
+}

src/main/java/net/fabricmc/fabric/impl/FabricAPIClientInitializer.java

@@ -19,13 +19,14 @@ package net.fabricmc.fabric.impl;
 import net.fabricmc.api.ClientModInitializer;
 import net.fabricmc.fabric.api.block.BlockPickInteractionAware;
 import net.fabricmc.fabric.api.entity.EntityPickInteractionAware;
-import net.fabricmc.fabric.api.event.client.player.ClientPickBlockCallback;
+import net.fabricmc.fabric.api.event.client.player.ClientPickBlockGatherCallback;
 import net.fabricmc.fabric.api.network.ClientSidePacketRegistry;
 import net.fabricmc.fabric.impl.client.gui.ScreenProviderRegistryImpl;
 import net.fabricmc.fabric.impl.registry.RegistrySyncManager;
 import net.minecraft.block.BlockState;
 import net.minecraft.client.MinecraftClient;
 import net.minecraft.entity.Entity;
+import net.minecraft.item.ItemStack;
 import net.minecraft.util.hit.BlockHitResult;
 import net.minecraft.util.hit.EntityHitResult;
 import net.minecraft.util.math.BlockPos;
@@ -39,24 +40,24 @@ public class FabricAPIClientInitializer implements ClientModInitializer {
 			RegistrySyncManager.receivePacket(ctx, buf, !MinecraftClient.getInstance().isInSingleplayer());
 		});
 
-		ClientPickBlockCallback.EVENT.register(((player, result, container) -> {
+		ClientPickBlockGatherCallback.EVENT.register(((player, result) -> {
 			if (result instanceof BlockHitResult) {
 				BlockView view = player.getEntityWorld();
 				BlockPos pos = ((BlockHitResult) result).getBlockPos();
 				BlockState state = view.getBlockState(pos);
 
 				if (state.getBlock() instanceof BlockPickInteractionAware) {
-					container.setStack(((BlockPickInteractionAware) state.getBlock()).getPickedStack(state, view, pos, player, result));
+					return (((BlockPickInteractionAware) state.getBlock()).getPickedStack(state, view, pos, player, result));
 				}
 			} else if (result instanceof EntityHitResult) {
 				Entity entity = ((EntityHitResult) result).getEntity();
 
 				if (entity instanceof EntityPickInteractionAware) {
-					container.setStack(((EntityPickInteractionAware) entity).getPickedStack(player, result));
+					return ((EntityPickInteractionAware) entity).getPickedStack(player, result);
 				}
 			}
 
-			return true;
+			return ItemStack.EMPTY;
 		}));
 
 		((ScreenProviderRegistryImpl) ScreenProviderRegistryImpl.INSTANCE).init();

src/main/java/net/fabricmc/fabric/mixin/events/playerinteraction/MixinMinecraftClient.java

@@ -16,12 +16,12 @@
 
 package net.fabricmc.fabric.mixin.events.playerinteraction;
 
+import net.fabricmc.fabric.api.event.client.player.ClientPickBlockApplyCallback;
 import net.fabricmc.fabric.api.event.client.player.ClientPickBlockCallback;
+import net.fabricmc.fabric.api.event.client.player.ClientPickBlockGatherCallback;
 import net.minecraft.block.entity.BlockEntity;
 import net.minecraft.client.MinecraftClient;
 import net.minecraft.client.gui.Screen;
-import net.minecraft.client.network.ClientPlayerEntity;
-import net.minecraft.client.world.ClientWorld;
 import net.minecraft.entity.player.PlayerInventory;
 import net.minecraft.item.ItemStack;
 import net.minecraft.util.Hand;
@@ -37,49 +37,57 @@ import org.spongepowered.asm.mixin.injection.callback.CallbackInfo;
 
 @Mixin(MinecraftClient.class)
 public abstract class MixinMinecraftClient {
-	private boolean fabric_itemPickSucceeded;
 	private boolean fabric_itemPickCancelled;
 
-	@Redirect(at = @At(value = "INVOKE", target = "Lnet/minecraft/client/MinecraftClient;doItemPick()V"), method = "handleInputEvents")
-	private void sillyRedirection() {
-		fabric_doItemPickWrapper();
+	@SuppressWarnings("deprecation")
+	private ItemStack fabric_emulateOldPick() {
+		MinecraftClient client = (MinecraftClient) (Object) this;
+		ClientPickBlockCallback.Container ctr = new ClientPickBlockCallback.Container(ItemStack.EMPTY);
+		ClientPickBlockCallback.EVENT.invoker().pick(client.player, client.hitResult, ctr);
+		return ctr.getStack();
 	}
 
-	private void fabric_doItemPickWrapper() {
-		// I HATE EVERYTHING THAT STANDS FOR THIS CODE
-		fabric_itemPickSucceeded = false;
-		doItemPick();
-		if (!fabric_itemPickSucceeded) {
-			// vanilla method bailed early, so we have to do this absurd kludge
-			ClientPickBlockCallback.Container ctr = new ClientPickBlockCallback.Container(ItemStack.EMPTY);
-			//noinspection ConstantConditions
-			MinecraftClient client = (MinecraftClient) (Object) this;
+	@Inject(at = @At("HEAD"), method = "doItemPick", cancellable = true)
+	private void fabric_doItemPickWrapper(CallbackInfo info) {
+		MinecraftClient client = (MinecraftClient) (Object) this;
 
-			if (ClientPickBlockCallback.EVENT.invoker().pick(client.player, client.hitResult, ctr)) {
-				// we cannot just jump into the middle of doItemPick, so we have to
-				// mimic vanilla logic here
+		// Do a "best effort" emulation of the old events.
+		ItemStack stack = ClientPickBlockGatherCallback.EVENT.invoker().pick(client.player, client.hitResult);
+		// TODO: Remove in 0.3.0
+		if (stack.isEmpty()) {
+			stack = fabric_emulateOldPick();
+		}
 
-				ItemStack stack = ctr.getStack();
-				PlayerInventory playerInventory = client.player.inventory;
+		if (stack.isEmpty()) {
+			// fall through
+		} else {
+			info.cancel();
 
-				if (client.player.abilities.creativeMode && Screen.isControlPressed() && client.hitResult.getType() == HitResult.Type.BLOCK) {
-					BlockEntity be = client.world.getBlockEntity(((BlockHitResult) client.hitResult).getBlockPos());
-					if (be != null) {
-						stack = addBlockEntityNbt(stack, be);
-					}
+			// I don't like that we clone vanilla logic here, but it's our best bet for now.
+			PlayerInventory playerInventory = client.player.inventory;
+
+			if (client.player.abilities.creativeMode && Screen.isControlPressed() && client.hitResult.getType() == HitResult.Type.BLOCK) {
+				BlockEntity be = client.world.getBlockEntity(((BlockHitResult) client.hitResult).getBlockPos());
+				if (be != null) {
+					stack = addBlockEntityNbt(stack, be);
 				}
+			}
 
-				if (client.player.abilities.creativeMode) {
-					playerInventory.addPickBlock(stack);
-					client.interactionManager.method_2909(client.player.getStackInHand(Hand.MAIN), 36 + playerInventory.selectedSlot);
-				} else {
-					int slot = playerInventory.getSlotWithStack(stack);
-					if (slot >= 0) {
-						if (PlayerInventory.isValidHotbarIndex(slot)) {
-							playerInventory.selectedSlot = slot;
-						} else {
-							client.interactionManager.pickFromInventory(slot);
-						}
+			stack = ClientPickBlockApplyCallback.EVENT.invoker().pick(client.player, client.hitResult, stack);
+			if (stack.isEmpty()) {
+				return;
+			}
+
+			if (client.player.abilities.creativeMode) {
+				playerInventory.addPickBlock(stack);
+				client.interactionManager.method_2909(client.player.getStackInHand(Hand.MAIN), 36 + playerInventory.selectedSlot);
+			} else {
+				int slot = playerInventory.getSlotWithStack(stack);
+				if (slot >= 0) {
+					if (PlayerInventory.isValidHotbarIndex(slot)) {
+						playerInventory.selectedSlot = slot;
+					} else {
+						client.interactionManager.pickFromInventory(slot);
 					}
 				}
 			}
@@ -91,28 +99,17 @@ public abstract class MixinMinecraftClient {
 	@Shadow
 	public abstract ItemStack addBlockEntityNbt(ItemStack itemStack_1, BlockEntity blockEntity_1);
 
-	@ModifyVariable(at = @At(value = "INVOKE", target = "Lnet/minecraft/item/ItemStack;isEmpty()Z", ordinal = 2), method = "doItemPick", ordinal = 0)
+	@ModifyVariable(at = @At(value = "INVOKE", target = "Lnet/minecraft/entity/player/PlayerInventory;getSlotWithStack(Lnet/minecraft/item/ItemStack;)I"), method = "doItemPick", ordinal = 0)
 	public ItemStack modifyItemPick(ItemStack stack) {
-		fabric_itemPickSucceeded = true;
-
-		ClientPickBlockCallback.Container ctr = new ClientPickBlockCallback.Container(stack);
-		//noinspection ConstantConditions
 		MinecraftClient client = (MinecraftClient) (Object) this;
-
-		boolean toContinue = ClientPickBlockCallback.EVENT.invoker().pick(client.player, client.hitResult, ctr);
-		if (!toContinue) {
-			fabric_itemPickCancelled = true;
-			return ItemStack.EMPTY;
-		} else {
-			fabric_itemPickCancelled = false;
-			return ctr.getStack();
-		}
+		ItemStack result = ClientPickBlockApplyCallback.EVENT.invoker().pick(client.player, client.hitResult, stack);
+		fabric_itemPickCancelled = result.isEmpty();
+		return result;
 	}
 
-	@Inject(at = @At(value = "INVOKE", target = "Lnet/minecraft/item/ItemStack;isEmpty()Z", ordinal = 2), method = "doItemPick", cancellable = true)
+	@Inject(at = @At(value = "INVOKE", target = "Lnet/minecraft/entity/player/PlayerInventory;getSlotWithStack(Lnet/minecraft/item/ItemStack;)I"), method = "doItemPick", cancellable = true)
 	public void cancelItemPick(CallbackInfo info) {
 		if (fabric_itemPickCancelled) {
-			fabric_itemPickCancelled = false;
 			info.cancel();
 		}
 	}

src/main/resources/fabric.mod.json

@@ -7,7 +7,7 @@
   "license": "Apache-2.0",
   "initializers": [
     "net.fabricmc.fabric.impl.FabricAPIInitializer",
-    "net.fabricmc.fabric.impl.FabricAPIClientInitializer"
+    "net.fabricmc.fabric.events.PickBlockEventModClient"
   ],
   "mixins": {
     "client": "net.fabricmc.fabric.mixins.client.json",

src/test/java/net/fabricmc/fabric/events/PickBlockEventModClient.java

@@ -0,0 +1,56 @@
+/*
+ * Copyright (c) 2016, 2017, 2018 FabricMC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package net.fabricmc.fabric.events;
+
+import net.fabricmc.api.ClientModInitializer;
+import net.fabricmc.fabric.api.event.client.player.ClientPickBlockApplyCallback;
+import net.fabricmc.fabric.api.event.client.player.ClientPickBlockGatherCallback;
+import net.minecraft.block.BlockState;
+import net.minecraft.block.Blocks;
+import net.minecraft.item.Item;
+import net.minecraft.item.ItemStack;
+import net.minecraft.util.hit.BlockHitResult;
+import net.minecraft.util.math.BlockPos;
+import net.minecraft.world.BlockView;
+import net.minecraft.world.World;
+
+public class PickBlockEventModClient implements ClientModInitializer {
+	@Override
+	public void onInitializeClient() {
+		ClientPickBlockGatherCallback.EVENT.register((player, result) -> {
+			if (result instanceof BlockHitResult) {
+				BlockView view = player.getEntityWorld();
+				BlockPos pos = ((BlockHitResult) result).getBlockPos();
+				BlockState state = view.getBlockState(pos);
+
+				if (state.getBlock() == Blocks.STONE) {
+					return new ItemStack(Blocks.OAK_WOOD);
+				}
+			}
+
+			return ItemStack.EMPTY;
+		});
+
+		ClientPickBlockApplyCallback.EVENT.register((player, result, stack) -> {
+			if (stack.getItem() == Item.getItemFromBlock(Blocks.OAK_WOOD)) {
+				return new ItemStack(Blocks.ACACIA_WOOD);
+			} else {
+				return stack;
+			}
+		});
+	}
+}