Merge pull request #1912 from GriffinRichards/moves-mask

Handful of fixes related to MAX_MON_MOVES

include/battle.h

@@ -392,7 +392,7 @@ struct BattleStruct
     u8 expGetterBattlerId;
     u8 unused_5;
     u8 absentBattlerFlags;
-    u8 palaceFlags; // First 4 bits are "is < 50% HP and not asleep" for each battler, last 4 bits are selected moves to pass to AI
+    u8 palaceFlags; // First 4 bits are "is <= 50% HP and not asleep" for each battler, last 4 bits are selected moves to pass to AI
     u8 field_93; // related to choosing pokemon?
     u8 wallyBattleState;
     u8 wallyMovesState;
@@ -440,6 +440,11 @@ struct BattleStruct
     u8 alreadyStatusedMoveAttempt; // As bits for battlers; For example when using Thunder Wave on an already paralyzed pokemon.
 };
 
+// The palaceFlags member of struct BattleStruct contains 1 flag per move to indicate which moves the AI should consider,
+// and 1 flag per battler to indicate whether the battler is awake and at <= 50% HP (which affects move choice).
+// The assert below is to ensure palaceFlags is large enough to store these flags without overlap.
+STATIC_ASSERT(sizeof(((struct BattleStruct *)0)->palaceFlags) * 8 >= MAX_BATTLERS_COUNT + MAX_MON_MOVES, PalaceFlagsTooSmall)
+
 #define F_DYNAMIC_TYPE_1 (1 << 6)
 #define F_DYNAMIC_TYPE_2 (1 << 7)
 #define DYNAMIC_TYPE_MASK (F_DYNAMIC_TYPE_1 - 1)

include/constants/global.h

@@ -80,6 +80,7 @@
 
 #define TRAINER_ID_LENGTH 4
 #define MAX_MON_MOVES 4
+#define ALL_MOVES_MASK ((1 << MAX_MON_MOVES) - 1)
 
 #define CONTESTANT_COUNT 4
 #define CONTEST_CATEGORY_COOL     0

include/global.h

@@ -85,6 +85,12 @@
 #define SAFE_DIV(a, b) ((a) / (b))
 #endif
 
+// The below macro does a%n, but (to match) will switch to a&(n-1) if n is a power of 2.
+// There are cases where GF does a&(n-1) where we would really like to have a%n, because
+// if n is changed to a value that isn't a power of 2 then a&(n-1) is unlikely to work as
+// intended, and a%n for powers of 2 isn't always optimized to use &.
+#define MOD(a, n)(((n) & ((n)-1)) ? ((a) % (n)) : ((a) & ((n)-1)))
+
 // Extracts the upper 16 bits of a 32-bit number
 #define HIHALF(n) (((n) & 0xFFFF0000) >> 16)
 

src/battle_ai_script_commands.c

@@ -478,9 +478,9 @@ static u8 ChooseMoveOrAction_Doubles(void)
         else
         {
             if (gBattleTypeFlags & BATTLE_TYPE_PALACE)
-                BattleAI_SetupAIData(gBattleStruct->palaceFlags >> 4);
+                BattleAI_SetupAIData(gBattleStruct->palaceFlags >> MAX_BATTLERS_COUNT);
             else
-                BattleAI_SetupAIData((1 << MAX_MON_MOVES) - 1);
+                BattleAI_SetupAIData(ALL_MOVES_MASK);
 
             gBattlerTarget = i;
 

src/battle_controller_opponent.c

@@ -1555,7 +1555,7 @@ static void OpponentHandleChooseMove(void)
         if (gBattleTypeFlags & (BATTLE_TYPE_TRAINER | BATTLE_TYPE_FIRST_BATTLE | BATTLE_TYPE_SAFARI | BATTLE_TYPE_ROAMER))
         {
 
-            BattleAI_SetupAIData(0xF);
+            BattleAI_SetupAIData(ALL_MOVES_MASK);
             chosenMoveId = BattleAI_ChooseMoveOrAction();
 
             switch (chosenMoveId)
@@ -1588,7 +1588,7 @@ static void OpponentHandleChooseMove(void)
             u16 move;
             do
             {
-                chosenMoveId = Random() & 3;
+                chosenMoveId = MOD(Random(), MAX_MON_MOVES);
                 move = moveInfo->moves[chosenMoveId];
             } while (move == MOVE_NONE);
 

src/battle_controller_player_partner.c

@@ -1515,7 +1515,7 @@ static void PlayerPartnerHandleChooseMove(void)
     u8 chosenMoveId;
     struct ChooseMoveStruct *moveInfo = (struct ChooseMoveStruct *)(&gBattleBufferA[gActiveBattler][4]);
 
-    BattleAI_SetupAIData(0xF);
+    BattleAI_SetupAIData(ALL_MOVES_MASK);
     chosenMoveId = BattleAI_ChooseMoveOrAction();
 
     if (gBattleMoves[moveInfo->moves[chosenMoveId]].target & (MOVE_TARGET_USER | MOVE_TARGET_USER_OR_SELECTED))

src/battle_gfx_sfx_util.c

@@ -121,9 +121,9 @@ u16 ChooseMoveAndTargetInBattlePalace(void)
     #define selectedGroup percent
     #define selectedMoves var2
     #define moveTarget var1
-    #define validMoveFlags var1
+    #define numMovesPerGroup var1
-    #define numValidMoveGroups var2
+    #define numMultipleMoveGroups var2
-    #define validMoveGroup var2
+    #define randSelectGroup var2
 
     // If battler is < 50% HP and not asleep, use second set of move group likelihoods
     // otherwise use first set
@@ -157,8 +157,10 @@ u16 ChooseMoveAndTargetInBattlePalace(void)
     // Pass selected moves to AI, pick one
     if (selectedMoves != 0)
     {
-        gBattleStruct->palaceFlags &= 0xF;
+        // Lower 4 bits of palaceFlags are flags for each battler.
-        gBattleStruct->palaceFlags |= (selectedMoves << 4);
+        // Clear the rest of palaceFlags, then set the selected moves in the upper 4 bits.
+        gBattleStruct->palaceFlags &= (1 << MAX_BATTLERS_COUNT) - 1;
+        gBattleStruct->palaceFlags |= (selectedMoves << MAX_BATTLERS_COUNT);
         BattleAI_SetupAIData(selectedMoves);
         chosenMoveId = BattleAI_ChooseMoveOrAction();
     }
@@ -168,34 +170,47 @@ u16 ChooseMoveAndTargetInBattlePalace(void)
     // If a move is chosen this way, there's a 50% chance that it will be unable to use it anyway
     if (chosenMoveId == -1)
     {
-        if (unusableMovesBits != 0xF)
+        if (unusableMovesBits != ALL_MOVES_MASK)
         {
-            validMoveFlags = 0, numValidMoveGroups = 0;
+            numMovesPerGroup = 0, numMultipleMoveGroups = 0;
 
             for (i = 0; i < MAX_MON_MOVES; i++)
             {
-                // validMoveFlags is used here as a bitfield for which moves can be used for each move group type
+                // Count the number of usable moves the battler has in each move group.
-                // first 4 bits are for attack (1 for each move), then 4 bits for defense, and 4 for support
+                // The totals will be stored separately in 3 groups of 4 bits each in numMovesPerGroup.
                 if (GetBattlePalaceMoveGroup(moveInfo->moves[i]) == PALACE_MOVE_GROUP_ATTACK && !(gBitTable[i] & unusableMovesBits))
-                    validMoveFlags += (1 << 0);
+                    numMovesPerGroup += (1 << 0);
                 if (GetBattlePalaceMoveGroup(moveInfo->moves[i]) == PALACE_MOVE_GROUP_DEFENSE && !(gBitTable[i] & unusableMovesBits))
-                    validMoveFlags += (1 << 4);
+                    numMovesPerGroup += (1 << 4);
                 if (GetBattlePalaceMoveGroup(moveInfo->moves[i]) == PALACE_MOVE_GROUP_SUPPORT && !(gBitTable[i] & unusableMovesBits))
-                    validMoveFlags += (1 << 8);
+                    numMovesPerGroup += (1 << 8);
             }
 
-            // Count the move groups the pokemon has
+            // Count the number of move groups for which the battler has at least 2 usable moves.
-            if ((validMoveFlags & 0xF) > 1)
+            // This is a roundabout way to determine if there is a move group that should be
-                numValidMoveGroups++;
+            // preferred, because it has multiple move options and the others do not.
-            if ((validMoveFlags & 0xF0) > 0x1F)
+            // The condition intended to check the total for the Support group is accidentally
-                numValidMoveGroups++;
+            // checking the Defense total, and is never true. As a result the preferences for
-            if ((validMoveFlags & 0xF0) > 0x1FF)
+            // random move selection here will skew away from the Support move group.
-                numValidMoveGroups++;
+            if ((numMovesPerGroup & 0xF) >= 2)
+                numMultipleMoveGroups++;
+            if ((numMovesPerGroup & (0xF << 4)) >= (2 << 4))
+                numMultipleMoveGroups++;
+#ifdef BUGFIX
+            if ((numMovesPerGroup & (0xF << 8)) >= (2 << 8))
+#else
+            if ((numMovesPerGroup & (0xF << 4)) >= (2 << 8))
+#endif
+                numMultipleMoveGroups++;
 
 
-            // If more than 1 possible move group, or no possible move groups
+            // By this point we already know the battler only has usable moves from at most 2 of the 3 move groups,
-            // then choose move randomly
+            // because they had no usable moves from the move group that was selected based on Nature.
-            if (numValidMoveGroups > 1 || numValidMoveGroups == 0)
+            //
+            // The below condition is effectively 'numMultipleMoveGroups != 1'.
+            // There is no stand-out group with multiple moves to choose from, so we pick randomly.
+            // Note that because of the bug above the battler may actually have any number of Support moves.
+            if (numMultipleMoveGroups > 1 || numMultipleMoveGroups == 0)
             {
                 do
                 {
@@ -204,27 +219,36 @@ u16 ChooseMoveAndTargetInBattlePalace(void)
                         chosenMoveId = i;
                 } while (chosenMoveId == -1);
             }
-            // Otherwise randomly choose move of only available move group
             else
             {
-                if ((validMoveFlags & 0xF) > 1)
+                // The battler has just 1 move group with multiple move options to choose from.
-                    validMoveGroup = PALACE_MOVE_GROUP_ATTACK;
+                // Choose a move randomly from this group.
-                if ((validMoveFlags & 0xF0) > 0x1F)
+
-                    validMoveGroup = PALACE_MOVE_GROUP_DEFENSE;
+                // Same bug as the previous set of conditions (the condition for Support is never true).
-                if ((validMoveFlags & 0xF0) > 0x1FF)
+                // This bug won't cause a softlock below, because if Support is the only group with multiple
-                    validMoveGroup = PALACE_MOVE_GROUP_SUPPORT;
+                // moves then it won't have been counted, and the 'numMultipleMoveGroups == 0' above will be true.
+                if ((numMovesPerGroup & 0xF) >= 2)
+                    randSelectGroup = PALACE_MOVE_GROUP_ATTACK;
+                if ((numMovesPerGroup & (0xF << 4)) >= (2 << 4))
+                    randSelectGroup = PALACE_MOVE_GROUP_DEFENSE;
+#ifdef BUGFIX
+                if ((numMovesPerGroup & (0xF << 8)) >= (2 << 8))
+#else
+                if ((numMovesPerGroup & (0xF << 4)) >= (2 << 8))
+#endif
+                    randSelectGroup = PALACE_MOVE_GROUP_SUPPORT;
 
                 do
                 {
                     i = Random() % MAX_MON_MOVES;
-                    if (!(gBitTable[i] & unusableMovesBits) && validMoveGroup == GetBattlePalaceMoveGroup(moveInfo->moves[i]))
+                    if (!(gBitTable[i] & unusableMovesBits) && randSelectGroup == GetBattlePalaceMoveGroup(moveInfo->moves[i]))
                         chosenMoveId = i;
                 } while (chosenMoveId == -1);
             }
 
-            // If a move was selected (and in this case was not from the Nature-chosen group)
+            // Because the selected move was not from the Nature-chosen move group there's a 50% chance
-            // then there's a 50% chance it won't be used anyway
+            // that it will be unable to use it. This could have been checked earlier to avoid the above work.
-            if (Random() % 100 > 49)
+            if (Random() % 100 >= 50)
             {
                 gProtectStructs[gActiveBattler].palaceUnableToUseMove = TRUE;
                 return 0;
@@ -232,6 +256,7 @@ u16 ChooseMoveAndTargetInBattlePalace(void)
         }
         else
         {
+            // All the battler's moves were flagged as unusable.
             gProtectStructs[gActiveBattler].palaceUnableToUseMove = TRUE;
             return 0;
         }
@@ -264,9 +289,9 @@ u16 ChooseMoveAndTargetInBattlePalace(void)
 #undef selectedGroup
 #undef selectedMoves
 #undef moveTarget
-#undef validMoveFlags
+#undef numMovesPerGroup
-#undef numValidMoveGroups
+#undef numMultipleMoveGroups
-#undef validMoveGroup
+#undef randSelectGroup
 
 static u8 GetBattlePalaceMoveGroup(u16 move)
 {

src/battle_pyramid.c

@@ -38,7 +38,7 @@
 #include "constants/moves.h"
 #include "constants/trainers.h"
 
-#define NUM_LAYOUT_OFFSETS 8 // Assumed to be a power of 2
+#define NUM_LAYOUT_OFFSETS 8
 
 extern const struct MapLayout *const gMapLayouts[];
 
@@ -1904,7 +1904,7 @@ static void GetPyramidFloorLayoutOffsets(u8 *layoutOffsets)
 
     for (i = 0; i < NUM_PYRAMID_FLOOR_SQUARES; i++)
     {
-        layoutOffsets[i] = sPyramidFloorTemplates[id].layoutOffsets[rand & (NUM_LAYOUT_OFFSETS - 1)];
+        layoutOffsets[i] = sPyramidFloorTemplates[id].layoutOffsets[MOD(rand, NUM_LAYOUT_OFFSETS)];
         rand >>= 3;
         if (i == 7)
         {

src/battle_script_commands.c

@@ -849,7 +849,7 @@ static const u8 sBallCatchBonuses[] =
 // In Battle Palace, moves are chosen based on the pokemons nature rather than by the player
 // Moves are grouped into "Attack", "Defense", or "Support" (see PALACE_MOVE_GROUP_*)
 // Each nature has a certain percent chance of selecting a move from a particular group
-// and a separate percent chance for each group when below 50% HP
+// and a separate percent chance for each group when at or below 50% HP
 // The table below doesn't list percentages for Support because you can subtract the other two
 // Support percentages are listed in comments off to the side instead
 #define PALACE_STYLE(atk, def, atkLow, defLow) {atk, atk + def, atkLow, atkLow + defLow}
@@ -7350,7 +7350,7 @@ static void Cmd_tryconversiontypechange(void)
     {
         do
         {
-            while ((moveChecked = Random() & (MAX_MON_MOVES - 1)) >= validMoves);
+            while ((moveChecked = MOD(Random(), MAX_MON_MOVES)) >= validMoves);
 
             moveType = gBattleMoves[gBattleMons[gBattlerAttacker].moves[moveChecked]].type;
 
@@ -8179,7 +8179,7 @@ static void Cmd_trychoosesleeptalkmove(void)
     }
 
     unusableMovesBits = CheckMoveLimitations(gBattlerAttacker, unusableMovesBits, ~MOVE_LIMITATION_PP);
-    if (unusableMovesBits == (1 << MAX_MON_MOVES) - 1) // all 4 moves cannot be chosen
+    if (unusableMovesBits == ALL_MOVES_MASK) // all 4 moves cannot be chosen
     {
         gBattlescriptCurrInstr += 5;
     }
@@ -8189,7 +8189,7 @@ static void Cmd_trychoosesleeptalkmove(void)
 
         do
         {
-            movePosition = Random() & (MAX_MON_MOVES - 1);
+            movePosition = MOD(Random(), MAX_MON_MOVES);
         } while ((gBitTable[movePosition] & unusableMovesBits));
 
         gCalledMove = gBattleMons[gBattlerAttacker].moves[movePosition];

src/battle_util.c

@@ -1110,7 +1110,6 @@ u8 CheckMoveLimitations(u8 battlerId, u8 unusableMoves, u8 check)
     return unusableMoves;
 }
 
-#define ALL_MOVES_MASK ((1 << MAX_MON_MOVES) - 1)
 bool8 AreAllMovesUnusable(void)
 {
     u8 unusable = CheckMoveLimitations(gActiveBattler, 0, MOVE_LIMITATIONS_ALL);
@@ -1127,7 +1126,6 @@ bool8 AreAllMovesUnusable(void)
 
     return (unusable == ALL_MOVES_MASK);
 }
-#undef ALL_MOVES_MASK
 
 u8 GetImprisonedMovesCount(u8 battlerId, u16 move)
 {
@@ -2775,11 +2773,12 @@ u8 AbilityBattleEffects(u8 caseID, u8 battler, u8 ability, u8 special, u16 moveA
                 {
                     do
                     {
+                        // Pick either MOVE_EFFECT_SLEEP, MOVE_EFFECT_POISON or MOVE_EFFECT_BURN
                         gBattleCommunication[MOVE_EFFECT_BYTE] = Random() & 3;
                     } while (gBattleCommunication[MOVE_EFFECT_BYTE] == 0);
-
+                    // Replace MOVE_EFFECT_BURN with MOVE_EFFECT_PARALYSIS
                     if (gBattleCommunication[MOVE_EFFECT_BYTE] == MOVE_EFFECT_BURN)
-                        gBattleCommunication[MOVE_EFFECT_BYTE] += 2; // 5 MOVE_EFFECT_PARALYSIS
+                        gBattleCommunication[MOVE_EFFECT_BYTE] += (MOVE_EFFECT_PARALYSIS - MOVE_EFFECT_BURN);
 
                     gBattleCommunication[MOVE_EFFECT_BYTE] += MOVE_EFFECT_AFFECTS_USER;
                     BattleScriptPushCursor();
@@ -3954,11 +3953,11 @@ u8 IsMonDisobedient(void)
     if (calc < obedienceLevel)
     {
         calc = CheckMoveLimitations(gBattlerAttacker, gBitTable[gCurrMovePos], MOVE_LIMITATIONS_ALL);
-        if (calc == 0xF) // all moves cannot be used
+        if (calc == ALL_MOVES_MASK) // all moves cannot be used
         {
             // Randomly select, then print a disobedient string
             // B_MSG_LOAFING, B_MSG_WONT_OBEY, B_MSG_TURNED_AWAY, or B_MSG_PRETEND_NOT_NOTICE
-            gBattleCommunication[MULTISTRING_CHOOSER] = Random() & (NUM_LOAF_STRINGS - 1);
+            gBattleCommunication[MULTISTRING_CHOOSER] = MOD(Random(), NUM_LOAF_STRINGS);
             gBattlescriptCurrInstr = BattleScript_MoveUsedLoafingAround;
             return 1;
         }
@@ -3966,7 +3965,7 @@ u8 IsMonDisobedient(void)
         {
             do
             {
-                gCurrMovePos = gChosenMovePos = Random() & (MAX_MON_MOVES - 1);
+                gCurrMovePos = gChosenMovePos = MOD(Random(), MAX_MON_MOVES);
             } while (gBitTable[gCurrMovePos] & calc);
 
             gCalledMove = gBattleMons[gBattlerAttacker].moves[gCurrMovePos];
@@ -4009,7 +4008,7 @@ u8 IsMonDisobedient(void)
         {
             // Randomly select, then print a disobedient string
             // B_MSG_LOAFING, B_MSG_WONT_OBEY, B_MSG_TURNED_AWAY, or B_MSG_PRETEND_NOT_NOTICE
-            gBattleCommunication[MULTISTRING_CHOOSER] = Random() & (NUM_LOAF_STRINGS - 1);
+            gBattleCommunication[MULTISTRING_CHOOSER] = MOD(Random(), NUM_LOAF_STRINGS);
             gBattlescriptCurrInstr = BattleScript_MoveUsedLoafingAround;
             return 1;
         }

src/contest_ai.c

@@ -326,7 +326,7 @@ u8 ContestAI_GetActionToUse(void)
     {
         // Randomly choose a move index. If it's the move
         // with the highest (or tied highest) score, return
-        u8 moveIdx = Random() & (MAX_MON_MOVES - 1); // % MAX_MON_MOVES doesn't match
+        u8 moveIdx = MOD(Random(), MAX_MON_MOVES);
         u8 score = eContestAI.moveScores[moveIdx];
         int i;
         for (i = 0; i < MAX_MON_MOVES; i++)

src/intro.c

@@ -1163,7 +1163,7 @@ void CB2_InitCopyrightScreenAfterTitleScreen(void)
 static void Task_Scene1_Load(u8 taskId)
 {
     SetVBlankCallback(NULL);
-    sIntroCharacterGender = Random() & 1;
+    sIntroCharacterGender = MOD(Random(), GENDER_COUNT);
     IntroResetGpuRegs();
     SetGpuReg(REG_OFFSET_BG3VOFS, 0);
     SetGpuReg(REG_OFFSET_BG2VOFS, 80);

src/naming_screen.c

@@ -1067,7 +1067,7 @@ static void SpriteCB_InputArrow(struct Sprite *sprite)
     if (sprite->sDelay == 0 || --sprite->sDelay == 0)
     {
         sprite->sDelay = 8;
-        sprite->sXPosId = (sprite->sXPosId + 1) & (ARRAY_COUNT(x) - 1);
+        sprite->sXPosId = MOD(sprite->sXPosId + 1, ARRAY_COUNT(x));
     }
     sprite->x2 = x[sprite->sXPosId];
 }
@@ -1097,7 +1097,7 @@ static void SpriteCB_Underscore(struct Sprite *sprite)
         sprite->sDelay++;
         if (sprite->sDelay > 8)
         {
-            sprite->sYPosId = (sprite->sYPosId + 1) & (ARRAY_COUNT(y) - 1);
+            sprite->sYPosId = MOD(sprite->sYPosId + 1, ARRAY_COUNT(y));
             sprite->sDelay = 0;
         }
     }