Improve Force Relay/Lens movement logic for block structures (#4510)

(fixes #3917, #4411, and #4509)

- fix pushing the wrong way if Force Relay is part of a retracting
sticky piston's block structure
- don't assume that the location where the force push seemingly
originates from contains an unmovable block
- updated/expanded tests to cover movement behavior in (potentially
nested) block structures or with sticky pistons

Fabric/src/main/resources/botania.accesswidener

@@ -9,3 +9,4 @@ accessible class net/minecraft/client/renderer/RenderType$CompositeState
 extendable method net/minecraft/world/entity/vehicle/AbstractMinecart getDropItem ()Lnet/minecraft/world/item/Item;
 extendable class net/minecraft/world/level/levelgen/NoiseBasedChunkGenerator
 accessible class net/minecraft/client/resources/model/ModelManager$ReloadState
+mutable field net/minecraft/world/level/block/piston/PistonStructureResolver pistonPos Lnet/minecraft/core/BlockPos;

Forge/src/main/resources/META-INF/accesstransformer.cfg

@@ -1,3 +1,4 @@
 public net.minecraft.client.gui.screens.worldselection.WorldPreset <init>(Ljava/lang/String;)V
 public net.minecraft.client.renderer.RenderType$CompositeRenderType
 public net.minecraft.core.registries.BuiltInRegistries$RegistryBootstrap
+private-f net.minecraft.world.level.block.piston.PistonStructureResolver pistonPos

Xplat/src/main/java/vazkii/botania/common/block/ForceRelayBlock.java

@@ -30,6 +30,7 @@
 
 import vazkii.botania.api.BotaniaAPI;
 import vazkii.botania.common.handler.BotaniaSounds;
+import vazkii.botania.common.helper.ForcePushHelper;
 import vazkii.botania.common.item.WandOfTheForestItem;
 import vazkii.botania.common.item.lens.ForceLens;
 import vazkii.botania.network.EffectType;
@@ -52,7 +53,9 @@ public void onRemove(@NotNull BlockState state, @NotNull Level world, @NotNull B
 			var data = WorldData.get(world);
 
 			if (isMoving && newState.is(Blocks.MOVING_PISTON)) {
-				var moveDirection = newState.getValue(MovingPistonBlock.FACING);
+				var pistonDirection = newState.getValue(MovingPistonBlock.FACING);
+				// if being moved as part of a retracting sticky piston's block structure, reverse movement direction
+				var moveDirection = ForcePushHelper.isExtendingMovementContext() ? pistonDirection : pistonDirection.getOpposite();
 
 				var destPos = data.mapping.get(pos);
 				if (destPos != null) {
@@ -67,7 +70,7 @@ public void onRemove(@NotNull BlockState state, @NotNull Level world, @NotNull B
 
 					if (newState.getValue(MovingPistonBlock.TYPE) == PistonType.DEFAULT) {
 						// Move the actual bound blocks
-						if (ForceLens.moveBlocks(world, destPos.relative(moveDirection.getOpposite()), moveDirection)) {
+						if (ForceLens.moveBlocks(world, destPos.relative(moveDirection.getOpposite()), moveDirection, pos)) {
 							// Move dest side of our binding
 							data.mapping.put(newSrcPos, data.mapping.get(newSrcPos).relative(moveDirection));
 						}

Xplat/src/main/java/vazkii/botania/common/helper/ForcePushHelper.java

@@ -1,20 +1,50 @@
 package vazkii.botania.common.helper;
 
-import org.apache.commons.lang3.mutable.MutableInt;
+import net.minecraft.core.BlockPos;
+
+import java.util.ArrayDeque;
+import java.util.Deque;
 
 public class ForcePushHelper implements AutoCloseable {
-	private static final ThreadLocal<MutableInt> forcePushCounter = ThreadLocal.withInitial(MutableInt::new);
+	/**
+	 * Keeps track of push origins for nested force relay moves.
+	 */
+	private static final ThreadLocal<Deque<BlockPos>> forcePushOriginStack = ThreadLocal.withInitial(ArrayDeque::new);
+
+	/**
+	 * Keeps track of nested block push movement types (extending=true or retracting=false).
+	 */
+	private static final ThreadLocal<Deque<Boolean>> movementTypeContextStack = ThreadLocal.withInitial(ArrayDeque::new);
 
 	public static boolean isForcePush() {
-		return forcePushCounter.get().intValue() > 0;
+		return !forcePushOriginStack.get().isEmpty();
+	}
+
+	public static BlockPos getForcePushOrigin() {
+		if (!isForcePush()) {
+			throw new IllegalStateException("Not currently performing a Force Relay or Force Lens push");
+		}
+		return forcePushOriginStack.get().peek();
+	}
+
+	public static void pushMovementTypeContext(boolean extending) {
+		movementTypeContextStack.get().push(extending);
+	}
+
+	public static void popMovementTypeContext() {
+		movementTypeContextStack.get().pop();
+	}
+
+	public static boolean isExtendingMovementContext() {
+		return movementTypeContextStack.get().peek() == Boolean.TRUE;
 	}
 
-	public ForcePushHelper() {
-		forcePushCounter.get().increment();
+	public ForcePushHelper(BlockPos pushLocation) {
+		forcePushOriginStack.get().push(pushLocation.immutable());
 	}
 
 	@Override
 	public void close() {
-		forcePushCounter.get().decrement();
+		forcePushOriginStack.get().pop();
 	}
 }

Xplat/src/main/java/vazkii/botania/common/item/lens/ForceLens.java

@@ -30,16 +30,28 @@ public boolean collideBurst(ManaBurst burst, HitResult pos, boolean isManaBlock,
 				&& !burst.isFake()
 				&& !isManaBlock) {
 			BlockHitResult rtr = (BlockHitResult) pos;
-			moveBlocks(entity.level(), rtr.getBlockPos().relative(rtr.getDirection()), rtr.getDirection().getOpposite());
+			// mana burst could have been warped here, so don't assume that any block is unmovable
+			moveBlocks(entity.level(), rtr.getBlockPos().relative(rtr.getDirection()), rtr.getDirection().getOpposite(), ManaBurst.NO_SOURCE);
 		}
 
 		return shouldKill;
 	}
 
+	/**
+	 * Executes a force-push of blocks without an actual piston being present.
+	 * 
+	 * @param level            The level.
+	 * @param impliedPistonPos Position where to push block from.
+	 * @param direction        Direction to move block towards from the impliedPistonPos.
+	 * @param pushSourcePos    Position of the push source. The block at this position is considered unmovable,
+	 *                         and usually it would be the pushing piston.
+	 * @return {@code true} if blocks have started moving, or {@code false} if moving blocks failed,
+	 *         e.g. due to exceeded push limit or unmovable blocks being in the way.
+	 */
 	@SuppressWarnings("try")
-	public static boolean moveBlocks(Level level, BlockPos pistonPos, Direction direction) {
-		try (var ignored = new ForcePushHelper()) {
-			return ((PistonBaseBlockAccessor) Blocks.PISTON).botania_moveBlocks(level, pistonPos, direction, true);
+	public static boolean moveBlocks(Level level, BlockPos impliedPistonPos, Direction direction, BlockPos pushSourcePos) {
+		try (var ignored = new ForcePushHelper(pushSourcePos)) {
+			return ((PistonBaseBlockAccessor) Blocks.PISTON).botania_moveBlocks(level, impliedPistonPos, direction, true);
 		}
 	}
 

Xplat/src/main/java/vazkii/botania/mixin/PistonBaseBlockMixin.java

@@ -32,6 +32,7 @@ private void preMoveBlocks(Level level, BlockPos pos, Direction dir, boolean ext
 			CallbackInfoReturnable<Boolean> cir) {
 		if (!level.isClientSide()) {
 			EthicalTntHelper.startTrackingTntEntities();
+			ForcePushHelper.pushMovementTypeContext(extending);
 		}
 	}
 
@@ -42,6 +43,7 @@ private void preMoveBlocks(Level level, BlockPos pos, Direction dir, boolean ext
 	private void postMoveBlocks(Level level, BlockPos pos, Direction dir, boolean extending,
 			CallbackInfoReturnable<Boolean> cir) {
 		if (!level.isClientSide()) {
+			ForcePushHelper.popMovementTypeContext();
 			EthicalTntHelper.endTrackingTntEntitiesAndCheck();
 		}
 	}

Xplat/src/main/java/vazkii/botania/mixin/PistonStructureResolverMixin.java

@@ -0,0 +1,30 @@
+package vazkii.botania.mixin;
+
+import net.minecraft.core.BlockPos;
+import net.minecraft.core.Direction;
+import net.minecraft.world.level.Level;
+import net.minecraft.world.level.block.piston.PistonStructureResolver;
+
+import org.spongepowered.asm.mixin.Mixin;
+import org.spongepowered.asm.mixin.Shadow;
+import org.spongepowered.asm.mixin.injection.*;
+import org.spongepowered.asm.mixin.injection.callback.CallbackInfo;
+
+import vazkii.botania.common.helper.ForcePushHelper;
+
+@Mixin(value = PistonStructureResolver.class)
+public class PistonStructureResolverMixin {
+	// not final, due to access widener/transformer definition:
+	@SuppressWarnings("ShadowModifiers")
+	@Shadow
+	private BlockPos pistonPos;
+
+	/**
+	 * Since the pushing piston block is handled separately via its position,
+	 * replace it with the force relay's position when pushing blocks that way.
+	 */
+	@Inject(method = "<init>", at = @At(value = "RETURN"))
+	private void modifyForcePushOrigin(Level level, BlockPos pistonPos, Direction pistonDirection, boolean extending, CallbackInfo ci) {
+		this.pistonPos = ForcePushHelper.isForcePush() ? ForcePushHelper.getForcePushOrigin() : pistonPos;
+	}
+}

Xplat/src/main/java/vazkii/botania/test/block/ForceRelayTest.java

@@ -12,6 +12,7 @@
 import net.minecraft.gametest.framework.GameTest;
 import net.minecraft.gametest.framework.GameTestHelper;
 import net.minecraft.world.level.block.Blocks;
+import net.minecraft.world.level.block.state.BlockState;
 
 import vazkii.botania.common.block.BotaniaBlocks;
 import vazkii.botania.common.block.ForceRelayBlock;
@@ -44,9 +45,8 @@ public void testBasic(GameTestHelper helper) {
 		}).thenExecuteAfter(4, () -> {
 			var relayPos = initialRelay.south().west();
 			helper.assertBlockPresent(BotaniaBlocks.pistonRelay, relayPos);
-			// Andesite should not have moved
-			helper.assertBlockPresent(Blocks.ANDESITE, initialAndesite.south());
-			helper.assertBlockPresent(Blocks.SLIME_BLOCK, initialSlimeUnderAndesite.south());
+			helper.assertBlockPresent(Blocks.ANDESITE, initialAndesite.south().west());
+			helper.assertBlockPresent(Blocks.SLIME_BLOCK, initialSlimeUnderAndesite.south().west());
 			helper.assertBlockPresent(Blocks.DIORITE, initialDiorite.south().west());
 			helper.assertBlockPresent(Blocks.SLIME_BLOCK, initialSlimeUnderDiorite.south().west());
 
@@ -76,4 +76,115 @@ public void testImmovable(GameTestHelper helper) {
 							"binding to the destination block's original position");
 		}).thenSucceed();
 	}
+
+	@GameTest(template = "botania:block/piston_relay_sticky_move_no_pull")
+	public void testStickyPistonMoveNotPropagated(GameTestHelper helper) {
+		final var initialRelay = new BlockPos(1, 2, 2);
+		final var initialGranite = new BlockPos(3, 2, 3);
+		final var stickyPistonButton = new BlockPos(1, 2, 0);
+		final var nonstickyPistonButton = new BlockPos(1, 2, 5);
+
+		final var data = ForceRelayBlock.WorldData.get(helper.getLevel());
+		data.mapping.put(helper.absolutePos(initialRelay), helper.absolutePos(initialGranite));
+
+		helper.startSequence()
+				.thenExecute(() -> helper.pressButton(stickyPistonButton))
+				.thenExecuteAfter(4, () -> {
+					// relay should have moved, but not the bound block
+					final var relayAfter = initialRelay.south();
+					helper.assertBlockPresent(BotaniaBlocks.pistonRelay, relayAfter);
+					helper.assertBlockPresent(Blocks.POLISHED_GRANITE, initialGranite);
+					TestingUtil.assertEquals(data.mapping.get(helper.absolutePos(relayAfter)),
+							helper.absolutePos(initialGranite),
+							() -> "If relay is moved directly via sticky piston, it should update its source " +
+									"location, but retain the binding to the destination block's original position");
+					TestingUtil.assertEquals(data.mapping.get(helper.absolutePos(initialRelay)), null,
+							() -> "If relay is moved for whatever reason, its location data should be updated");
+				})
+				.thenExecuteAfter(20, () -> {
+					// piston should have retracted by now, nothing else should have changed
+					final var relayAfter = initialRelay.south();
+					helper.assertBlockPresent(BotaniaBlocks.pistonRelay, relayAfter);
+					helper.assertBlockPresent(Blocks.POLISHED_GRANITE, initialGranite);
+					TestingUtil.assertEquals(data.mapping.get(helper.absolutePos(relayAfter)),
+							helper.absolutePos(initialGranite),
+							() -> "If a sticky piston retracts from the relay, neither the relay nor its bound " +
+									"location should change");
+					TestingUtil.assertEquals(data.mapping.get(helper.absolutePos(initialRelay)), null,
+							() -> "If relay is not moved, its location data should not change");
+				})
+				.thenExecute(() -> helper.pressButton(nonstickyPistonButton))
+				.thenExecuteAfter(4, () -> {
+					// relay should have moved back and also moved the bound block
+					final var graniteAfter = initialGranite.north();
+					helper.assertBlockPresent(BotaniaBlocks.pistonRelay, initialRelay);
+					helper.assertBlockPresent(Blocks.POLISHED_GRANITE, graniteAfter);
+					TestingUtil.assertEquals(data.mapping.get(helper.absolutePos(initialRelay)),
+							helper.absolutePos(graniteAfter),
+							() -> "If relay is moved directly via non-sticky piston, " +
+									"it should update its source and bound locations");
+					TestingUtil.assertEquals(data.mapping.get(helper.absolutePos(initialRelay.south())), null,
+							() -> "If relay is moved for whatever reason, its location data should be updated");
+				})
+				.thenSucceed();
+	}
+
+	@GameTest(template = "botania:block/piston_relay_structure_pull")
+	public void testMovingInPistonStructure(GameTestHelper helper) {
+		final var button = new BlockPos(0, 2, 1);
+		final var initialRelay1 = new BlockPos(3, 2, 2);
+		final var initialRelay2 = new BlockPos(3, 2, 5);
+		final var initialBoundSlime = new BlockPos(2, 2, 4);
+		final var initialAndesite = new BlockPos(3, 4, 3);
+
+		final var data = ForceRelayBlock.WorldData.get(helper.getLevel());
+		data.mapping.put(helper.absolutePos(initialRelay1), helper.absolutePos(initialBoundSlime));
+		data.mapping.put(helper.absolutePos(initialRelay2), helper.absolutePos(initialAndesite));
+
+		helper.startSequence()
+				.thenExecute(() -> helper.pressButton(button))
+				.thenExecuteAfter(4, () -> {
+					// sticky piston should have moved the block structure with the first relay,
+					// which should have moved the other block structure with the second relay,
+					// which should have moved the andesite block
+					helper.assertBlockPresent(BotaniaBlocks.pistonRelay, initialRelay1.east());
+					helper.assertBlockPresent(BotaniaBlocks.pistonRelay, initialRelay2.east());
+					helper.assertBlockState(initialBoundSlime, BlockState::isAir,
+							() -> "Second slime block structure should have moved, " +
+									"leaving behind an air block at the originally bound location");
+					helper.assertBlockPresent(Blocks.POLISHED_ANDESITE, initialAndesite.east());
+					TestingUtil.assertEquals(data.mapping.get(helper.absolutePos(initialRelay1.east())),
+							helper.absolutePos(initialBoundSlime.east()),
+							() -> "Even if the structure was pushed by a sticky piston, the relay and bound " +
+									"locations should have been updated, since the relay was not pushed directly.");
+					TestingUtil.assertEquals(data.mapping.get(helper.absolutePos(initialRelay2.east())),
+							helper.absolutePos(initialAndesite.east()),
+							() -> "If a relay is moved indirectly as part of a block structure, its location and the " +
+									"bound block's location data should have been updated.");
+					TestingUtil.assertEquals(data.mapping.get(helper.absolutePos(initialRelay1)), null,
+							() -> "If relay is moved for whatever reason, its location data should be updated");
+					TestingUtil.assertEquals(data.mapping.get(helper.absolutePos(initialRelay2)), null,
+							() -> "If relay is moved for whatever reason, its location data should be updated");
+				})
+				.thenExecuteAfter(24, () -> {
+					// the sticky piston should have retracted by now and everything should have moved back
+					helper.assertBlockPresent(BotaniaBlocks.pistonRelay, initialRelay1);
+					helper.assertBlockPresent(BotaniaBlocks.pistonRelay, initialRelay2);
+					helper.assertBlockPresent(Blocks.SLIME_BLOCK, initialBoundSlime);
+					helper.assertBlockPresent(Blocks.POLISHED_ANDESITE, initialAndesite);
+					TestingUtil.assertEquals(data.mapping.get(helper.absolutePos(initialRelay1)),
+							helper.absolutePos(initialBoundSlime),
+							() -> "Even if the structure was pushed by a sticky piston, the relay and bound " +
+									"locations should have been updated, since the relay was not pushed directly.");
+					TestingUtil.assertEquals(data.mapping.get(helper.absolutePos(initialRelay2)),
+							helper.absolutePos(initialAndesite),
+							() -> "If a relay is moved indirectly as part of a block structure, its location and the " +
+									"bound block's location data should have been updated.");
+					TestingUtil.assertEquals(data.mapping.get(helper.absolutePos(initialRelay1.east())), null,
+							() -> "If relay is moved for whatever reason, its location data should be updated");
+					TestingUtil.assertEquals(data.mapping.get(helper.absolutePos(initialRelay2.east())), null,
+							() -> "If relay is moved for whatever reason, its location data should be updated");
+				})
+				.thenSucceed();
+	}
 }

Xplat/src/main/resources/botania_xplat.accesswidener

@@ -10,3 +10,4 @@ accessible class net/minecraft/client/renderer/RenderType$CompositeRenderType
 accessible class net/minecraft/client/renderer/RenderType$CompositeState
 extendable method net/minecraft/world/entity/vehicle/AbstractMinecart getDropItem ()Lnet/minecraft/world/item/Item;
 extendable class net/minecraft/world/level/levelgen/NoiseBasedChunkGenerator
+mutable field net/minecraft/world/level/block/piston/PistonStructureResolver pistonPos Lnet/minecraft/core/BlockPos;

Xplat/src/main/resources/botania_xplat.mixins.json

@@ -39,6 +39,7 @@
     "NearestAttackableTargetGoalAccessor",
     "PistonBaseBlockAccessor",
     "PistonBaseBlockMixin",
+    "PistonStructureResolverMixin",
     "PlayerMixin",
     "PollinateGoalMixin",
     "RecipeManagerAccessor",

Xplat/src/main/resources/data/botania/gametest/structures/block/piston_relay_sticky_move_no_pull.snbt

@@ -0,0 +1,21 @@
+{
+    DataVersion: 3465,
+    size: [5, 3, 6],
+    data: [
+        {pos: [1, 1, 0], state: "minecraft:polished_blackstone_button{face:wall,facing:north,powered:false}"},
+        {pos: [1, 1, 1], state: "minecraft:sticky_piston{extended:false,facing:south}"},
+        {pos: [1, 1, 2], state: "botania:piston_relay"},
+        {pos: [1, 1, 4], state: "minecraft:piston{extended:false,facing:north}"},
+        {pos: [1, 1, 5], state: "minecraft:polished_blackstone_button{face:wall,facing:south,powered:false}"},
+        {pos: [3, 1, 3], state: "minecraft:polished_granite"}
+    ],
+    entities: [],
+    palette: [
+        "minecraft:sticky_piston{extended:false,facing:south}",
+        "botania:piston_relay",
+        "minecraft:piston{extended:false,facing:north}",
+        "minecraft:polished_granite",
+        "minecraft:polished_blackstone_button{face:wall,facing:north,powered:false}",
+        "minecraft:polished_blackstone_button{face:wall,facing:south,powered:false}"
+    ]
+}

Xplat/src/main/resources/data/botania/gametest/structures/block/piston_relay_structure_pull.snbt

@@ -0,0 +1,31 @@
+{
+    DataVersion: 3465,
+    size: [7, 5, 7],
+    data: [
+        {pos: [0, 1, 1], state: "minecraft:polished_blackstone_button{face:wall,facing:west,powered:false}"},
+        {pos: [1, 1, 1], state: "minecraft:sticky_piston{extended:false,facing:east}"},
+        {pos: [2, 1, 1], state: "minecraft:slime_block"},
+        {pos: [2, 1, 2], state: "minecraft:polished_diorite"},
+        {pos: [2, 1, 4], state: "minecraft:slime_block"},
+        {pos: [2, 1, 5], state: "minecraft:polished_granite"},
+        {pos: [3, 1, 1], state: "minecraft:slime_block"},
+        {pos: [3, 1, 2], state: "botania:piston_relay"},
+        {pos: [3, 1, 4], state: "minecraft:slime_block"},
+        {pos: [3, 1, 5], state: "botania:piston_relay"},
+        {pos: [4, 1, 1], state: "minecraft:slime_block"},
+        {pos: [4, 1, 2], state: "minecraft:polished_diorite"},
+        {pos: [4, 1, 4], state: "minecraft:slime_block"},
+        {pos: [4, 1, 5], state: "minecraft:polished_granite"},
+        {pos: [3, 3, 3], state: "minecraft:polished_andesite"}
+    ],
+    entities: [],
+    palette: [
+        "minecraft:sticky_piston{extended:false,facing:east}",
+        "minecraft:slime_block",
+        "minecraft:polished_diorite",
+        "minecraft:polished_granite",
+        "botania:piston_relay",
+        "minecraft:polished_andesite",
+        "minecraft:polished_blackstone_button{face:wall,facing:west,powered:false}"
+    ]
+}