Fixes some savage fail problems

Fixes item not being returned when salvage fails
Removes some redundancy when giving back items
Makes extended salvage return 50% of the salvage (no items if only one item would be returned)
Adds a better message when failing to salvage extended salvage items
Fixes a bug in the caching of smith presets

src/main/java/net/knarcraft/blacksmith/config/SmithPreset.java

@@ -46,7 +46,7 @@ public enum SmithPreset {
 
     private final SmithPresetFilter[] filters;
     private static final Map<SmithPreset, Set<String>> presetMaterialNames = new HashMap<>();
-    private static final Map<SmithPresetFilter, Set<String>> filterMaterialNames = new HashMap<>();
+    private static final Map<SmithPreset, Map<SmithPresetFilter, Set<String>>> filterMaterialNames = new HashMap<>();
     private static Set<Material> armor = null;
     private static Set<Material> ranged = null;
     private static Set<Material> weapons = null;
@@ -145,8 +145,8 @@ public enum SmithPreset {
                 materialNames = presetMaterialNames.get(preset);
             }
         } else {
-            if (filterMaterialNames.containsKey(filter)) {
-                materialNames = filterMaterialNames.get(filter);
+            if (filterMaterialNames.containsKey(preset) && filterMaterialNames.get(preset).containsKey(filter)) {
+                materialNames = filterMaterialNames.get(preset).get(filter);
             }
         }
 
@@ -154,7 +154,8 @@ public enum SmithPreset {
         if (materialNames == null) {
             if (filter != null) {
                 materialNames = preset.getMaterialNames(filter);
-                filterMaterialNames.put(filter, materialNames);
+                filterMaterialNames.putIfAbsent(preset, new HashMap<>());
+                filterMaterialNames.get(preset).put(filter, materialNames);
             } else {
                 materialNames = preset.getMaterialNames();
                 presetMaterialNames.put(preset, materialNames);

src/main/java/net/knarcraft/blacksmith/config/scrapper/ScrapperNPCSettings.java

@@ -127,10 +127,20 @@ public class ScrapperNPCSettings implements TraitSettings<ScrapperSetting> {
      * @return <p>The salvage fail message</p>
      */
     @NotNull
-    public String getFailMessage() {
+    public String getFailSalvageMessage() {
         return asString(ScrapperSetting.FAIL_SALVAGE_MESSAGE);
     }
 
+    /**
+     * Gets the message to display when a scrapper fails to salvage an item without durability
+     *
+     * @return <p>The extended salvage fail message</p>
+     */
+    @NotNull
+    public String getFailExtendedSalvageMessage() {
+        return asString(ScrapperSetting.FAIL_SALVAGE_EXTENDED_MESSAGE);
+    }
+
     /**
      * Gets the message to use for displaying salvage cost
      *

src/main/java/net/knarcraft/blacksmith/config/scrapper/ScrapperSetting.java

@@ -146,6 +146,13 @@ public enum ScrapperSetting implements Setting {
             "next time?", "The message to display when a scrapper fails to salvage an item because of the " +
             "fail chance.", true, true),
 
+    /**
+     * The message displayed if a salvage of an item without durability is unsuccessful
+     */
+    FAIL_SALVAGE_EXTENDED_MESSAGE("failExtendedSalvageMessage", SettingValueType.STRING,
+            "&cWhoops! I was unable to extract much, if any, salvage! Maybe next time?",
+            "The message to display when the scrapper fails to salvage an item without durability", true, true),
+
     /**
      * The message displayed if a player presents a different item after seeing the price to reforge an item
      */

src/main/java/net/knarcraft/blacksmith/trait/ReforgeSession.java

@@ -34,7 +34,6 @@ public class ReforgeSession extends Session implements Runnable {
     private final ItemStack itemToReforge;
     private final BlacksmithNPCSettings config;
     private static List<String> enchantments = null;
-    private static final Random random = new Random();
 
     /**
      * Instantiates a new session
@@ -123,17 +122,8 @@ public class ReforgeSession extends Session implements Runnable {
      * Gives the reforged item back to the player
      */
     private void giveReforgedItem() {
-        if (this.config.getMaxReforgeDelay() > 0) {
-            //If the player isn't online, or the player cannot fit the item, drop the item to prevent it from disappearing
-            if (this.config.getDropItem() || !this.player.isOnline() || this.player.getInventory().firstEmpty() == -1) {
-                this.player.getWorld().dropItemNaturally(this.npc.getEntity().getLocation(), this.itemToReforge);
-            } else {
-                this.player.getInventory().addItem(this.itemToReforge);
-            }
-        } else {
-            //It can be assumed as this happens instantly, that the player still has the item's previous slot selected
-            this.player.getInventory().setItemInMainHand(this.itemToReforge);
-        }
+        giveResultingItem(this.config.getMaxReforgeDelay() > 0, this.config.getDropItem(), this.npc,
+                this.itemToReforge);
     }
 
     /**
@@ -156,7 +146,10 @@ public class ReforgeSession extends Session implements Runnable {
      */
     private void succeedReforge() {
         // Remove any damage done to the item
-        updateDamage(this.itemToReforge, 0);
+        if (ItemHelper.updateDamage(this.itemToReforge, 0)) {
+            BlacksmithPlugin.getInstance().getLogger().log(Level.WARNING, "Unable to update damage for " +
+                    this.itemToReforge);
+        }
 
         //Replace damaged anvils with a normal anvil
         if (ItemHelper.isAnvil(this.itemToReforge.getType(), true)) {
@@ -216,15 +209,7 @@ public class ReforgeSession extends Session implements Runnable {
         }
 
         //Damage the item
-        short currentItemDurability = ItemHelper.getDurability(this.itemToReforge);
-        short newDurability = (short) (currentItemDurability + (currentItemDurability * random.nextInt(8)));
-        short maxDurability = this.itemToReforge.getType().getMaxDurability();
-        if (newDurability <= 0) {
-            newDurability = (short) (maxDurability / 3);
-        } else if (currentItemDurability + newDurability > maxDurability) {
-            newDurability = (short) (maxDurability - random.nextInt(maxDurability - 25));
-        }
-        updateDamage(this.itemToReforge, maxDurability - newDurability);
+        damageItemRandomly(this.itemToReforge);
     }
 
     /**
@@ -243,16 +228,4 @@ public class ReforgeSession extends Session implements Runnable {
         }
     }
 
-    /**
-     * Updates the damage done to an item
-     *
-     * @param item      <p>The item to update damage for</p>
-     * @param newDamage <p>The new damage done</p>
-     */
-    private void updateDamage(ItemStack item, int newDamage) {
-        if (!ItemHelper.updateDamage(item, newDamage)) {
-            BlacksmithPlugin.getInstance().getLogger().log(Level.WARNING, "Unable to change damage of " + item);
-        }
-    }
-
 }

src/main/java/net/knarcraft/blacksmith/trait/SalvageSession.java

@@ -6,19 +6,18 @@ import net.knarcraft.blacksmith.config.scrapper.ScrapperNPCSettings;
 import net.knarcraft.blacksmith.manager.EconomyManager;
 import net.knarcraft.blacksmith.util.ItemHelper;
 import net.knarcraft.blacksmith.util.SalvageHelper;
-import org.bukkit.entity.Damageable;
 import org.bukkit.entity.LivingEntity;
 import org.bukkit.entity.Player;
 import org.bukkit.inventory.ItemStack;
 import org.jetbrains.annotations.NotNull;
 
+import java.util.ArrayList;
 import java.util.Calendar;
 import java.util.List;
 import java.util.Objects;
 import java.util.Random;
 
 import static net.knarcraft.blacksmith.formatting.BlacksmithStringFormatter.sendNPCMessage;
-import static net.knarcraft.blacksmith.util.ItemHelper.updateDamage;
 
 /**
  * A representation of the session between a player and a scrapper
@@ -30,6 +29,7 @@ public class SalvageSession extends Session implements Runnable {
     private final ItemStack itemToSalvage;
     private final ScrapperNPCSettings config;
     private final List<ItemStack> salvage;
+    private final int itemsConsumed;
     private final int enchantmentLevels;
     protected final SalvageMethod salvageMethod;
     private static final Random random = new Random();
@@ -42,18 +42,20 @@ public class SalvageSession extends Session implements Runnable {
      * @param npc           <p>The scrapper NPC involved in the session</p>
      * @param config        <p>The config to use for the session</p>
      * @param salvageMethod <p>The salvage method performed in this session</p>
+     * @param itemsConsumed <p>The number of items actually consumed, in case a salvaging fails</p>
      */
     SalvageSession(@NotNull ScrapperTrait scrapperTrait, @NotNull Player player, @NotNull NPC npc,
                    @NotNull ScrapperNPCSettings config, @NotNull List<ItemStack> salvage,
-                   @NotNull SalvageMethod salvageMethod) {
+                   @NotNull SalvageMethod salvageMethod, int itemsConsumed) {
         super(player);
         this.scrapperTrait = scrapperTrait;
         this.npc = npc;
-        this.itemToSalvage = player.getInventory().getItemInMainHand();
+        this.itemToSalvage = player.getInventory().getItemInMainHand().clone();
         this.config = config;
         this.salvage = salvage;
         this.enchantmentLevels = SalvageHelper.getTotalEnchantmentLevels(this.itemToSalvage);
         this.salvageMethod = salvageMethod;
+        this.itemsConsumed = itemsConsumed;
     }
 
     @Override
@@ -88,7 +90,7 @@ public class SalvageSession extends Session implements Runnable {
      */
     @Override
     public void run() {
-        sendNPCMessage(this.npc, this.player, salvageItem() ? this.config.getSuccessMessage() : this.config.getFailMessage());
+        salvageItem();
 
         //Stop the reforged item from displaying in the scrapper's hand
         if (this.npc.getEntity() instanceof Player) {
@@ -108,17 +110,13 @@ public class SalvageSession extends Session implements Runnable {
 
     /**
      * Trues to salvage an item, and gives the return item
-     *
-     * @return <p>True if the salvage was successful. False otherwise.</p>
      */
-    private boolean salvageItem() {
+    private void salvageItem() {
         if (random.nextInt(100) < this.config.getFailChance()) {
             failSalvage();
-            return false;
         } else {
-            // Give the player the result of the salvage
             giveSalvage();
-            return true;
+            sendNPCMessage(this.npc, this.player, this.config.getSuccessMessage());
         }
     }
 
@@ -126,19 +124,22 @@ public class SalvageSession extends Session implements Runnable {
      * The method to run when a crapper fails salvaging an item
      */
     private void failSalvage() {
-        if (this.itemToSalvage.getItemMeta() instanceof Damageable) {
-            //Damage the item
-            short currentItemDurability = ItemHelper.getDurability(this.itemToSalvage);
-            short newDurability = (short) (currentItemDurability + (currentItemDurability * random.nextInt(8)));
-            short maxDurability = this.itemToSalvage.getType().getMaxDurability();
-            if (newDurability <= 0) {
-                newDurability = (short) (maxDurability / 3);
-            } else if (currentItemDurability + newDurability > maxDurability) {
-                newDurability = (short) (maxDurability - random.nextInt(maxDurability - 25));
-            }
-            updateDamage(this.itemToSalvage, maxDurability - newDurability);
+        // Make sure the given amount is the same amount taken for salvage to avoid duplication
+        this.itemToSalvage.setAmount(itemsConsumed);
+
+        if (ItemHelper.getMaxDurability(this.itemToSalvage) > 0) {
+            //Damage the item if possible
+            damageItemRandomly(this.itemToSalvage);
+            giveResultingItem(this.config.getMaxSalvageDelay() > 0, this.config.getDropItem(), this.npc,
+                    this.itemToSalvage);
+            sendNPCMessage(this.npc, this.player, this.config.getFailSalvageMessage());
         } else {
-            this.itemToSalvage.setAmount(Math.max(this.itemToSalvage.getAmount() - 1, 1));
+            // Give half the salvage
+            List<ItemStack> halfSalvage = SalvageHelper.pickRandomSalvage(this.salvage, new ArrayList<>(), 0.5);
+            for (ItemStack item : halfSalvage) {
+                giveResultingItem(this.config.getMaxSalvageDelay() > 0, this.config.getDropItem(), this.npc, item);
+            }
+            sendNPCMessage(this.npc, this.player, this.config.getFailExtendedSalvageMessage());
         }
     }
 
@@ -149,21 +150,8 @@ public class SalvageSession extends Session implements Runnable {
         // TODO: Find a better calculation than 1 enchantment level = 1 exp level
         // Gives the player back some of the EXP used on an item
         this.player.giveExpLevels(this.enchantmentLevels);
-        if (this.config.getDropItem() || !this.player.isOnline() || this.player.getInventory().firstEmpty() == -1) {
-            // If the player isn't online, or the player cannot fit the item, drop the item to prevent it from
-            // disappearing, even if drop item is disabled.
-            for (ItemStack item : this.salvage) {
-                this.player.getWorld().dropItemNaturally(this.npc.getEntity().getLocation(), item);
-            }
-        } else {
-            for (ItemStack item : this.salvage) {
-                if (ItemHelper.canFitItem(this.player.getInventory(), item)) {
-                    this.player.getInventory().addItem(item);
-                } else {
-                    // If the player cannot fit all the salvage, drop it on the ground
-                    this.player.getWorld().dropItemNaturally(this.player.getLocation(), item);
-                }
-            }
+        for (ItemStack item : this.salvage) {
+            giveResultingItem(this.config.getMaxSalvageDelay() > 0, this.config.getDropItem(), this.npc, item);
         }
     }
 

src/main/java/net/knarcraft/blacksmith/trait/ScrapperTrait.java

@@ -80,7 +80,7 @@ public class ScrapperTrait extends CustomTrait<ScrapperSetting> {
      * @param player <p>The player to start the session for</p>
      */
     public void startSession(@NotNull Player player) {
-        ItemStack itemInHand = player.getInventory().getItemInMainHand();
+        ItemStack itemInHand = player.getInventory().getItemInMainHand().clone();
         List<Material> salvageAbleItems = getSettings().getSalvageAbleItems();
         boolean extended = getSettings().extendedSalvageEnabled();
 
@@ -123,6 +123,7 @@ public class ScrapperTrait extends CustomTrait<ScrapperSetting> {
         if (salvageMethod == SalvageMethod.SALVAGE && !SalvageHelper.isSalvageable(player.getServer(), itemInHand)) {
             sendNPCMessage(this.npc, player, StringFormatter.replacePlaceholder(getSettings().getInvalidItemMessage(),
                     "{title}", getSettings().getScrapperTitle()));
+            return;
         }
 
         // Check if the item is enchanted, and whether this blacksmith can salvage it
@@ -142,7 +143,9 @@ public class ScrapperTrait extends CustomTrait<ScrapperSetting> {
         }
 
         //Start a new scrapper session for the player
-        startSession(player, salvage, salvageMethod);
+        currentSessionStartTime = System.currentTimeMillis();
+        int itemsConsumed = SalvageHelper.getRequiredAmountForSalvage(player.getServer(), itemInHand);
+        session = new SalvageSession(this, player, npc, getSettings(), salvage, salvageMethod, itemsConsumed);
         // Print the cost to the player
         printCostMessage(player, itemInHand, EconomyManager.formatSalvageCost(salvageMethod), salvageMethod);
     }
@@ -176,20 +179,6 @@ public class ScrapperTrait extends CustomTrait<ScrapperSetting> {
         }
     }
 
-    /**
-     * Starts a new scrapper session
-     *
-     * @param player        <p>The player to start the session for</p>
-     * @param salvage       <p>The salvage to return to the player</p>
-     * @param salvageMethod <p>The type of salvage performed</p>
-     */
-    private void startSession(@NotNull Player player, @NotNull List<ItemStack> salvage,
-                              @NotNull SalvageMethod salvageMethod) {
-        //Start a new scrapper session for the player
-        currentSessionStartTime = System.currentTimeMillis();
-        session = new SalvageSession(this, player, npc, getSettings(), salvage, salvageMethod);
-    }
-
     @Override
     protected boolean showExactTime() {
         return BlacksmithPlugin.getInstance().getGlobalScrapperSettings().showExactTime();

src/main/java/net/knarcraft/blacksmith/trait/Session.java

@@ -1,15 +1,22 @@
 package net.knarcraft.blacksmith.trait;
 
+import net.citizensnpcs.api.npc.NPC;
 import net.knarcraft.blacksmith.BlacksmithPlugin;
+import net.knarcraft.blacksmith.util.ItemHelper;
 import org.bukkit.entity.Player;
+import org.bukkit.inventory.ItemStack;
 import org.bukkit.scheduler.BukkitScheduler;
 import org.jetbrains.annotations.NotNull;
 
+import java.util.Random;
+import java.util.logging.Level;
+
 /**
  * A runnable session for performing a reforging/salvage task
  */
 public abstract class Session implements Runnable {
 
+    protected static final Random random = new Random();
     protected final Player player;
     protected long finishTime;
     protected int taskId;
@@ -85,6 +92,47 @@ public abstract class Session implements Runnable {
         taskId = scheduler.scheduleSyncDelayedTask(BlacksmithPlugin.getInstance(), this, actionDelay * 20L);
     }
 
+    /**
+     * Gives the resulting item to the player
+     *
+     * @param hasDelay     <p>Whether the session was delayed, or if the item was processed instantly</p>
+     * @param dropItem     <p>Whether the item should be dropped on the ground</p>
+     * @param npc          <p>The NPC holding the item</p>
+     * @param itemToReturn <p>The item to return to the player</p>
+     */
+    protected void giveResultingItem(boolean hasDelay, boolean dropItem, @NotNull NPC npc, @NotNull ItemStack itemToReturn) {
+        if (hasDelay) {
+            //If the player isn't online, or the player cannot fit the item, drop the item to prevent it from disappearing
+            if (dropItem || !this.player.isOnline() || !ItemHelper.canFitItem(this.player.getInventory(), itemToReturn)) {
+                this.player.getWorld().dropItemNaturally(npc.getEntity().getLocation(), itemToReturn);
+            } else {
+                this.player.getInventory().addItem(itemToReturn);
+            }
+        } else {
+            //It can be assumed as this happens instantly, that the player still has the item's previous slot selected
+            this.player.getInventory().setItemInMainHand(itemToReturn);
+        }
+    }
+
+    /**
+     * Randomly damages the given item
+     *
+     * @param item <p>The item to damage</p>
+     */
+    protected void damageItemRandomly(@NotNull ItemStack item) {
+        short currentItemDurability = ItemHelper.getDurability(item);
+        short newDurability = (short) (currentItemDurability + (currentItemDurability * random.nextInt(8)));
+        short maxDurability = item.getType().getMaxDurability();
+        if (newDurability <= 0) {
+            newDurability = (short) (maxDurability / 3);
+        } else if (currentItemDurability + newDurability > maxDurability) {
+            newDurability = (short) (maxDurability - random.nextInt(maxDurability - 25));
+        }
+        if (ItemHelper.updateDamage(item, maxDurability - newDurability)) {
+            BlacksmithPlugin.getInstance().getLogger().log(Level.WARNING, "Unable to update damage for " + item);
+        }
+    }
+
     /**
      * Gets the delay for this session's action to finish
      *

src/main/java/net/knarcraft/blacksmith/util/ItemHelper.java

@@ -83,16 +83,16 @@ public final class ItemHelper {
      *
      * @param item      <p>The item to update damage for</p>
      * @param newDamage <p>The new damage done</p>
-     * @return <p>True if the damage was updated. False if not damageable.</p>
+     * @return <p>False if the damage was updated. False if not damageable.</p>
      */
     public static boolean updateDamage(@NotNull ItemStack item, int newDamage) {
         ItemMeta meta = item.getItemMeta();
         if (!(meta instanceof Damageable damageable)) {
-            return false;
+            return true;
         }
         damageable.setDamage(newDamage);
         item.setItemMeta(meta);
-        return true;
+        return false;
     }
 
     /**

src/main/java/net/knarcraft/blacksmith/util/SalvageHelper.java

@@ -246,13 +246,32 @@ public final class SalvageHelper {
         }
 
         double percentageRemaining = (double) durability / maxDurability;
+        return pickRandomSalvage(recipeItems, trashSalvage, percentageRemaining);
+    }
 
-        int totalItems = totalItemAmount(recipeItems);
+    /**
+     * Picks random salvage from the given items
+     *
+     * @param itemsToChooseFrom   <p>The salvage to choose from</p>
+     * @param trashSalvage        <p>The salvage considered trash</p>
+     * @param percentageRemaining <p>The percentage remaining for the salvaged item (0-1)</p>
+     * @return <p>Random salvage</p>
+     */
+    @NotNull
+    public static List<ItemStack> pickRandomSalvage(@NotNull List<ItemStack> itemsToChooseFrom,
+                                                    @NotNull Collection<Material> trashSalvage,
+                                                    double percentageRemaining) {
+        // Avoid an infinite loop
+        if (percentageRemaining > 1 || percentageRemaining < 0) {
+            percentageRemaining = 1;
+        }
+
+        int totalItems = totalItemAmount(itemsToChooseFrom);
         //Get the amount of recipe items to be returned
         int itemsToReturn = (int) Math.floor(percentageRemaining * totalItems);
-        int bound = recipeItems.size();
+        int bound = itemsToChooseFrom.size();
 
-        List<ItemStack> goodItems = copyItems(recipeItems);
+        List<ItemStack> goodItems = copyItems(itemsToChooseFrom);
         goodItems.removeIf((item) -> trashSalvage.contains(item.getType()));
         int goodSalvage = totalItemAmount(goodItems);
 
@@ -260,7 +279,7 @@ public final class SalvageHelper {
         for (int i = 0; i < itemsToReturn; i++) {
             // Pick random item
             int itemIndex = SalvageHelper.random.nextInt(bound);
-            ItemStack itemStack = recipeItems.get(itemIndex);
+            ItemStack itemStack = itemsToChooseFrom.get(itemIndex);
 
             // The selected item is trash, so skip it
             if (trashSalvage.contains(itemStack.getType()) && i < goodSalvage) {
@@ -275,7 +294,6 @@ public final class SalvageHelper {
             }
 
             itemStack.setAmount(itemStack.getAmount() - 1);
-
             salvage.add(new ItemStack(itemStack.getType(), 1));
         }
         return salvage;
@@ -287,7 +305,7 @@ public final class SalvageHelper {
      * @param items <p>The items to get the sum of</p>
      * @return <p>The total number of items</p>
      */
-    private static int totalItemAmount(@NotNull List<ItemStack> items) {
+    public static int totalItemAmount(@NotNull List<ItemStack> items) {
         int sum = 0;
         for (ItemStack itemStack : items) {
             sum += itemStack.getAmount();

src/main/resources/config.yml

@@ -202,6 +202,9 @@ scrapper:
       # The message to display when the scrapper fails to salvage an item
       failSalvageMessage: "&cWhoops! The item broke! Maybe next time?"
       
+      # The message to display when the scrapper fails to salvage an item without durability
+      failExtendedSalvageMessage: "&cWhoops! I was unable to extract much, if any, salvage! Maybe next time?"
+      
       # The message to display when presenting a different item than the one just evaluated
       itemChangedMessage: "&cThat's not the item you wanted to salvage before!"