Fix potions not brewing as the correct type

Changelog.txt

@@ -1,3 +1,9 @@
+Version 2.2.008
+    Fixed alchemy potions not upgrading correctly (See notes)
+
+    NOTES:
+    Alchemy potions will now be brewed as type "Mundane" behind the scenes, this used to be Uncraftable/Water. This led to some issues. So I've changed it to be Mundane.
+
 Version 2.2.007
     Compatibility with the 1.20.5 / 1.20.6 MC Update
     Fixed bug where Alchemy was not brewing certain potions (haste, etc)

pom.xml

@@ -2,7 +2,7 @@
     <modelVersion>4.0.0</modelVersion>
     <groupId>com.gmail.nossr50.mcMMO</groupId>
     <artifactId>mcMMO</artifactId>
-    <version>2.2.007</version>
+    <version>2.2.008-SNAPSHOT</version>
     <name>mcMMO</name>
     <url>https://github.com/mcMMO-Dev/mcMMO</url>
     <scm>

src/main/java/com/gmail/nossr50/config/skills/alchemy/PotionConfig.java

@@ -127,9 +127,6 @@ public class PotionConfig extends LegacyConfigLoader {
     private AlchemyPotion loadPotion(ConfigurationSection potion_section) {
         try {
             final String key = potion_section.getName();
-            final String displayName = potion_section.getString("Name") != null
-                    ? LocaleLoader.addColors(potion_section.getString("Name"))
-                    : convertKeyToName(key);
 
             final ConfigurationSection potionData = potion_section.getConfigurationSection("PotionData");
             boolean extended = false;
@@ -159,14 +156,11 @@ public class PotionConfig extends LegacyConfigLoader {
             final PotionMeta potionMeta = (PotionMeta) itemStack.getItemMeta();
 
             if (potionMeta == null) {
-                mcMMO.p.getLogger().severe("PotionConfig: Failed to get PotionMeta for " + displayName + ", from configuration section:" +
+                mcMMO.p.getLogger().severe("PotionConfig: Failed to get PotionMeta for " + key + ", from configuration section:" +
                         " " + potion_section);
                 return null;
             }
 
-            // Set the name of the potion
-            potionMeta.setDisplayName(displayName);
-
             // extended and upgraded seem to be mutually exclusive
             if (extended && upgraded) {
                 mcMMO.p.getLogger().warning("Potion " + key + " has both Extended and Upgraded set to true," +
@@ -176,7 +170,7 @@ public class PotionConfig extends LegacyConfigLoader {
 
             String potionTypeStr = potionData.getString("PotionType", null);
             if (potionTypeStr == null) {
-                mcMMO.p.getLogger().severe("PotionConfig: Missing PotionType for " + displayName + ", from configuration section:" +
+                mcMMO.p.getLogger().severe("PotionConfig: Missing PotionType for " + key + ", from configuration section:" +
                         " " + potion_section);
                 return null;
             }
@@ -190,7 +184,7 @@ public class PotionConfig extends LegacyConfigLoader {
 
             if (potionType == null) {
                 mcMMO.p.getLogger().severe("PotionConfig: Failed to parse potion type for: " + potionTypeStr
-                        + ", upgraded: " + upgraded + ", extended: " + extended + " for potion " + displayName
+                        + ", upgraded: " + upgraded + ", extended: " + extended + " for potion " + key
                         + ", from configuration section: " + potion_section);
                 return null;
             }
@@ -225,7 +219,7 @@ public class PotionConfig extends LegacyConfigLoader {
                     if (type != null) {
                         potionMeta.addCustomEffect(new PotionEffect(type, duration, amplifier), true);
                     } else {
-                        mcMMO.p.getLogger().severe("PotionConfig: Failed to parse effect for potion " + displayName + ": " + effect);
+                        mcMMO.p.getLogger().severe("PotionConfig: Failed to parse effect for potion " + key + ": " + effect);
                     }
                 }
             }
@@ -238,17 +232,21 @@ public class PotionConfig extends LegacyConfigLoader {
             }
             potionMeta.setColor(color);
 
-            Map<ItemStack, String> children = new HashMap<>();
+            final Map<ItemStack, String> children = new HashMap<>();
             if (potion_section.contains("Children")) {
                 for (String child : potion_section.getConfigurationSection("Children").getKeys(false)) {
                     ItemStack ingredient = loadIngredient(child);
                     if (ingredient != null) {
                         children.put(ingredient, potion_section.getConfigurationSection("Children").getString(child));
                     } else {
-                        mcMMO.p.getLogger().severe("PotionConfig: Failed to parse child for potion " + displayName + ": " + child);
+                        mcMMO.p.getLogger().severe("PotionConfig: Failed to parse child for potion " + key + ": " + child);
                     }
                 }
             }
+
+            // Set the name of the potion
+            setPotionDisplayName(potion_section, potionMeta);
+
             // TODO: Might not need to .setItemMeta
             itemStack.setItemMeta(potionMeta);
             return new AlchemyPotion(itemStack, children);
@@ -258,6 +256,23 @@ public class PotionConfig extends LegacyConfigLoader {
         }
     }
 
+    private void setPotionDisplayName(ConfigurationSection section, PotionMeta potionMeta) {
+        String configuredName = section.getString("Name", null);
+        if (configuredName != null) {
+            potionMeta.setDisplayName(configuredName);
+            return;
+        }
+
+        // Potion is water, but has effects
+        if (isPotionTypeWater(potionMeta)
+                && (PotionUtil.hasBasePotionEffects(potionMeta) || !potionMeta.getCustomEffects().isEmpty())) {
+            // If we don't set a name for these potions, they will simply be called "Water Potion"
+            final String name = section.getName().toUpperCase().replace("_", " ");
+            potionMeta.setDisplayName(name);
+            System.out.println("DEBUG: Renaming potion to " + name);
+        }
+    }
+
     /**
      * Parse a string representation of an ingredient.
      * Format: '&lt;MATERIAL&gt;[:data]'

src/main/java/com/gmail/nossr50/datatypes/skills/alchemy/AlchemyPotion.java

@@ -53,12 +53,11 @@ public class AlchemyPotion {
         }
 
         final PotionMeta otherPotionMeta = (PotionMeta) otherPotion.getItemMeta();
-
-        // all custom effects must be present
-        for (var effect : getAlchemyPotionMeta().getCustomEffects()) {
-            if (!otherPotionMeta.hasCustomEffect(effect.getType())) {
-                return false;
-            }
+        // compare custom effects on both potions, this has to be done in two traversals
+        // comparing thisPotionMeta -> otherPotionMeta and otherPotionMeta -> thisPotionMeta
+        if (hasDifferingCustomEffects(getAlchemyPotionMeta(), otherPotionMeta)
+                || hasDifferingCustomEffects(otherPotionMeta, getAlchemyPotionMeta())) {
+            return false;
         }
 
         if (!samePotionType(getAlchemyPotionMeta(), otherPotionMeta)) {
@@ -78,6 +77,25 @@ public class AlchemyPotion {
         return true;
     }
 
+    private boolean hasDifferingCustomEffects(PotionMeta potionMeta, PotionMeta otherPotionMeta) {
+        for (int i = 0; i < potionMeta.getCustomEffects().size(); i++) {
+            var effect = potionMeta.getCustomEffects().get(i);
+
+            // One has an effect the other does not, they are not the same potion
+            if (!otherPotionMeta.hasCustomEffect(effect.getType())) {
+                return true;
+            }
+
+            var otherEffect = otherPotionMeta.getCustomEffects().get(i);
+            // Amplifier or duration are not equal, they are not the same potion
+            if (effect.getAmplifier() != otherEffect.getAmplifier()
+                    || effect.getDuration() != otherEffect.getDuration()) {
+                return true;
+            }
+        }
+        return false;
+    }
+
     public PotionMeta getAlchemyPotionMeta() {
         return (PotionMeta) potionItemstack.getItemMeta();
     }

src/main/java/com/gmail/nossr50/datatypes/skills/alchemy/PotionStage.java

@@ -34,12 +34,13 @@ public enum PotionStage {
     }
 
     public static PotionStage getPotionStage(AlchemyPotion input, AlchemyPotion output) {
-        PotionStage potionStage = getPotionStage(output);
-        if (!isWaterBottle(input) && getPotionStage(input) == potionStage) {
-            potionStage = PotionStage.FIVE;
+        PotionStage outputPotionStage = getPotionStage(output);
+        PotionStage inputPotionStage = getPotionStage(input);
+        if (!isWaterBottle(input) && inputPotionStage == outputPotionStage) {
+            outputPotionStage = PotionStage.FIVE;
         }
 
-        return potionStage;
+        return outputPotionStage;
     }
 
     private static boolean isWaterBottle(AlchemyPotion alchemyPotion) {

src/main/java/com/gmail/nossr50/listeners/InventoryListener.java

@@ -5,6 +5,7 @@ import com.gmail.nossr50.datatypes.player.McMMOPlayer;
 import com.gmail.nossr50.datatypes.skills.PrimarySkillType;
 import com.gmail.nossr50.datatypes.skills.SubSkillType;
 import com.gmail.nossr50.events.fake.FakeBrewEvent;
+import com.gmail.nossr50.events.fake.FakeEvent;
 import com.gmail.nossr50.mcMMO;
 import com.gmail.nossr50.runnables.player.PlayerUpdateInventoryTask;
 import com.gmail.nossr50.skills.alchemy.Alchemy;
@@ -28,6 +29,7 @@ import org.bukkit.entity.Player;
 import org.bukkit.event.EventHandler;
 import org.bukkit.event.EventPriority;
 import org.bukkit.event.Listener;
+import org.bukkit.event.block.BrewingStartEvent;
 import org.bukkit.event.inventory.*;
 import org.bukkit.inventory.*;
 
@@ -343,6 +345,7 @@ public class InventoryListener implements Listener {
 
         if (event instanceof FakeBrewEvent)
             return;
+
         Location location = event.getBlock().getLocation();
         if (Alchemy.brewingStandMap.containsKey(location)) {
             Alchemy.brewingStandMap.get(location).finishImmediately();
@@ -350,6 +353,16 @@ public class InventoryListener implements Listener {
         }
     }
 
+//    @EventHandler(priority = EventPriority.LOWEST, ignoreCancelled = true)
+//    public void onBrewStart(BrewingStartEvent event) {
+//        /* WORLD BLACKLIST CHECK */
+//        if(WorldBlacklist.isWorldBlacklisted(event.getBlock().getWorld()))
+//            return;
+//
+//        if (event instanceof FakeEvent)
+//            return;
+//    }
+
     @EventHandler(priority = EventPriority.NORMAL, ignoreCancelled = true)
     public void onInventoryMoveItemEvent(InventoryMoveItemEvent event) {
         /* WORLD BLACKLIST CHECK */

src/main/java/com/gmail/nossr50/runnables/skills/AlchemyBrewCheckTask.java

@@ -29,7 +29,10 @@ public class AlchemyBrewCheckTask extends CancellableRunnable {
         boolean validBrew = brewingStand.getFuelLevel() > 0 && AlchemyPotionBrewer.isValidBrew(player, newInventory);
 
         if (Alchemy.brewingStandMap.containsKey(location)) {
-            if (oldInventory[Alchemy.INGREDIENT_SLOT] == null || newInventory[Alchemy.INGREDIENT_SLOT] == null || !oldInventory[Alchemy.INGREDIENT_SLOT].isSimilar(newInventory[Alchemy.INGREDIENT_SLOT]) || !validBrew) {
+            if (oldInventory[Alchemy.INGREDIENT_SLOT] == null
+                    || newInventory[Alchemy.INGREDIENT_SLOT] == null
+                    || !oldInventory[Alchemy.INGREDIENT_SLOT].isSimilar(newInventory[Alchemy.INGREDIENT_SLOT])
+                    || !validBrew) {
                 Alchemy.brewingStandMap.get(location).cancelBrew();
             }
         } else if (validBrew) {

src/main/java/com/gmail/nossr50/skills/alchemy/AlchemyManager.java

@@ -52,6 +52,11 @@ public class AlchemyManager extends SkillManager {
         return Math.min(Alchemy.catalysisMaxSpeed, Alchemy.catalysisMinSpeed + (Alchemy.catalysisMaxSpeed - Alchemy.catalysisMinSpeed) * (skillLevel - RankUtils.getUnlockLevel(SubSkillType.ALCHEMY_CATALYSIS)) / (Alchemy.catalysisMaxBonusLevel - RankUtils.getUnlockLevel(SubSkillType.ALCHEMY_CATALYSIS))) * (isLucky ? LUCKY_MODIFIER : 1.0);
     }
 
+    /**
+     * Handle the XP gain for a successful potion brew.
+     * @param potionStage The potion stage, this is used to determine the XP gain.
+     * @param amount The amount of potions brewed.
+     */
     public void handlePotionBrewSuccesses(PotionStage potionStage, int amount) {
         applyXpGain((float) (ExperienceConfig.getInstance().getPotionXP(potionStage) * amount), XPGainReason.PVE, XPGainSource.PASSIVE);
     }

src/main/java/com/gmail/nossr50/skills/alchemy/AlchemyPotionBrewer.java

@@ -38,7 +38,8 @@ public final class AlchemyPotionBrewer {
                 continue;
             }
 
-            if (getChildPotion(mcMMO.p.getPotionConfig().getPotion(contents[i]), contents[Alchemy.INGREDIENT_SLOT]) != null) {
+            final AlchemyPotion potion = mcMMO.p.getPotionConfig().getPotion(contents[i]);
+            if (getChildPotion(potion, contents[Alchemy.INGREDIENT_SLOT]) != null) {
                 return true;
             }
         }

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

@@ -1,7 +1,6 @@
 package com.gmail.nossr50.util;
 
 import com.gmail.nossr50.mcMMO;
-import org.bukkit.Bukkit;
 import org.bukkit.NamespacedKey;
 import org.bukkit.inventory.meta.PotionMeta;
 import org.bukkit.potion.PotionEffectType;
@@ -33,15 +32,16 @@ public class PotionUtil {
 
     public static final String STRONG = "STRONG";
     public static final String LONG = "LONG";
-    public static final String LEGACY_WATER_POTION_TYPE = "WATER";
+    public static final String WATER_POTION_TYPE_STR = "WATER";
 
     private static final PotionCompatibilityType COMPATIBILITY_MODE;
 
     static {
-        // We used to use uncraftable as the potion type
-        // this type isn't available anymore, so water will do
         potionDataClass = getPotionDataClass();
-        legacyPotionTypes.put("UNCRAFTABLE", "WATER");
+        // Uncraftable doesn't exist in modern versions
+        // It served as a potion that didn't craft into anything else so it didn't conflict with vanilla systems
+        // Instead we will use Mundane, which doesn't make anything in vanilla systems
+        legacyPotionTypes.put("UNCRAFTABLE", "MUNDANE");
         legacyPotionTypes.put("JUMP", "LEAPING");
         legacyPotionTypes.put("SPEED", "SWIFTNESS");
         legacyPotionTypes.put("INSTANT_HEAL", "HEALING");
@@ -314,6 +314,12 @@ public class PotionUtil {
         return (NamespacedKey) methodPotionTypeGetKey.invoke(potionType);
     }
 
+    public static boolean isPotionJustWater(PotionMeta potionMeta) {
+        return isPotionTypeWater(potionMeta)
+                && !hasBasePotionEffects(potionMeta)
+                && potionMeta.getCustomEffects().isEmpty();
+    }
+
     public static boolean isPotionTypeWater(@NotNull PotionMeta potionMeta) {
         if (COMPATIBILITY_MODE == PotionCompatibilityType.PRE_1_20_5) {
             return isPotionTypeWaterLegacy(potionMeta);
@@ -322,12 +328,44 @@ public class PotionUtil {
         }
     }
 
+    public static boolean isPotionType(@NotNull PotionMeta potionMeta, String potionType) {
+        if (COMPATIBILITY_MODE == PotionCompatibilityType.PRE_1_20_5) {
+            return isPotionTypeLegacy(potionMeta, potionType);
+        } else {
+            return isPotionTypeModern(potionMeta, potionType);
+        }
+    }
+
+    public static boolean isPotionTypeWithoutEffects(@NotNull PotionMeta potionMeta, String potionType) {
+        return isPotionType(potionMeta, potionType)
+                && !hasBasePotionEffects(potionMeta)
+                && potionMeta.getCustomEffects().isEmpty();
+    }
+
+    private static boolean isPotionTypeModern(@NotNull PotionMeta potionMeta, String potionType) {
+        try {
+            return getModernPotionTypeKey(potionMeta).getKey().equalsIgnoreCase(potionType);
+        } catch (IllegalAccessException | InvocationTargetException ex) {
+            throw new RuntimeException(ex);
+        }
+    }
+
+    private static boolean isPotionTypeLegacy(@NotNull PotionMeta potionMeta, String potionType) {
+        try {
+            Object potionData = methodPotionMetaGetBasePotionData.invoke(potionMeta);
+            PotionType potionTypeObj = (PotionType) methodPotionDataGetType.invoke(potionData);
+            return potionTypeObj.name().equalsIgnoreCase(potionType);
+        } catch (IllegalAccessException | InvocationTargetException ex) {
+            throw new RuntimeException(ex);
+        }
+    }
+
     private static boolean isPotionTypeWaterLegacy(@NotNull PotionMeta potionMeta) {
         try {
             Object potionData = methodPotionMetaGetBasePotionData.invoke(potionMeta);
             PotionType potionType = (PotionType) methodPotionDataGetType.invoke(potionData);
-            return potionType.name().equalsIgnoreCase(LEGACY_WATER_POTION_TYPE)
-                    || PotionType.valueOf(LEGACY_WATER_POTION_TYPE) == potionType;
+            return potionType.name().equalsIgnoreCase(WATER_POTION_TYPE_STR)
+                    || PotionType.valueOf(WATER_POTION_TYPE_STR) == potionType;
         } catch (InvocationTargetException | IllegalAccessException ex) {
             throw new RuntimeException(ex);
         }
@@ -335,7 +373,7 @@ public class PotionUtil {
 
     private static boolean isPotionTypeWaterModern(@NotNull PotionMeta potionMeta) {
         try {
-            return getModernPotionTypeKey(potionMeta).getKey().equalsIgnoreCase(LEGACY_WATER_POTION_TYPE);
+            return getModernPotionTypeKey(potionMeta).getKey().equalsIgnoreCase(WATER_POTION_TYPE_STR);
         } catch (IllegalAccessException | InvocationTargetException ex) {
             throw new RuntimeException(ex);
         }

src/test/java/com/gmail/nossr50/util/PotionUtilTest.java

@@ -30,6 +30,6 @@ class PotionUtilTest {
     void testConvertLegacyNames() {
         final String potionTypeStr = "UNCRAFTABLE";
         final String converted = convertLegacyNames(potionTypeStr);
-        assertEquals("WATER", converted);
+        assertEquals("MUNDANE", converted);
     }
 }

src/test/java/com/gmail/nossr50/util/random/ProbabilityTestUtils.java

@@ -16,7 +16,7 @@ public class ProbabilityTestUtils {
         System.out.println("Wins: " + winCount);
         System.out.println("Fails: " + (iterations - winCount));
         System.out.println("Percentage succeeded: " + successPercent + ", Expected: " + expectedWinPercent);
-        assertEquals(expectedWinPercent, successPercent, 0.025D);
+        assertEquals(expectedWinPercent, successPercent, 0.035D);
         System.out.println("Variance is within tolerance levels!");
     }
 }