Get all logs in Tree Feller, and optimize performance

Tree Feller has been shown, both anecdotally and with timings, to put a strain on the server, and therefore is worthy of the effort of optimization.
Prior to this change, on jungle trees, Tree Feller would take around 20-40 milliseconds to process a Jungle Tree after the JIT kicked in, and around 15-25 milliseconds for a normal tree.

Additionally, logs would be left up in the air for jungle trees.

After this change, Tree Feller takes 2-5 milliseconds on normal trees, and 10-15 milliseconds on jungle trees, and no logs are left up in the air.

Changelog.txt

@@ -60,6 +60,7 @@ Version 1.4.07-dev
  ! Vampirism can now be enabled without having Skill Death Penalty enabled
  ! Admin and Party chat prefixes are now customizable
  ! Changed the color of party leader names in Party chat
+ ! Improved "Tree Feller" algorithm (Thanks Riking!)
  ! Improved profile saving
  ! Improved partial name matcher
  ! Improved update checker

src/main/java/com/gmail/nossr50/skills/woodcutting/Woodcutting.java

@@ -1,9 +1,12 @@
 package com.gmail.nossr50.skills.woodcutting;
 
 import java.util.ArrayList;
+import java.util.LinkedHashSet;
 import java.util.List;
+import java.util.Set;
 
 import org.bukkit.Material;
+import org.bukkit.block.BlockFace;
 import org.bukkit.block.BlockState;
 import org.bukkit.enchantments.Enchantment;
 import org.bukkit.inventory.ItemStack;
@@ -24,6 +27,8 @@ public final class Woodcutting {
     public static int leafBlowerUnlockLevel = AdvancedConfig.getInstance().getLeafBlowUnlockLevel();
     public static int treeFellerThreshold = Config.getInstance().getTreeFellerThreshold();
 
+    protected static boolean treeFellerReachedThreshold = false;
+
     protected enum ExperienceGainMethod {
         DEFAULT,
         TREE_FELLER,
@@ -121,58 +126,68 @@ public final class Woodcutting {
     }
 
     /**
-     * Processes Tree Feller for generic Trees
+     * The x/y differences to the blocks in a flat cylinder around the center
-     *
+     * block, which is excluded.
-     * @param blockState Block being checked
-     * @param treeFellerBlocks List of blocks to be removed
      */
-    protected static void processRegularTrees(BlockState blockState, List<BlockState> treeFellerBlocks) {
+    private static final int[][] directions = {
-        List<BlockState> futureCenterBlocks = new ArrayList<BlockState>();
+                            new int[] {-2, -1}, new int[] {-2, 0}, new int[] {-2, 1},
-
+        new int[] {-1, -2}, new int[] {-1, -1}, new int[] {-1, 0}, new int[] {-1, 1}, new int[] {-1, 2},
-        // Handle the blocks around 'block'
+        new int[] { 0, -2}, new int[] { 0, -1},                    new int[] { 0, 1}, new int[] { 0, 2},
-        for (int y = 0; y <= 1; y++) {
+        new int[] { 1, -2}, new int[] { 1, -1}, new int[] { 1, 0}, new int[] { 1, 1}, new int[] { 1, 2},
-            for (int x = -1; x <= 1; x++) {
+                            new int[] { 2, -1}, new int[] { 2, 0}, new int[] { 2, 1},
-                for (int z = -1; z <= 1; z++) {
+    };
-                    BlockState nextBlock = blockState.getBlock().getRelative(x, y, z).getState();
-                    handleBlock(nextBlock, futureCenterBlocks, treeFellerBlocks);
-
-                    if (WoodcuttingManager.treeFellerReachedThreshold) {
-                        return;
-                    }
-                }
-            }
-        }
-
-        // Recursive call for each log found
-        for (BlockState futureCenterBlock : futureCenterBlocks) {
-            if (WoodcuttingManager.treeFellerReachedThreshold) {
-                return;
-            }
-
-            processRegularTrees(futureCenterBlock, treeFellerBlocks);
-        }
-    }
 
     /**
-     * Processes Tree Feller for Red Mushrooms (Dome Shaped)
+     * Processes Tree Feller in a recursive manner
      *
      * @param blockState Block being checked
      * @param treeFellerBlocks List of blocks to be removed
      */
-    protected static void processRedMushroomTrees(BlockState blockState, List<BlockState> treeFellerBlocks) {
+    /*
+     * Algorithm: An int[][] of X/Z directions is created on static class
+     * initialization, representing a cylinder with radius of about 2 - the
+     * (0,0) center and all (+-2, +-2) corners are omitted.
+     *
+     * handleBlock() returns a boolean, which is used for the sole purpose of
+     * switching between these two behaviors:
+     *
+     * (Call blockState "this log" for the below explanation.)
+     *
+     *  [A] There is another log above this log (TRUNK)
+     *    Only the flat cylinder in the directions array is searched.
+     *  [B] There is not another log above this log (BRANCH AND TOP)
+     *    The cylinder in the directions array is extended up and down by 1
+     *    block in the Y-axis, and the block below this log is checked as
+     *    well. Due to the fact that the directions array will catch all
+     *    blocks on a red mushroom, the special method for it is eliminated.
+     *
+     * This algorithm has been shown to achieve a performance of 2-5
+     * milliseconds on regular trees and 10-15 milliseconds on jungle trees
+     * once the JIT has optimized the function (use the ability about 4 times
+     * before taking measurements).
+     */
+    protected static void processTree(BlockState blockState, LinkedHashSet<BlockState> treeFellerBlocks) {
         List<BlockState> futureCenterBlocks = new ArrayList<BlockState>();
 
-        // Handle the blocks around 'block'
+        // Check the block up and take different behavior (smaller search) if it's a log
-        for (int y = 0; y <= 1; y++) {
+        if (handleBlock(blockState.getBlock().getRelative(BlockFace.UP).getState(), futureCenterBlocks, treeFellerBlocks)) {
-            for (int x = -1; x <= 1; x++) {
+            for (int[] dir : directions) {
-                for (int z = -1; z <= 1; z++) {
+                handleBlock(blockState.getBlock().getRelative(dir[0], 0, dir[1]).getState(), futureCenterBlocks, treeFellerBlocks);
-                    BlockState nextBlock = blockState.getBlock().getRelative(x, y, z).getState();
-                    BlockState otherNextBlock = blockState.getBlock().getRelative(x, y - (y * 2), z).getState();
 
-                    handleBlock(nextBlock, futureCenterBlocks, treeFellerBlocks);
+                if (treeFellerReachedThreshold) {
-                    handleBlock(otherNextBlock, futureCenterBlocks, treeFellerBlocks);
+                    return;
+                }
+            }
+        }
+        else {
+            // Cover DOWN
+            handleBlock(blockState.getBlock().getRelative(BlockFace.DOWN).getState(), futureCenterBlocks, treeFellerBlocks);
+            // Search in a cube
+            for (int y = -1; y <= 1; y++) {
+                for (int[] dir : directions) {
+                    handleBlock(blockState.getBlock().getRelative(dir[0], y, dir[1]).getState(), futureCenterBlocks, treeFellerBlocks);
 
-                    if (WoodcuttingManager.treeFellerReachedThreshold) {
+                    if (treeFellerReachedThreshold) {
                         return;
                     }
                 }
@@ -181,11 +196,11 @@ public final class Woodcutting {
 
         // Recursive call for each log found
         for (BlockState futureCenterBlock : futureCenterBlocks) {
-            if (WoodcuttingManager.treeFellerReachedThreshold) {
+            if (treeFellerReachedThreshold) {
                 return;
             }
 
-            processRedMushroomTrees(futureCenterBlock, treeFellerBlocks);
+            processTree(futureCenterBlock, treeFellerBlocks);
         }
     }
 
@@ -196,7 +211,7 @@ public final class Woodcutting {
      * @param inHand tool being used
      * @return True if the tool can sustain the durability loss
      */
-    protected static boolean handleDurabilityLoss(List<BlockState> treeFellerBlocks, ItemStack inHand) {
+    protected static boolean handleDurabilityLoss(Set<BlockState> treeFellerBlocks, ItemStack inHand) {
         Material inHandMaterial = inHand.getType();
 
         if (inHandMaterial == Material.AIR) {
@@ -221,27 +236,36 @@ public final class Woodcutting {
     }
 
     /**
-     * Handle a block addition to the list of blocks to be removed and to the list of blocks used for future recursive calls of 'processRecursively()'
+     * Handle a block addition to the list of blocks to be removed and to the
+     * list of blocks used for future recursive calls of
+     * 'processTree()'
      *
      * @param blockState Block to be added
-     * @param futureCenterBlocks List of blocks that will be used to call 'processRecursively()'
+     * @param futureCenterBlocks List of blocks that will be used to call
+     *     'processTree()'
      * @param treeFellerBlocks List of blocks to be removed
+     * @return true if and only if the given blockState was a Log not already
+     *     in treeFellerBlocks.
      */
-    private static void handleBlock(BlockState blockState, List<BlockState> futureCenterBlocks, List<BlockState> treeFellerBlocks) {
+    private static boolean handleBlock(BlockState blockState, List<BlockState> futureCenterBlocks, Set<BlockState> treeFellerBlocks) {
-        if (!BlockUtils.affectedByTreeFeller(blockState) || mcMMO.getPlaceStore().isTrue(blockState) || treeFellerBlocks.contains(blockState)) {
+        if (treeFellerBlocks.contains(blockState) || mcMMO.getPlaceStore().isTrue(blockState)) {
-            return;
+            return false;
         }
 
-        treeFellerBlocks.add(blockState);
-
         if (treeFellerBlocks.size() > treeFellerThreshold) {
-            WoodcuttingManager.treeFellerReachedThreshold = true;
+            treeFellerReachedThreshold = true;
-            return;
         }
 
         // Without this check Tree Feller propagates through leaves until the threshold is hit
         if (BlockUtils.isLog(blockState)) {
+            treeFellerBlocks.add(blockState);
             futureCenterBlocks.add(blockState);
+            return true;
         }
+        else if (BlockUtils.isLeaves(blockState)) {
+            treeFellerBlocks.add(blockState);
+            return false;
+        }
+        return false;
     }
 }

src/main/java/com/gmail/nossr50/skills/woodcutting/WoodcuttingManager.java

@@ -1,7 +1,7 @@
 package com.gmail.nossr50.skills.woodcutting;
 
-import java.util.ArrayList;
+import java.util.LinkedHashSet;
-import java.util.List;
+import java.util.Set;
 
 import org.bukkit.Material;
 import org.bukkit.block.Block;
@@ -27,8 +27,6 @@ import com.gmail.nossr50.util.skills.CombatUtils;
 import com.gmail.nossr50.util.skills.SkillUtils;
 
 public class WoodcuttingManager extends SkillManager {
-    protected static boolean treeFellerReachedThreshold = false;
-
     public WoodcuttingManager(McMMOPlayer mcMMOPlayer) {
         super(mcMMOPlayer, SkillType.WOODCUTTING);
     }
@@ -74,28 +72,15 @@ public class WoodcuttingManager extends SkillManager {
      */
     public void processTreeFeller(BlockState blockState) {
         Player player = getPlayer();
-        List<BlockState> treeFellerBlocks = new ArrayList<BlockState>();
+        LinkedHashSet<BlockState> treeFellerBlocks = new LinkedHashSet<BlockState>();
 
-        switch (blockState.getType()) {
+        Woodcutting.treeFellerReachedThreshold = false;
-            case LOG:
-            case HUGE_MUSHROOM_1:
-                Woodcutting.processRegularTrees(blockState, treeFellerBlocks);
-                break;
 
-            case HUGE_MUSHROOM_2:
+        Woodcutting.processTree(blockState, treeFellerBlocks);
-                Woodcutting.processRedMushroomTrees(blockState, treeFellerBlocks);
-                break;
-
-            default:
-                if (ModUtils.isCustomLogBlock(blockState)) {
-                    Woodcutting.processRegularTrees(blockState, treeFellerBlocks);
-                }
-                break;
-        }
 
         // If the player is trying to break too many blocks
-        if (treeFellerReachedThreshold) {
+        if (Woodcutting.treeFellerReachedThreshold) {
-            treeFellerReachedThreshold = false;
+            Woodcutting.treeFellerReachedThreshold = false;
 
             player.sendMessage(LocaleLoader.getString("Woodcutting.Skills.TreeFellerThreshold"));
             return;
@@ -115,7 +100,7 @@ public class WoodcuttingManager extends SkillManager {
         }
 
         dropBlocks(treeFellerBlocks);
-        treeFellerReachedThreshold = false; // Reset the value after we're done with Tree Feller each time.
+        Woodcutting.treeFellerReachedThreshold = false; // Reset the value after we're done with Tree Feller each time.
     }
 
     /**
@@ -123,7 +108,7 @@ public class WoodcuttingManager extends SkillManager {
      *
      * @param treeFellerBlocks List of blocks to be dropped
      */
-    private void dropBlocks(List<BlockState> treeFellerBlocks) {
+    private void dropBlocks(Set<BlockState> treeFellerBlocks) {
         Player player = getPlayer();
         int xp = 0;
 

src/main/java/com/gmail/nossr50/util/BlockUtils.java

@@ -199,16 +199,6 @@ public final class BlockUtils {
         }
     }
 
-    /**
-     * Determine if a given block should be affected by Tree Feller
-     *
-     * @param blockState The {@link BlockState} of the block to check
-     * @return true if the block should affected by Tree Feller, false otherwise
-     */
-    public static boolean affectedByTreeFeller(BlockState blockState) {
-        return isLog(blockState) || isLeaves(blockState);
-    }
-
     /**
      * Check if a given block is a log
      *