De-deprecate BlockData#getDestroySpeed

6 files changed, 206 insertions, 28 deletions

diff --git a/patches/api/0214-Add-Destroy-Speed-API.patch b/patches/api/0214-Add-Destroy-Speed-API.patch
index 509bde4c8a..d6e08a09df 100644
--- a/patches/api/0214-Add-Destroy-Speed-API.patch
+++ b/patches/api/0214-Add-Destroy-Speed-API.patch
@@ -42,10 +42,10 @@ index 25db31b2e9a6d75f0c59f75237842f9ad7d1c350..75c2aadb0a2baebe8b2625ad11b16380
 +    // Paper end - destroy speed API
  }
 diff --git a/src/main/java/org/bukkit/block/data/BlockData.java b/src/main/java/org/bukkit/block/data/BlockData.java
-index cd3b3e05cc825cfedec07f9a2a1e0b7b2a8866d6..a2dc7376b2a3d386b671c894f73389139e0d97bf 100644
+index cd3b3e05cc825cfedec07f9a2a1e0b7b2a8866d6..890a511355dd3f2aa9330fdc72c0fb4b3e44e5cb 100644
 --- a/src/main/java/org/bukkit/block/data/BlockData.java
 +++ b/src/main/java/org/bukkit/block/data/BlockData.java
-@@ -266,4 +266,35 @@ public interface BlockData extends Cloneable {
+@@ -266,4 +266,33 @@ public interface BlockData extends Cloneable {
      @NotNull
      @ApiStatus.Experimental
      BlockState createBlockState();
@@ -58,10 +58,9 @@ index cd3b3e05cc825cfedec07f9a2a1e0b7b2a8866d6..a2dc7376b2a3d386b671c894f7338913
 +     *
 +     * @param itemStack {@link ItemStack} used to mine this Block
 +     * @return the speed that this Block will be mined by the given {@link ItemStack}
-+     * @deprecated the destroy speed of a block was never purely tied to an item stack. Since 1.21 enchantments
-+     * also use complex effects that require a consuming player to compute their effects, including mining efficiency.
++     * @apiNote this method assumes default player state and hence, e.g., does not take into account changed
++     * player attributes or potion effects.
 +     */
-+    @Deprecated(forRemoval = true, since = "1.21")
 +    default float getDestroySpeed(final @NotNull ItemStack itemStack) {
 +        return this.getDestroySpeed(itemStack, false);
 +    }
@@ -74,10 +73,9 @@ index cd3b3e05cc825cfedec07f9a2a1e0b7b2a8866d6..a2dc7376b2a3d386b671c894f7338913
 +     * @param itemStack {@link ItemStack} used to mine this Block
 +     * @param considerEnchants true to look at enchants on the itemstack
 +     * @return the speed that this Block will be mined by the given {@link ItemStack}
-+     * @deprecated the destroy speed of a block was never purely tied to an item stack. Since 1.21 enchantments
-+     * also use complex effects that require a consuming player to compute their effects, including mining efficiency.
++     * @apiNote this method assumes default player state and hence, e.g., does not take into account changed
++     * player attributes or potion effects.
 +     */
-+    @Deprecated(forRemoval = true, since = "1.21")
 +    float getDestroySpeed(@NotNull ItemStack itemStack, boolean considerEnchants);
 +    // Paper end - destroy speed API
  }
diff --git a/patches/api/0357-Block-Ticking-API.patch b/patches/api/0357-Block-Ticking-API.patch
index 5371277e73..1836045273 100644
--- a/patches/api/0357-Block-Ticking-API.patch
+++ b/patches/api/0357-Block-Ticking-API.patch
@@ -51,11 +51,11 @@ index 87327df6a37668eaf87394b6b049e6d4badec6df..a13c8ddd4a1222e7a16debb61769af37
  
      /**
 diff --git a/src/main/java/org/bukkit/block/data/BlockData.java b/src/main/java/org/bukkit/block/data/BlockData.java
-index a2dc7376b2a3d386b671c894f73389139e0d97bf..26b70af4a1f3db5b17957bfa644e758603f8863c 100644
+index 890a511355dd3f2aa9330fdc72c0fb4b3e44e5cb..54664651f34311e95f6c2dcfd93e58477beda8c2 100644
 --- a/src/main/java/org/bukkit/block/data/BlockData.java
 +++ b/src/main/java/org/bukkit/block/data/BlockData.java
-@@ -297,4 +297,14 @@ public interface BlockData extends Cloneable {
-     @Deprecated(forRemoval = true, since = "1.21")
+@@ -295,4 +295,14 @@ public interface BlockData extends Cloneable {
+      */
      float getDestroySpeed(@NotNull ItemStack itemStack, boolean considerEnchants);
      // Paper end - destroy speed API
 +
diff --git a/patches/api/0425-Add-API-to-get-the-collision-shape-of-a-block-before.patch b/patches/api/0425-Add-API-to-get-the-collision-shape-of-a-block-before.patch
index b5d2e36ec2..3bb30787dc 100644
--- a/patches/api/0425-Add-API-to-get-the-collision-shape-of-a-block-before.patch
+++ b/patches/api/0425-Add-API-to-get-the-collision-shape-of-a-block-before.patch
@@ -6,7 +6,7 @@ Subject: [PATCH] Add API to get the collision shape of a block before it's
 
 
 diff --git a/src/main/java/org/bukkit/block/data/BlockData.java b/src/main/java/org/bukkit/block/data/BlockData.java
-index 26b70af4a1f3db5b17957bfa644e758603f8863c..a1ee73254b1389396e7d53f08abe4b3780bd3d0e 100644
+index 54664651f34311e95f6c2dcfd93e58477beda8c2..0ecc54bd810a2805b7209d9433b76743500e45a8 100644
 --- a/src/main/java/org/bukkit/block/data/BlockData.java
 +++ b/src/main/java/org/bukkit/block/data/BlockData.java
 @@ -205,6 +205,19 @@ public interface BlockData extends Cloneable {
diff --git a/patches/server/0436-Add-Destroy-Speed-API.patch b/patches/server/0436-Add-Destroy-Speed-API.patch
index 6832fd221a..43e752ecb5 100644
--- a/patches/server/0436-Add-Destroy-Speed-API.patch
+++ b/patches/server/0436-Add-Destroy-Speed-API.patch
@@ -5,11 +5,40 @@ Subject: [PATCH] Add Destroy Speed API
 
 Co-authored-by: Jake Potrebic <[email protected]>
 
+diff --git a/src/main/java/net/minecraft/world/entity/ai/attributes/AttributeInstance.java b/src/main/java/net/minecraft/world/entity/ai/attributes/AttributeInstance.java
+index 36c943d709c1c0ae49ec0baf0ccf7b6cb9a2ecf3..d28f9e077a50122e86848cfa9db83f6b0e8eef6c 100644
+--- a/src/main/java/net/minecraft/world/entity/ai/attributes/AttributeInstance.java
++++ b/src/main/java/net/minecraft/world/entity/ai/attributes/AttributeInstance.java
+@@ -143,20 +143,20 @@ public class AttributeInstance {
+         double d = this.getBaseValue();
+ 
+         for (AttributeModifier attributeModifier : this.getModifiersOrEmpty(AttributeModifier.Operation.ADD_VALUE)) {
+-            d += attributeModifier.amount();
++            d += attributeModifier.amount(); // Paper - destroy speed API - diff on change
+         }
+ 
+         double e = d;
+ 
+         for (AttributeModifier attributeModifier2 : this.getModifiersOrEmpty(AttributeModifier.Operation.ADD_MULTIPLIED_BASE)) {
+-            e += d * attributeModifier2.amount();
++            e += d * attributeModifier2.amount(); // Paper - destroy speed API - diff on change
+         }
+ 
+         for (AttributeModifier attributeModifier3 : this.getModifiersOrEmpty(AttributeModifier.Operation.ADD_MULTIPLIED_TOTAL)) {
+-            e *= 1.0 + attributeModifier3.amount();
++            e *= 1.0 + attributeModifier3.amount(); // Paper - destroy speed API - diff on change
+         }
+ 
+-        return this.attribute.value().sanitizeValue(e);
++        return attribute.value().sanitizeValue(e); // Paper - destroy speed API - diff on change
+     }
+ 
+     private Collection<AttributeModifier> getModifiersOrEmpty(AttributeModifier.Operation operation) {
 diff --git a/src/main/java/org/bukkit/craftbukkit/block/data/CraftBlockData.java b/src/main/java/org/bukkit/craftbukkit/block/data/CraftBlockData.java
-index 9953b6b36cbcbfd1756bac478b568ca5700fc898..b318b287572ced45cc3e9f0691a98a037635fbce 100644
+index 9953b6b36cbcbfd1756bac478b568ca5700fc898..33869e725c3b3f2120fa36ca468019a78321e862 100644
 --- a/src/main/java/org/bukkit/craftbukkit/block/data/CraftBlockData.java
 +++ b/src/main/java/org/bukkit/craftbukkit/block/data/CraftBlockData.java
-@@ -721,4 +721,27 @@ public class CraftBlockData implements BlockData {
+@@ -721,4 +721,35 @@ public class CraftBlockData implements BlockData {
      public BlockState createBlockState() {
          return CraftBlockStates.getBlockState(this.state, null);
      }
@@ -20,20 +49,171 @@ index 9953b6b36cbcbfd1756bac478b568ca5700fc898..b318b287572ced45cc3e9f0691a98a03
 +        net.minecraft.world.item.ItemStack nmsItemStack = CraftItemStack.unwrap(itemStack);
 +        float speed = nmsItemStack.getDestroySpeed(this.state);
 +        if (speed > 1.0F && considerEnchants) {
-+            final net.minecraft.core.Holder<net.minecraft.world.item.enchantment.Enchantment> efficiencyHolder = net.minecraft.server.MinecraftServer
-+                .getServer()
-+                .registryAccess()
-+                .registryOrThrow(net.minecraft.core.registries.Registries.ENCHANTMENT)
-+                .getHolderOrThrow(net.minecraft.world.item.enchantment.Enchantments.EFFICIENCY);
-+
-+            final int enchantLevel = net.minecraft.world.item.enchantment.EnchantmentHelper.getItemEnchantmentLevel(
-+                efficiencyHolder, nmsItemStack
++            final net.minecraft.core.Holder<net.minecraft.world.entity.ai.attributes.Attribute> attribute = net.minecraft.world.entity.ai.attributes.Attributes.MINING_EFFICIENCY;
++            // Logic sourced from AttributeInstance#calculateValue
++            final double initialBaseValue = attribute.value().getDefaultValue();
++            final org.apache.commons.lang3.mutable.MutableDouble modifiedBaseValue = new org.apache.commons.lang3.mutable.MutableDouble(initialBaseValue);
++            final org.apache.commons.lang3.mutable.MutableDouble baseValMul = new org.apache.commons.lang3.mutable.MutableDouble(1);
++            final org.apache.commons.lang3.mutable.MutableDouble totalValMul = new org.apache.commons.lang3.mutable.MutableDouble(1);
++
++            net.minecraft.world.item.enchantment.EnchantmentHelper.forEachModifier(
++                nmsItemStack, net.minecraft.world.entity.EquipmentSlot.MAINHAND, (attributeHolder, attributeModifier) -> {
++                    switch (attributeModifier.operation()) {
++                        case ADD_VALUE -> modifiedBaseValue.add(attributeModifier.amount());
++                        case ADD_MULTIPLIED_BASE -> baseValMul.add(attributeModifier.amount());
++                        case ADD_MULTIPLIED_TOTAL -> totalValMul.setValue(totalValMul.doubleValue() * (1D + attributeModifier.amount()));
++                    }
++                }
 +            );
-+            if (enchantLevel > 0) {
-+                speed += enchantLevel * enchantLevel + 1;
-+            }
++
++            final double actualModifier = modifiedBaseValue.doubleValue() * baseValMul.doubleValue() * totalValMul.doubleValue();
++
++            speed += (float) attribute.value().sanitizeValue(actualModifier);
 +        }
 +        return speed;
 +    }
 +    // Paper end - destroy speed API
  }
+diff --git a/src/test/java/io/papermc/paper/block/CraftBlockDataDestroySpeedTest.java b/src/test/java/io/papermc/paper/block/CraftBlockDataDestroySpeedTest.java
+new file mode 100644
+index 0000000000000000000000000000000000000000..62aef9abab896491f7806490184fc6899ec36c57
+--- /dev/null
++++ b/src/test/java/io/papermc/paper/block/CraftBlockDataDestroySpeedTest.java
+@@ -0,0 +1,137 @@
++package io.papermc.paper.block;
++
++import java.util.List;
++import java.util.Optional;
++import net.minecraft.core.Holder;
++import net.minecraft.core.HolderSet;
++import net.minecraft.core.component.DataComponentMap;
++import net.minecraft.core.component.DataComponents;
++import net.minecraft.network.chat.Component;
++import net.minecraft.world.entity.EquipmentSlot;
++import net.minecraft.world.entity.EquipmentSlotGroup;
++import net.minecraft.world.entity.ai.attributes.AttributeInstance;
++import net.minecraft.world.entity.ai.attributes.AttributeModifier;
++import net.minecraft.world.entity.ai.attributes.Attributes;
++import net.minecraft.world.item.Items;
++import net.minecraft.world.item.enchantment.Enchantment;
++import net.minecraft.world.item.enchantment.EnchantmentEffectComponents;
++import net.minecraft.world.item.enchantment.EnchantmentHelper;
++import net.minecraft.world.item.enchantment.ItemEnchantments;
++import net.minecraft.world.item.enchantment.LevelBasedValue;
++import net.minecraft.world.item.enchantment.effects.EnchantmentAttributeEffect;
++import net.minecraft.world.level.block.Blocks;
++import net.minecraft.world.level.block.state.BlockState;
++import org.bukkit.craftbukkit.block.data.CraftBlockData;
++import org.bukkit.craftbukkit.inventory.CraftItemStack;
++import org.bukkit.inventory.ItemStack;
++import org.bukkit.support.AbstractTestingBase;
++import org.bukkit.util.Vector;
++import org.jetbrains.annotations.NotNull;
++import org.junit.jupiter.api.Assertions;
++import org.junit.jupiter.api.Test;
++
++import static net.minecraft.resources.ResourceLocation.fromNamespaceAndPath;
++
++/**
++ * CraftBlockData's {@link org.bukkit.craftbukkit.block.data.CraftBlockData#getDestroySpeed(ItemStack, boolean)}
++ * uses a reimplementation of AttributeValue without any map to avoid attribute instance allocation and mutation
++ * for 0 gain.
++ * <p>
++ * This test is responsible for ensuring that said logic emits the expected destroy speed under heavy attribute
++ * modifier use.
++ */
++public class CraftBlockDataDestroySpeedTest extends AbstractTestingBase {
++
++    @Test
++    public void testCorrectEnchantmentDestroySpeedComputation() {
++        // Construct fake enchantment that has *all and multiple of* operations
++        final Enchantment speedEnchantment = speedEnchantment();
++        final BlockState blockStateToMine = Blocks.STONE.defaultBlockState();
++
++        final ItemEnchantments.Mutable mutable = new ItemEnchantments.Mutable(ItemEnchantments.EMPTY);
++        mutable.set(Holder.direct(speedEnchantment), 1);
++
++        final net.minecraft.world.item.ItemStack itemStack = new net.minecraft.world.item.ItemStack(Items.DIAMOND_PICKAXE);
++        itemStack.set(DataComponents.ENCHANTMENTS, mutable.toImmutable());
++
++        // Compute expected value by running the entire attribute instance chain
++        final AttributeInstance dummyInstance = new AttributeInstance(Attributes.MINING_EFFICIENCY, $ -> {
++        });
++        EnchantmentHelper.forEachModifier(itemStack, EquipmentSlot.MAINHAND, (attributeHolder, attributeModifier) -> {
++            if (attributeHolder.is(Attributes.MINING_EFFICIENCY)) dummyInstance.addTransientModifier(attributeModifier);
++        });
++
++        final double toolSpeed = itemStack.getDestroySpeed(blockStateToMine);
++        final double expectedSpeed = toolSpeed <= 1.0F ? toolSpeed : toolSpeed + dummyInstance.getValue();
++
++        // API stack + computation
++        final CraftItemStack craftMirror = CraftItemStack.asCraftMirror(itemStack);
++        final CraftBlockData data = CraftBlockData.createData(blockStateToMine);
++        final float actualSpeed = data.getDestroySpeed(craftMirror, true);
++
++        Assertions.assertEquals(expectedSpeed, actualSpeed, Vector.getEpsilon());
++    }
++
++    /**
++     * Complex enchantment that holds attribute modifiers for the mining efficiency.
++     * The enchantment holds 2 of each operation to also ensure that such behaviour works correctly.
++     *
++     * @return the enchantment.
++     */
++    private static @NotNull Enchantment speedEnchantment() {
++        return new Enchantment(
++            Component.empty(),
++            new Enchantment.EnchantmentDefinition(
++                HolderSet.empty(),
++                Optional.empty(),
++                0, 0,
++                Enchantment.constantCost(0),
++                Enchantment.constantCost(0),
++                0,
++                List.of(EquipmentSlotGroup.ANY)
++            ),
++            HolderSet.empty(),
++            DataComponentMap.builder()
++                .set(EnchantmentEffectComponents.ATTRIBUTES, List.of(
++                    new EnchantmentAttributeEffect(
++                        fromNamespaceAndPath("paper", "base1"),
++                        Attributes.MINING_EFFICIENCY,
++                        LevelBasedValue.constant(1),
++                        AttributeModifier.Operation.ADD_VALUE
++                    ),
++                    new EnchantmentAttributeEffect(
++                        fromNamespaceAndPath("paper", "base2"),
++                        Attributes.MINING_EFFICIENCY,
++                        LevelBasedValue.perLevel(3),
++                        AttributeModifier.Operation.ADD_VALUE
++                    ),
++                    new EnchantmentAttributeEffect(
++                        fromNamespaceAndPath("paper", "base-mul1"),
++                        Attributes.MINING_EFFICIENCY,
++                        LevelBasedValue.perLevel(7),
++                        AttributeModifier.Operation.ADD_MULTIPLIED_BASE
++                    ),
++                    new EnchantmentAttributeEffect(
++                        fromNamespaceAndPath("paper", "base-mul2"),
++                        Attributes.MINING_EFFICIENCY,
++                        LevelBasedValue.constant(10),
++                        AttributeModifier.Operation.ADD_MULTIPLIED_BASE
++                    ),
++                    new EnchantmentAttributeEffect(
++                        fromNamespaceAndPath("paper", "total-mul1"),
++                        Attributes.MINING_EFFICIENCY,
++                        LevelBasedValue.constant(.2f),
++                        AttributeModifier.Operation.ADD_MULTIPLIED_TOTAL
++                    ),
++                    new EnchantmentAttributeEffect(
++                        fromNamespaceAndPath("paper", "total-mul2"),
++                        Attributes.MINING_EFFICIENCY,
++                        LevelBasedValue.constant(-.5F),
++                        AttributeModifier.Operation.ADD_MULTIPLIED_TOTAL
++                    )
++                ))
++                .build()
++        );
++    }
++
++}
diff --git a/patches/server/0736-Block-Ticking-API.patch b/patches/server/0736-Block-Ticking-API.patch
index 3da9c64590..9e5247f35f 100644
--- a/patches/server/0736-Block-Ticking-API.patch
+++ b/patches/server/0736-Block-Ticking-API.patch
@@ -46,10 +46,10 @@ index 6d10396347b69d9243ab902ecc68ede93fa17b7d..af219df5267589300f0ad1d30fa5c81a
      // Paper end
  }
 diff --git a/src/main/java/org/bukkit/craftbukkit/block/data/CraftBlockData.java b/src/main/java/org/bukkit/craftbukkit/block/data/CraftBlockData.java
-index b318b287572ced45cc3e9f0691a98a037635fbce..53dddaf1fb608312991739d488b8cd2dadc58e22 100644
+index 33869e725c3b3f2120fa36ca468019a78321e862..64ddd6d1c40dc91b6e7fc3118403415bb4533d97 100644
 --- a/src/main/java/org/bukkit/craftbukkit/block/data/CraftBlockData.java
 +++ b/src/main/java/org/bukkit/craftbukkit/block/data/CraftBlockData.java
-@@ -744,4 +744,11 @@ public class CraftBlockData implements BlockData {
+@@ -752,4 +752,11 @@ public class CraftBlockData implements BlockData {
          return speed;
      }
      // Paper end - destroy speed API
diff --git a/patches/server/0906-Add-API-to-get-the-collision-shape-of-a-block-before.patch b/patches/server/0906-Add-API-to-get-the-collision-shape-of-a-block-before.patch
index ea30c04785..f0fcc8b9eb 100644
--- a/patches/server/0906-Add-API-to-get-the-collision-shape-of-a-block-before.patch
+++ b/patches/server/0906-Add-API-to-get-the-collision-shape-of-a-block-before.patch
@@ -6,7 +6,7 @@ Subject: [PATCH] Add API to get the collision shape of a block before it's
 
 
 diff --git a/src/main/java/org/bukkit/craftbukkit/block/data/CraftBlockData.java b/src/main/java/org/bukkit/craftbukkit/block/data/CraftBlockData.java
-index 53dddaf1fb608312991739d488b8cd2dadc58e22..17933c51abf657335fd449635f198c6802adf14c 100644
+index 64ddd6d1c40dc91b6e7fc3118403415bb4533d97..0daa0bf7e56aa7228d89867500cb780b37f06541 100644
 --- a/src/main/java/org/bukkit/craftbukkit/block/data/CraftBlockData.java
 +++ b/src/main/java/org/bukkit/craftbukkit/block/data/CraftBlockData.java
 @@ -679,6 +679,20 @@ public class CraftBlockData implements BlockData {