fix probability being unbounded

Changelog.txt

@@ -1,3 +1,10 @@
+Version 2.2.005
+    Fixed a bug where certain skills such as Dodge/Arrow Deflect had no skill cap and would continue improving forever
+    Reduced messages on startup for SQL DB
+    (API) Constructor for ProbabilityImpl now takes a raw value between 0 and 1 instead of an inflated percentage
+    (API) Added some convenience methods to Probability, and ProbabilityUtil classes
+    (Codebase) Added more unit tests revolving around Probability/RNG
+
 Version 2.2.004
     Fixed bug where values from Experience_Formula.Skill_Multiplier were not functioning
 

src/main/java/com/gmail/nossr50/database/SQLDatabaseManager.java

@@ -1037,13 +1037,13 @@ public final class SQLDatabaseManager implements DatabaseManager {
         try (Connection connection = getConnection(PoolIdentifier.MISC)) {
             if (!columnExists(connection, mcMMO.p.getGeneralConfig().getMySQLDatabaseName(), tablePrefix+tableName, columnName)) {
                 try (Statement createStatement = connection.createStatement()) {
-                    logger.info("[SQLDB Check] Adding column '" + columnName + "' to table '" + tablePrefix + tableName + "'...");
+                    // logger.info("[SQLDB Check] Adding column '" + columnName + "' to table '" + tablePrefix + tableName + "'...");
                     String startingLevel = "'" + mcMMO.p.getAdvancedConfig().getStartingLevel() + "'";
                     createStatement.executeUpdate("ALTER TABLE `" + tablePrefix + tableName + "` "
                             + "ADD COLUMN `" + columnName + "` int(" + columnSize + ") unsigned NOT NULL DEFAULT " + startingLevel);
                 }
             } else {
-                logger.info("[SQLDB Check] Column '" + columnName + "' already exists in table '" + tablePrefix + tableName + "', looks good!");
+                // logger.info("[SQLDB Check] Column '" + columnName + "' already exists in table '" + tablePrefix + tableName + "', looks good!");
             }
         } catch (SQLException e) {
             e.printStackTrace(); // Consider more robust logging
@@ -1052,7 +1052,7 @@ public final class SQLDatabaseManager implements DatabaseManager {
     }
 
     private boolean columnExists(Connection connection, String database, String tableName, String columnName) throws SQLException {
-        logger.info("[SQLDB Check] Checking if column '" + columnName + "' exists in table '" + tableName + "'");
+        // logger.info("[SQLDB Check] Checking if column '" + columnName + "' exists in table '" + tableName + "'");
         try (Statement createStatement = connection.createStatement()) {
             String sql = "SELECT `COLUMN_NAME`\n" +
                     "FROM `INFORMATION_SCHEMA`.`COLUMNS`\n" +

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

@@ -1,10 +1,21 @@
 package com.gmail.nossr50.util.random;
 
+import com.gmail.nossr50.api.exceptions.ValueOutOfBoundsException;
 import org.jetbrains.annotations.NotNull;
 
 import java.util.concurrent.ThreadLocalRandom;
 
 public interface Probability {
+    /**
+     * A Probability that always fails.
+     */
+    Probability ALWAYS_FAILS = () -> 0;
+
+    /**
+     * A Probability that always succeeds.
+     */
+    Probability ALWAYS_SUCCEEDS = () -> 1;
+
     /**
      * The value of this Probability
      * Should return a result between 0 and 1 (inclusive)
@@ -17,19 +28,40 @@ public interface Probability {
     double getValue();
 
     /**
-     * Create a new Probability with the given value
+     * Create a new Probability of a percentage.
-     * A value of 100 would represent 100% chance of success
+     * This method takes a percentage and creates a Probability of equivalent odds.
-     * A value of 50 would represent 50% chance of success
+     *
-     * A value of 0 would represent 0% chance of success
+     * A value of 100 would represent 100% chance of success,
-     * A value of 1 would represent 1% chance of success
+     * A value of 50 would represent 50% chance of success,
-     * A value of 0.5 would represent 0.5% chance of success
+     * A value of 0 would represent 0% chance of success,
-     * A value of 0.01 would represent 0.01% chance of success
+     * A value of 1 would represent 1% chance of success,
+     * A value of 0.5 would represent 0.5% chance of success,
+     * A value of 0.01 would represent 0.01% chance of success.
      *
      * @param percentage the value of the probability
      * @return a new Probability with the given value
      */
     static @NotNull Probability ofPercent(double percentage) {
-        return new ProbabilityImpl(percentage);
+        if (percentage < 0) {
+            throw new ValueOutOfBoundsException("Value should never be negative for Probability! This suggests a coding mistake, contact the devs!");
+        }
+
+        // Convert to a 0-1 floating point representation
+        double probabilityValue = percentage / 100.0D;
+        return new ProbabilityImpl(probabilityValue);
+    }
+
+    /**
+     * Create a new Probability of a value.
+     * This method takes a value between 0 and 1 and creates a Probability of equivalent odds.
+     * A value of 1 or greater represents something that will always succeed.
+     * A value of around 0.5 represents something that succeeds around half the time.
+     * A value of 0 represents something that will always fail.
+     * @param value the value of the probability
+     * @return a new Probability with the given value
+     */
+    static @NotNull Probability ofValue(double value) {
+        return new ProbabilityImpl(value);
     }
 
     /**

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

@@ -8,31 +8,22 @@ public class ProbabilityImpl implements Probability {
     private final double probabilityValue;
 
     /**
-     * Create a probability with a static value
+     * Create a probability from a static value.
+     * A value of 0 represents a 0% chance of success,
+     * A value of 1 represents a 100% chance of success.
+     * A value of 0.5 represents a 50% chance of success.
+     * A value of 0.01 represents a 1% chance of success.
+     * And so on.
      *
-     * @param percentage the percentage value of the probability
+     * @param value the value of the probability between 0 and 100
      */
-    ProbabilityImpl(double percentage) throws ValueOutOfBoundsException {
+    public ProbabilityImpl(double value) throws ValueOutOfBoundsException {
-        if (percentage < 0) {
+        if (value < 0) {
-            throw new ValueOutOfBoundsException("Value should never be negative for Probability! This suggests a coding mistake, contact the devs!");
+            throw new ValueOutOfBoundsException("Value should never be negative for Probability!" +
+                    " This suggests a coding mistake, contact the devs!");
         }
 
-        // Convert to a 0-1 floating point representation
+        probabilityValue = value;
-        probabilityValue = percentage / 100.0D;
-    }
-
-    ProbabilityImpl(double xPos, double xCeiling, double probabilityCeiling) throws ValueOutOfBoundsException {
-        if(probabilityCeiling > 100) {
-            throw new ValueOutOfBoundsException("Probability Ceiling should never be above 100!");
-        } else if (probabilityCeiling < 0) {
-            throw new ValueOutOfBoundsException("Probability Ceiling should never be below 0!");
-        }
-
-        //Get the percent success, this will be from 0-100
-        double probabilityPercent = (probabilityCeiling * (xPos / xCeiling));
-
-        //Convert to a 0-1 floating point representation
-        this.probabilityValue = probabilityPercent / 100.0D;
     }
 
     @Override

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

@@ -79,24 +79,24 @@ public class ProbabilityUtil {
         switch (getProbabilityType(subSkillType)) {
             case DYNAMIC_CONFIGURABLE:
                 double probabilityCeiling;
-                double xCeiling;
+                double skillLevel;
-                double xPos;
+                double maxBonusLevel; // If a skill level is equal to the cap, it has the full probability
 
                 if (player != null) {
                     McMMOPlayer mmoPlayer = UserManager.getPlayer(player);
                     if (mmoPlayer == null) {
                         return Probability.ofPercent(0);
                     }
-                    xPos = mmoPlayer.getSkillLevel(subSkillType.getParentSkill());
+                    skillLevel = mmoPlayer.getSkillLevel(subSkillType.getParentSkill());
                 } else {
-                    xPos = 0;
+                    skillLevel = 0;
                 }
 
                 //Probability ceiling is configurable in this type
                 probabilityCeiling = mcMMO.p.getAdvancedConfig().getMaximumProbability(subSkillType);
                 //The xCeiling is configurable in this type
-                xCeiling = mcMMO.p.getAdvancedConfig().getMaxBonusLevel(subSkillType);
+                maxBonusLevel = mcMMO.p.getAdvancedConfig().getMaxBonusLevel(subSkillType);
-                return new ProbabilityImpl(xPos, xCeiling, probabilityCeiling);
+                return calculateCurrentSkillProbability(skillLevel, 0, probabilityCeiling, maxBonusLevel);
             case STATIC_CONFIGURABLE:
                 try {
                     return getStaticRandomChance(subSkillType);
@@ -127,22 +127,7 @@ public class ProbabilityUtil {
      * @return true if the Skill RNG succeeds, false if it fails
      */
     public static boolean isSkillRNGSuccessful(@NotNull SubSkillType subSkillType, @NotNull Player player) {
-        //Process probability
+        final Probability probability = getSkillProbability(subSkillType, player);
-        Probability probability = getSubSkillProbability(subSkillType, player);
-
-        //Send out event
-        SubSkillEvent subSkillEvent = EventUtils.callSubSkillEvent(player, subSkillType);
-
-        if(subSkillEvent.isCancelled()) {
-            return false; //Event got cancelled so this doesn't succeed
-        }
-
-        //Result modifier
-        double resultModifier = subSkillEvent.getResultModifier();
-
-        //Mutate probability
-        if(resultModifier != 1.0D)
-            probability = Probability.ofPercent(probability.getValue() * resultModifier);
 
         //Luck
         boolean isLucky = Permissions.lucky(player, subSkillType.getParentSkill());
@@ -154,12 +139,42 @@ public class ProbabilityUtil {
         }
     }
 
+    /**
+     * Returns the {@link Probability} for a specific {@link SubSkillType} for a specific {@link Player}.
+     * This does not take into account perks such as lucky for the player.
+     * This is affected by other plugins who can listen to the {@link SubSkillEvent} and cancel it or mutate it.
+     *
+     * @param subSkillType the target subskill
+     * @param player the target player
+     * @return the probability for this skill
+     */
+    public static Probability getSkillProbability(@NotNull SubSkillType subSkillType, @NotNull Player player) {
+        //Process probability
+        Probability probability = getSubSkillProbability(subSkillType, player);
+
+        //Send out event
+        SubSkillEvent subSkillEvent = EventUtils.callSubSkillEvent(player, subSkillType);
+
+        if(subSkillEvent.isCancelled()) {
+            return Probability.ALWAYS_FAILS;
+        }
+
+        //Result modifier
+        double resultModifier = subSkillEvent.getResultModifier();
+
+        //Mutate probability
+        if(resultModifier != 1.0D)
+            probability = Probability.ofPercent(probability.getValue() * resultModifier);
+
+        return probability;
+    }
+
     /**
      * This is one of several Skill RNG check methods
      * This helper method is specific to static value RNG, which can be influenced by a player's Luck
      *
      * @param primarySkillType the related primary skill
-     * @param player the target player, can be null (null players have the worst odds)
+     * @param player the target player can be null (null players have the worst odds)
      * @param probabilityPercentage the probability of this player succeeding in "percentage" format (0-100 inclusive)
      * @return true if the RNG succeeds, false if it fails
      */
@@ -223,4 +238,31 @@ public class ProbabilityUtil {
 
         return new String[]{percent.format(firstValue), percent.format(secondValue)};
     }
+
+    /**
+     * Helper function to calculate what probability a given skill has at a certain level
+     * @param skillLevel the skill level currently between the floor and the ceiling
+     * @param floor the minimum odds this skill can have
+     * @param ceiling the maximum odds this skill can have
+     * @param maxBonusLevel the maximum level this skill can have to reach the ceiling
+     *
+     * @return the probability of success for this skill at this level
+     */
+    public static Probability calculateCurrentSkillProbability(double skillLevel, double floor,
+                                                               double ceiling, double maxBonusLevel) {
+        // The odds of success are between the value of the floor and the value of the ceiling.
+        // If the skill has a maxBonusLevel of 500 on this skill, then at skill level 500 you would have the full odds,
+        // at skill level 250 it would be half odds.
+
+        if (skillLevel >= maxBonusLevel || maxBonusLevel <= 0) {
+            // Avoid divide by zero bugs
+            // Max benefit has been reached, should always succeed
+            return Probability.ofPercent(ceiling);
+        }
+
+        double odds = (skillLevel / maxBonusLevel) * 100D;
+
+        // 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/ProbabilityTest.java

@@ -14,19 +14,19 @@ class ProbabilityTest {
     private static Stream<Arguments> provideProbabilitiesForWithinExpectations() {
         return Stream.of(
                 // static probability, % of time for success
-                Arguments.of(new ProbabilityImpl(5), 5),
+                Arguments.of(new ProbabilityImpl(.05), 5),
-                Arguments.of(new ProbabilityImpl(10), 10),
+                Arguments.of(new ProbabilityImpl(.10), 10),
-                Arguments.of(new ProbabilityImpl(15), 15),
+                Arguments.of(new ProbabilityImpl(.15), 15),
-                Arguments.of(new ProbabilityImpl(20), 20),
+                Arguments.of(new ProbabilityImpl(.20), 20),
-                Arguments.of(new ProbabilityImpl(25), 25),
+                Arguments.of(new ProbabilityImpl(.25), 25),
-                Arguments.of(new ProbabilityImpl(50), 50),
+                Arguments.of(new ProbabilityImpl(.50), 50),
-                Arguments.of(new ProbabilityImpl(75), 75),
+                Arguments.of(new ProbabilityImpl(.75), 75),
-                Arguments.of(new ProbabilityImpl(90), 90),
+                Arguments.of(new ProbabilityImpl(.90), 90),
-                Arguments.of(new ProbabilityImpl(99.9), 99.9),
+                Arguments.of(new ProbabilityImpl(.999), 99.9),
-                Arguments.of(new ProbabilityImpl(0.05), 0.05),
+                Arguments.of(new ProbabilityImpl(0.0005), 0.05),
-                Arguments.of(new ProbabilityImpl(0.1), 0.1),
+                Arguments.of(new ProbabilityImpl(0.001), 0.1),
-                Arguments.of(new ProbabilityImpl(500), 100),
+                Arguments.of(new ProbabilityImpl(50.0), 100),
-                Arguments.of(new ProbabilityImpl(1000), 100)
+                Arguments.of(new ProbabilityImpl(100.0), 100)
         );
     }
 

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

@@ -0,0 +1,22 @@
+package com.gmail.nossr50.util.random;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+public class ProbabilityTestUtils {
+    public static void assertProbabilityExpectations(double expectedWinPercent, Probability probability) {
+        double iterations = 2.0e7; //20 million
+        double winCount = 0;
+        for (int i = 0; i < iterations; i++) {
+            if(probability.evaluate()) {
+                winCount++;
+            }
+        }
+
+        double successPercent = (winCount / iterations) * 100;
+        System.out.println("Wins: " + winCount);
+        System.out.println("Fails: " + (iterations - winCount));
+        System.out.println("Percentage succeeded: " + successPercent + ", Expected: " + expectedWinPercent);
+        assertEquals(expectedWinPercent, successPercent, 0.025D);
+        System.out.println("Variance is within tolerance levels!");
+    }
+}

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

@@ -1,39 +1,44 @@
 package com.gmail.nossr50.util.random;
 
-import com.gmail.nossr50.config.AdvancedConfig;
+import com.gmail.nossr50.MMOTestEnvironment;
 import com.gmail.nossr50.datatypes.skills.SubSkillType;
-import com.gmail.nossr50.mcMMO;
+import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.MethodSource;
 
+import java.util.logging.Logger;
 import java.util.stream.Stream;
 
 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;
+import static java.util.logging.Logger.getLogger;
 import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
-class ProbabilityUtilTest {
+class ProbabilityUtilTest extends MMOTestEnvironment {
-    mcMMO mmoInstance;
+    private static final Logger logger = getLogger(ProbabilityUtilTest.class.getName());
-    AdvancedConfig advancedConfig;
 
     final static double impactChance = 11D;
     final static double greaterImpactChance = 0.007D;
     final static double fastFoodChance = 45.5D;
 
     @BeforeEach
-    public void setupMocks() throws NoSuchFieldException, IllegalAccessException {
+    public void setupMocks() {
-        this.mmoInstance = mock(mcMMO.class);
+        mockBaseEnvironment(logger);
-        mcMMO.class.getField("p").set(null, mmoInstance);
-        this.advancedConfig = mock(AdvancedConfig.class);
-        when(mmoInstance.getAdvancedConfig()).thenReturn(advancedConfig);
         when(advancedConfig.getImpactChance()).thenReturn(impactChance);
         when(advancedConfig.getGreaterImpactChance()).thenReturn(greaterImpactChance);
         when(advancedConfig.getFastFoodChance()).thenReturn(fastFoodChance);
     }
 
+    @AfterEach
+    public void tearDown() {
+        cleanupBaseEnvironment();
+    }
+
     private static Stream<Arguments> staticChanceSkills() {
         return Stream.of(
                 // static probability, % of time for success
@@ -45,22 +50,26 @@ class ProbabilityUtilTest {
 
     @ParameterizedTest
     @MethodSource("staticChanceSkills")
-    void testStaticChanceSkills(SubSkillType subSkillType, double expectedWinPercent) throws InvalidStaticChance {
+    void staticChanceSkillsShouldSucceedAsExpected(SubSkillType subSkillType, double expectedWinPercent)
+            throws InvalidStaticChance {
         Probability staticRandomChance = ProbabilityUtil.getStaticRandomChance(subSkillType);
         assertProbabilityExpectations(expectedWinPercent, staticRandomChance);
     }
 
-    private static void assertProbabilityExpectations(double expectedWinPercent, Probability probability) {
+    @Test
-        double iterations = 2.0e7;
+    public void isSkillRNGSuccessfulShouldBehaveAsExpected() {
-        double winCount = 0;
+        // Given
-        for (int i = 0; i < iterations; i++) {
+        when(advancedConfig.getMaximumProbability(UNARMED_ARROW_DEFLECT)).thenReturn(20D);
-            if(probability.evaluate()) {
+        when(advancedConfig.getMaxBonusLevel(UNARMED_ARROW_DEFLECT)).thenReturn(0);
-                winCount++;
+
-            }
+        final Probability probability = ProbabilityUtil.getSkillProbability(UNARMED_ARROW_DEFLECT, player);
+        assertEquals(0.2D, probability.getValue());
+        assertProbabilityExpectations(20, probability);
     }
 
-        double successPercent = (winCount / iterations) * 100;
+    @Test
-        System.out.println(successPercent + ", " + expectedWinPercent);
+    public void calculateCurrentSkillProbabilityShouldBeTwenty() {
-        assertEquals(expectedWinPercent, successPercent, 0.05D);
+        final Probability probability = calculateCurrentSkillProbability(1000, 0, 20, 1000);
+        assertEquals(0.2D, probability.getValue());
     }
 }