Interaction events fixes (#2774)

* Deprecate `BlockAttackInteractionAware`

* Document PickBlock events, fix nullability and edge case

* Fix `UseEntityCallback` usability and documentation. Fixes #1260. Fixes #1870.

fabric-events-interaction-v0/src/client/java/net/fabricmc/fabric/mixin/event/interaction/client/ClientPlayerInteractionManagerMixin.java

@@ -43,16 +43,13 @@ import net.minecraft.util.ActionResult;
 import net.minecraft.util.Hand;
 import net.minecraft.util.TypedActionResult;
 import net.minecraft.util.hit.BlockHitResult;
-import net.minecraft.util.hit.EntityHitResult;
 import net.minecraft.util.math.BlockPos;
 import net.minecraft.util.math.Direction;
-import net.minecraft.util.math.Vec3d;
 import net.minecraft.world.GameMode;
 
 import net.fabricmc.fabric.api.event.player.AttackBlockCallback;
 import net.fabricmc.fabric.api.event.player.AttackEntityCallback;
 import net.fabricmc.fabric.api.event.player.UseBlockCallback;
-import net.fabricmc.fabric.api.event.player.UseEntityCallback;
 import net.fabricmc.fabric.api.event.player.UseItemCallback;
 
 @Mixin(ClientPlayerInteractionManager.class)
@@ -143,20 +140,6 @@ public abstract class ClientPlayerInteractionManagerMixin {
 		}
 	}
 
-	@Inject(at = @At(value = "INVOKE", target = "Lnet/minecraft/client/network/ClientPlayNetworkHandler;sendPacket(Lnet/minecraft/network/Packet;)V", ordinal = 0), method = "interactEntityAtLocation", cancellable = true)
-	public void interactEntityAtLocation(PlayerEntity player, Entity entity, EntityHitResult hitResult, Hand hand, CallbackInfoReturnable<ActionResult> info) {
-		ActionResult result = UseEntityCallback.EVENT.invoker().interact(player, player.getEntityWorld(), hand, entity, hitResult);
-
-		if (result != ActionResult.PASS) {
-			if (result == ActionResult.SUCCESS) {
-				Vec3d hitVec = hitResult.getPos().subtract(entity.getX(), entity.getY(), entity.getZ());
-				this.networkHandler.sendPacket(PlayerInteractEntityC2SPacket.interactAt(entity, player.isSneaking(), hand, hitVec));
-			}
-
-			info.setReturnValue(result);
-		}
-	}
-
 	@Shadow
 	public abstract void sendSequencedPacket(ClientWorld clientWorld, SequencedPacketCreator supplier);
 }

fabric-events-interaction-v0/src/client/java/net/fabricmc/fabric/mixin/event/interaction/client/MinecraftClientMixin.java

@@ -22,19 +22,28 @@ import org.spongepowered.asm.mixin.injection.At;
 import org.spongepowered.asm.mixin.injection.Inject;
 import org.spongepowered.asm.mixin.injection.ModifyVariable;
 import org.spongepowered.asm.mixin.injection.callback.CallbackInfo;
+import org.spongepowered.asm.mixin.injection.callback.LocalCapture;
 
 import net.minecraft.block.entity.BlockEntity;
 import net.minecraft.client.MinecraftClient;
 import net.minecraft.client.gui.screen.Screen;
+import net.minecraft.client.network.ClientPlayNetworkHandler;
+import net.minecraft.client.network.ClientPlayerEntity;
+import net.minecraft.entity.Entity;
 import net.minecraft.entity.player.PlayerInventory;
 import net.minecraft.item.ItemStack;
+import net.minecraft.network.packet.c2s.play.PlayerInteractEntityC2SPacket;
+import net.minecraft.util.ActionResult;
 import net.minecraft.util.Hand;
 import net.minecraft.util.hit.BlockHitResult;
+import net.minecraft.util.hit.EntityHitResult;
 import net.minecraft.util.hit.HitResult;
+import net.minecraft.util.math.Vec3d;
 
 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.fabricmc.fabric.api.event.player.UseEntityCallback;
 
 @Mixin(MinecraftClient.class)
 public abstract class MinecraftClientMixin {
@@ -116,4 +125,36 @@ public abstract class MinecraftClientMixin {
 			info.cancel();
 		}
 	}
+
+	@Shadow
+	private ClientPlayerEntity player;
+
+	@Shadow
+	public abstract ClientPlayNetworkHandler getNetworkHandler();
+
+	@Inject(
+			at = @At(
+					value = "INVOKE",
+					target = "net/minecraft/client/network/ClientPlayerInteractionManager.interactEntityAtLocation(Lnet/minecraft/entity/player/PlayerEntity;Lnet/minecraft/entity/Entity;Lnet/minecraft/util/hit/EntityHitResult;Lnet/minecraft/util/Hand;)Lnet/minecraft/util/ActionResult;"
+			),
+			method = "doItemUse",
+			cancellable = true,
+			locals = LocalCapture.CAPTURE_FAILHARD
+	)
+	private void injectUseEntityCallback(CallbackInfo ci, Hand[] hands, int i1, int i2, Hand hand, ItemStack stack, EntityHitResult hitResult, Entity entity) {
+		ActionResult result = UseEntityCallback.EVENT.invoker().interact(player, player.getEntityWorld(), hand, entity, hitResult);
+
+		if (result != ActionResult.PASS) {
+			if (result.isAccepted()) {
+				Vec3d hitVec = hitResult.getPos().subtract(entity.getX(), entity.getY(), entity.getZ());
+				getNetworkHandler().sendPacket(PlayerInteractEntityC2SPacket.interactAt(entity, player.isSneaking(), hand, hitVec));
+			}
+
+			if (result.shouldSwingHand()) {
+				player.swingHand(hand);
+			}
+
+			ci.cancel();
+		}
+	}
 }

fabric-events-interaction-v0/src/main/java/net/fabricmc/fabric/api/block/BlockAttackInteractionAware.java

@@ -23,9 +23,15 @@ import net.minecraft.util.math.BlockPos;
 import net.minecraft.util.math.Direction;
 import net.minecraft.world.World;
 
+import net.fabricmc.fabric.api.event.player.AttackBlockCallback;
+
 /**
  * Convenience interface for blocks which listen to "break interactions" (left-click).
+ *
+ * @deprecated Use {@link AttackBlockCallback} instead and check for the block.
+ * This gives more control over the different cancellation outcomes.
  */
+@Deprecated
 public interface BlockAttackInteractionAware {
 	/**
 	 * @return True if the block accepted the player and it should no longer be processed.

fabric-events-interaction-v0/src/main/java/net/fabricmc/fabric/api/block/BlockPickInteractionAware.java

@@ -16,15 +16,21 @@
 
 package net.fabricmc.fabric.api.block;
 
-import org.jetbrains.annotations.Nullable;
-
+import net.minecraft.block.Block;
 import net.minecraft.block.BlockState;
 import net.minecraft.entity.player.PlayerEntity;
 import net.minecraft.item.ItemStack;
+import net.minecraft.util.hit.BlockHitResult;
 import net.minecraft.util.hit.HitResult;
 import net.minecraft.util.math.BlockPos;
 import net.minecraft.world.BlockView;
 
+/**
+ * Convenience interface for blocks that want more stack picking context than what
+ * {@link Block#getPickStack(BlockView, BlockPos, BlockState)} provides.
+ *
+ * <p>The hit result is guaranteed to be a {@link BlockHitResult} that did not miss.
+ */
 public interface BlockPickInteractionAware {
-	ItemStack getPickedStack(BlockState state, BlockView view, BlockPos pos, @Nullable PlayerEntity player, @Nullable HitResult result);
+	ItemStack getPickedStack(BlockState state, BlockView view, BlockPos pos, PlayerEntity player, HitResult result);
 }

fabric-events-interaction-v0/src/main/java/net/fabricmc/fabric/api/entity/EntityPickInteractionAware.java

@@ -16,12 +16,18 @@
 
 package net.fabricmc.fabric.api.entity;
 
-import org.jetbrains.annotations.Nullable;
-
+import net.minecraft.entity.Entity;
 import net.minecraft.entity.player.PlayerEntity;
 import net.minecraft.item.ItemStack;
+import net.minecraft.util.hit.EntityHitResult;
 import net.minecraft.util.hit.HitResult;
 
+/**
+ * Convenience interface for entities that want more stack picking context than what
+ * {@link Entity#getPickBlockStack()} provides.
+ *
+ * <p>The hit result is guaranteed to be an {@link EntityHitResult}.
+ */
 public interface EntityPickInteractionAware {
-	ItemStack getPickedStack(@Nullable PlayerEntity player, @Nullable HitResult result);
+	ItemStack getPickedStack(PlayerEntity player, HitResult result);
 }

fabric-events-interaction-v0/src/main/java/net/fabricmc/fabric/api/event/client/player/ClientPickBlockGatherCallback.java

@@ -18,6 +18,8 @@ package net.fabricmc.fabric.api.event.client.player;
 
 import net.minecraft.entity.player.PlayerEntity;
 import net.minecraft.item.ItemStack;
+import net.minecraft.util.hit.BlockHitResult;
+import net.minecraft.util.hit.EntityHitResult;
 import net.minecraft.util.hit.HitResult;
 
 import net.fabricmc.fabric.api.event.Event;
@@ -27,6 +29,11 @@ import net.fabricmc.fabric.api.event.EventFactory;
  * 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 behavior.
+ *
+ * <p>Note that this is called any time the pick key is pressed, even if there is no target block.
+ * The {@link HitResult} could be a {@link BlockHitResult} or an {@link EntityHitResult}.
+ * If the hit missed, it will be a {@link BlockHitResult} with {@linkplain BlockHitResult#getType() type}
+ * {@link BlockHitResult.Type#MISS}, so make sure to check for that.
  */
 public interface ClientPickBlockGatherCallback {
 	Event<ClientPickBlockGatherCallback> EVENT = EventFactory.createArrayBacked(ClientPickBlockGatherCallback.class,

fabric-events-interaction-v0/src/main/java/net/fabricmc/fabric/api/event/player/UseEntityCallback.java

@@ -32,10 +32,23 @@ import net.fabricmc.fabric.api.event.EventFactory;
  * Callback for right-clicking ("using") an entity.
  * Is hooked in before the spectator check, so make sure to check for the player's game mode as well!
  *
- * <p>Upon return:
- * <ul><li>SUCCESS cancels further processing and, on the client, sends a packet to the server.
- * <li>PASS falls back to further processing.
- * <li>FAIL cancels further processing and does not send a packet to the server.</ul>
+ * <p>On the logical client, the return values have the following meaning:
+ * <ul>
+ *     <li>SUCCESS cancels further processing, causes a hand swing, and sends a packet to the server.</li>
+ *     <li>CONSUME cancels further processing, and sends a packet to the server. It does NOT cause a hand swing.</li>
+ *     <li>PASS falls back to further processing.</li>
+ *     <li>FAIL cancels further processing and does not send a packet to the server.</li>
+ * </ul>
+ *
+ * <p>On the logical server, the return values have the following meaning:
+ * <ul>
+ *     <li>PASS falls back to further processing.</li>
+ *     <li>Any other value cancels further processing.</li>
+ * </ul>
+ *
+ * <p>Note that on the server, the {@link EntityHitResult} may be {@code null} if the client successfully interacted using
+ * the {@linkplain PlayerEntity#interact(Entity, Hand) position-less overload}.
+ * On the client, the {@link EntityHitResult} will never be null.
  */
 public interface UseEntityCallback {
 	Event<UseEntityCallback> EVENT = EventFactory.createArrayBacked(UseEntityCallback.class,

fabric-events-interaction-v0/src/main/java/net/fabricmc/fabric/impl/event/interaction/InteractionEventsRouterClient.java

@@ -21,6 +21,7 @@ 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.hit.HitResult;
 import net.minecraft.util.math.BlockPos;
 import net.minecraft.world.BlockView;
 
@@ -33,7 +34,7 @@ public class InteractionEventsRouterClient implements ClientModInitializer {
 	@Override
 	public void onInitializeClient() {
 		ClientPickBlockGatherCallback.EVENT.register(((player, result) -> {
-			if (result instanceof BlockHitResult) {
+			if (result instanceof BlockHitResult && result.getType() != HitResult.Type.MISS) {
 				BlockView view = player.getEntityWorld();
 				BlockPos pos = ((BlockHitResult) result).getBlockPos();
 				BlockState state = view.getBlockState(pos);

fabric-events-interaction-v0/src/main/java/net/fabricmc/fabric/mixin/event/interaction/ServerPlayNetworkHandlerMixin.java

@@ -57,4 +57,16 @@ public abstract class ServerPlayNetworkHandlerMixin implements PlayerInteractEnt
 			info.cancel();
 		}
 	}
+
+	@Inject(method = "interact(Lnet/minecraft/util/Hand;)V", at = @At(value = "HEAD"), cancellable = true)
+	public void onPlayerInteractEntity(Hand hand, CallbackInfo info) {
+		PlayerEntity player = field_28963.player;
+		World world = player.getEntityWorld();
+
+		ActionResult result = UseEntityCallback.EVENT.invoker().interact(player, world, hand, field_28962, null);
+
+		if (result != ActionResult.PASS) {
+			info.cancel();
+		}
+	}
 }

fabric-events-interaction-v0/src/testmod/java/net/fabricmc/fabric/test/event/interaction/UseEntityTests.java

@@ -0,0 +1,38 @@
+/*
+ * Copyright (c) 2016, 2017, 2018, 2019 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.test.event.interaction;
+
+import net.minecraft.entity.passive.VillagerEntity;
+import net.minecraft.util.ActionResult;
+import net.minecraft.village.VillagerProfession;
+
+import net.fabricmc.api.ModInitializer;
+import net.fabricmc.fabric.api.event.player.UseEntityCallback;
+
+public class UseEntityTests implements ModInitializer {
+	@Override
+	public void onInitialize() {
+		// Disallow interactions with toolsmiths
+		UseEntityCallback.EVENT.register((player, world, hand, entity, hitResult) -> {
+			if (entity instanceof VillagerEntity villager && villager.getVillagerData().getProfession() == VillagerProfession.TOOLSMITH) {
+				return ActionResult.FAIL;
+			}
+
+			return ActionResult.PASS;
+		});
+	}
+}

fabric-events-interaction-v0/src/testmod/resources/fabric.mod.json

@@ -12,7 +12,8 @@
     "main": [
       "net.fabricmc.fabric.test.event.interaction.AttackBlockTests",
       "net.fabricmc.fabric.test.event.interaction.PlayerBreakBlockTests",
-      "net.fabricmc.fabric.test.event.interaction.PlayerPickBlockTests"
+      "net.fabricmc.fabric.test.event.interaction.PlayerPickBlockTests",
+      "net.fabricmc.fabric.test.event.interaction.UseEntityTests"
     ]
   }
 }