Fixed probability bug Fixes #5055 Fixes #5054

Changelog.txt

@@ -1,3 +1,10 @@
+Version 2.2.018
+    Fixed a probability bug where certain skills would max out in chance to succeed well before they were supposed to (such as Dodge)
+    (Codebase) Added more unit tests for Probability and RNG
+
+    NOTES:
+    This probability bug was a big oopsie and showed a gap in unit test coverage, I've added that coverage and a bug like this in theory shouldn't happen again.
+
 Version 2.2.017
     Fixed a bug with default Mace permissions (thanks SrBedrock)
     Fixed Blast Mining being almost completely broken

src/main/java/com/gmail/nossr50/commands/skills/AcrobaticsCommand.java

@@ -59,9 +59,6 @@ public class AcrobaticsCommand extends SkillCommand {
 
                 messages.add(getStatMessage(SubSkillType.ACROBATICS_ROLL, rollStrings[0])
                         + (isLucky ? LocaleLoader.getString("Perks.Lucky.Bonus", rollStrings[1]) : ""));
-
-                /*messages.add(getStatMessage(true, false, SubSkillType.ACROBATICS_ROLL, String.valueOf(graceChance))
-                        + (isLucky ? LocaleLoader.getString("Perks.Lucky.Bonus", String.valueOf(graceChanceLucky)) : ""));*/
             }
         }
 

src/main/java/com/gmail/nossr50/commands/skills/SkillCommand.java

@@ -30,6 +30,8 @@ import java.util.List;
 import java.util.Locale;
 
 public abstract class SkillCommand implements TabExecutor {
+    public static final String ABILITY_GENERIC_TEMPLATE_CUSTOM = "Ability.Generic.Template.Custom";
+    public static final String ABILITY_GENERIC_TEMPLATE = "Ability.Generic.Template";
     protected PrimarySkillType skill;
 
     protected DecimalFormat percent = new DecimalFormat("##0.00%");
@@ -163,8 +165,6 @@ public abstract class SkillCommand implements TabExecutor {
             /*
              * CHILD SKILLS
              */
-
-
             var parents = mcMMO.p.getSkillTools().getChildSkillParents(skill);
 
             //TODO: Add JSON here
@@ -186,28 +186,7 @@ public abstract class SkillCommand implements TabExecutor {
             player.sendMessage(LocaleLoader.getString("Commands.XPGain.Overhaul", LocaleLoader.getString("Commands.XPGain.Child")));
 
             player.sendMessage(LocaleLoader.getString("Effects.Child.Overhaul", skillValue, parentMessage.toString()));
-            //LEVEL
-            //player.sendMessage(LocaleLoader.getString("Effects.Child.Overhaul", skillValue, skillValue));
-
         }
-        /*
-        if (!SkillTools.isChildSkill(skill)) {
-            player.sendMessage(LocaleLoader.getString("Skills.Header", skillName));
-            player.sendMessage(LocaleLoader.getString("Commands.XPGain", LocaleLoader.getString("Commands.XPGain." + StringUtils.getCapitalized(skill.toString()))));
-            player.sendMessage(LocaleLoader.getString("Effects.Level", skillValue, mcMMOPlayer.getSkillXpLevel(skill), mcMMOPlayer.getXpToLevel(skill)));
-        } else {
-            player.sendMessage(LocaleLoader.getString("Skills.Header", skillName + " " + LocaleLoader.getString("Skills.Child")));
-            player.sendMessage(LocaleLoader.getString("Commands.XPGain", LocaleLoader.getString("Commands.XPGain.Child")));
-            player.sendMessage(LocaleLoader.getString("Effects.Child", skillValue));
-
-            player.sendMessage(LocaleLoader.getString("Skills.Header", LocaleLoader.getString("Skills.Parents")));
-            Set<PrimarySkillType> parents = FamilyTree.getParents(skill);
-
-            for (PrimarySkillType parent : parents) {
-                player.sendMessage(parent.getName() + " - " + LocaleLoader.getString("Effects.Level", mcMMOPlayer.getSkillLevel(parent), mcMMOPlayer.getSkillXpLevel(parent), mcMMOPlayer.getXpToLevel(parent)));
-            }
-        }
-        */
     }
 
     @Override
@@ -222,10 +201,6 @@ public abstract class SkillCommand implements TabExecutor {
         return Math.min((int) skillValue, maxLevel) / rankChangeLevel;
     }
 
-//    protected String[] getAbilityDisplayValues(SkillActivationType skillActivationType, Player player, SubSkillType subSkill) {
-//        return RandomChanceUtil.calculateAbilityDisplayValues(skillActivationType, player, subSkill);
-//    }
-
     protected String[] calculateLengthDisplayValues(Player player, float skillValue) {
         int maxLength = mcMMO.p.getSkillTools().getSuperAbilityMaxLength(mcMMO.p.getSkillTools().getSuperAbility(skill));
         int abilityLengthVar = mcMMO.p.getAdvancedConfig().getAbilityLength();
@@ -252,14 +227,19 @@ public abstract class SkillCommand implements TabExecutor {
         return getStatMessage(false, false, subSkillType, vars);
     }
 
-    protected String getStatMessage(boolean isExtra, boolean isCustom, SubSkillType subSkillType, String... vars) {
-        String templateKey = isCustom ? "Ability.Generic.Template.Custom" : "Ability.Generic.Template";
-        String statDescriptionKey = !isExtra ? subSkillType.getLocaleKeyStatDescription() : subSkillType.getLocaleKeyStatExtraDescription();
+    protected String getStatMessage(boolean isExtra, boolean isCustom,
+                                    @NotNull SubSkillType subSkillType, String... vars) {
+        final String templateKey = isCustom ? ABILITY_GENERIC_TEMPLATE_CUSTOM : ABILITY_GENERIC_TEMPLATE;
+        final String statDescriptionKey = !isExtra
+                ? subSkillType.getLocaleKeyStatDescription()
+                : subSkillType.getLocaleKeyStatExtraDescription();
 
-        if (isCustom)
+        if (isCustom) {
             return LocaleLoader.getString(templateKey, LocaleLoader.getString(statDescriptionKey, vars));
-        else {
-            String[] mergedList = NotificationManager.addItemToFirstPositionOfArray(LocaleLoader.getString(statDescriptionKey), vars);
+        } else {
+            final String[] mergedList
+                    = NotificationManager.addItemToFirstPositionOfArray(
+                            LocaleLoader.getString(statDescriptionKey), vars);
             return LocaleLoader.getString(templateKey, mergedList);
         }
     }
@@ -276,8 +256,6 @@ public abstract class SkillCommand implements TabExecutor {
 
     protected abstract void permissionsCheck(Player player);
 
-    //protected abstract List<String> effectsDisplay();
-
     protected abstract List<String> statsDisplay(Player player, float skillValue, boolean hasEndurance, boolean isLucky);
 
     protected abstract List<Component> getTextComponents(Player player);

src/main/java/com/gmail/nossr50/util/random/ProbabilityUtil.java

@@ -41,7 +41,7 @@ public class ProbabilityUtil {
     public static double chanceOfSuccessPercentage(@Nullable McMMOPlayer mmoPlayer,
                                                    @NotNull SubSkillType subSkillType,
                                                    boolean isLucky) {
-        Probability probability = getSubSkillProbability(subSkillType, mmoPlayer);
+        final Probability probability = getSubSkillProbability(subSkillType, mmoPlayer);
         //Probability values are on a 0-1 scale and need to be "transformed" into a 1-100 scale
         double percentageValue = probability.getValue(); //Doesn't need to be scaled
 
@@ -437,7 +437,7 @@ public class ProbabilityUtil {
             return Probability.ofPercent(ceiling);
         }
 
-        double odds = (skillLevel / maxBonusLevel) * 100D;
+        double odds = ((skillLevel / maxBonusLevel) * ceiling);
 
         // make sure the odds aren't lower or higher than the floor or ceiling
         return Probability.ofPercent(Math.min(Math.max(floor, odds), ceiling));

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

@@ -12,6 +12,8 @@ import org.junit.jupiter.params.provider.MethodSource;
 import java.util.logging.Logger;
 import java.util.stream.Stream;
 
+import static com.gmail.nossr50.datatypes.skills.PrimarySkillType.ACROBATICS;
+import static com.gmail.nossr50.datatypes.skills.PrimarySkillType.MINING;
 import static com.gmail.nossr50.datatypes.skills.SubSkillType.*;
 import static com.gmail.nossr50.util.random.ProbabilityTestUtils.assertProbabilityExpectations;
 import static com.gmail.nossr50.util.random.ProbabilityUtil.calculateCurrentSkillProbability;
@@ -68,9 +70,106 @@ class ProbabilityUtilTest extends MMOTestEnvironment {
         assertProbabilityExpectations(20, probability);
     }
 
+    private static Stream<Arguments> provideSkillProbabilityTestData() {
+        return Stream.of(
+                // skillLevel, floor, ceiling, maxBonusLevel, expectedValue
+
+                // 20% chance at skill level 1000
+                Arguments.of(1000, 0, 20, 1000, 0.2),
+                // 10% chance at skill level 500
+                Arguments.of(500, 0, 20, 1000, 0.1),
+                // 5% chance at skill level 250
+                Arguments.of(250, 0, 20, 1000, 0.05),
+                // 0% chance at skill level 0
+                Arguments.of(0, 0, 20, 1000, 0.0),
+                // 0% chance at skill level 1000
+                Arguments.of(1000, 0, 0, 1000, 0.0),
+                // 1% chance at skill level 1000
+                Arguments.of(1000, 0, 1, 1000, 0.01)
+        );
+    }
+
+    @ParameterizedTest
+    @MethodSource("provideSkillProbabilityTestData")
+    void testCalculateCurrentSkillProbability(double skillLevel, double floor, double ceiling, double maxBonusLevel,
+                                              double expectedValue) {
+        // When
+        final Probability probability = calculateCurrentSkillProbability(skillLevel, floor, ceiling, maxBonusLevel);
+
+        // Then
+        assertEquals(expectedValue, probability.getValue());
+    }
+
     @Test
-    public void calculateCurrentSkillProbabilityShouldBeTwenty() {
-        final Probability probability = calculateCurrentSkillProbability(1000, 0, 20, 1000);
-        assertEquals(0.2D, probability.getValue());
+    public void getRNGDisplayValuesShouldReturn10PercentForDodge() {
+        // Given
+        when(advancedConfig.getMaximumProbability(ACROBATICS_DODGE)).thenReturn(20D);
+        when(advancedConfig.getMaxBonusLevel(ACROBATICS_DODGE)).thenReturn(1000);
+        mmoPlayer.modifySkill(ACROBATICS, 500);
+
+        // When & Then
+        final String[] rngDisplayValues = ProbabilityUtil.getRNGDisplayValues(mmoPlayer, ACROBATICS_DODGE);
+        assertEquals("10.00%", rngDisplayValues[0]);
+    }
+
+    @Test
+    public void getRNGDisplayValuesShouldReturn20PercentForDodge() {
+        // Given
+        when(advancedConfig.getMaximumProbability(ACROBATICS_DODGE)).thenReturn(20D);
+        when(advancedConfig.getMaxBonusLevel(ACROBATICS_DODGE)).thenReturn(1000);
+        mmoPlayer.modifySkill(ACROBATICS, 1000);
+
+        // When & then
+        final String[] rngDisplayValues = ProbabilityUtil.getRNGDisplayValues(mmoPlayer, ACROBATICS_DODGE);
+        assertEquals("20.00%", rngDisplayValues[0]);
+    }
+
+    @Test
+    public void getRNGDisplayValuesShouldReturn0PercentForDodge() {
+        // Given
+        when(advancedConfig.getMaximumProbability(ACROBATICS_DODGE)).thenReturn(20D);
+        when(advancedConfig.getMaxBonusLevel(ACROBATICS_DODGE)).thenReturn(1000);
+        mmoPlayer.modifySkill(ACROBATICS, 0);
+
+        // When & then
+        final String[] rngDisplayValues = ProbabilityUtil.getRNGDisplayValues(mmoPlayer, ACROBATICS_DODGE);
+        assertEquals("0.00%", rngDisplayValues[0]);
+    }
+
+    @Test
+    public void getRNGDisplayValuesShouldReturn10PercentForDoubleDrops() {
+        // Given
+        when(advancedConfig.getMaximumProbability(MINING_DOUBLE_DROPS)).thenReturn(100D);
+        when(advancedConfig.getMaxBonusLevel(MINING_DOUBLE_DROPS)).thenReturn(1000);
+        mmoPlayer.modifySkill(MINING, 100);
+
+        // When & Then
+        final String[] rngDisplayValues = ProbabilityUtil.getRNGDisplayValues(mmoPlayer, MINING_DOUBLE_DROPS);
+        assertEquals("10.00%", rngDisplayValues[0]);
     }
+
+    @Test
+    public void getRNGDisplayValuesShouldReturn50PercentForDoubleDrops() {
+        // Given
+        when(advancedConfig.getMaximumProbability(MINING_DOUBLE_DROPS)).thenReturn(100D);
+        when(advancedConfig.getMaxBonusLevel(MINING_DOUBLE_DROPS)).thenReturn(1000);
+        mmoPlayer.modifySkill(MINING, 500);
+
+        // When & Then
+        final String[] rngDisplayValues = ProbabilityUtil.getRNGDisplayValues(mmoPlayer, MINING_DOUBLE_DROPS);
+        assertEquals("50.00%", rngDisplayValues[0]);
+    }
+
+    @Test
+    public void getRNGDisplayValuesShouldReturn100PercentForDoubleDrops() {
+        // Given
+        when(advancedConfig.getMaximumProbability(MINING_DOUBLE_DROPS)).thenReturn(100D);
+        when(advancedConfig.getMaxBonusLevel(MINING_DOUBLE_DROPS)).thenReturn(1000);
+        mmoPlayer.modifySkill(MINING, 1000);
+
+        // When & Then
+        final String[] rngDisplayValues = ProbabilityUtil.getRNGDisplayValues(mmoPlayer, MINING_DOUBLE_DROPS);
+        assertEquals("100.00%", rngDisplayValues[0]);
+    }
+
 }