diff --git a/include/battle.h b/include/battle.h index 118d560984..bb0e42fb83 100644 --- a/include/battle.h +++ b/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) diff --git a/include/constants/global.h b/include/constants/global.h index 199acfe6c6..2b70378ff5 100644 --- a/include/constants/global.h +++ b/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 diff --git a/include/global.h b/include/global.h index e0cecafd07..e6d58f36e0 100644 --- a/include/global.h +++ b/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) diff --git a/src/battle_ai_script_commands.c b/src/battle_ai_script_commands.c index b2c900b422..aa34f46ec7 100644 --- a/src/battle_ai_script_commands.c +++ b/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; diff --git a/src/battle_controller_opponent.c b/src/battle_controller_opponent.c index 12402f91ea..e434e3ee62 100644 --- a/src/battle_controller_opponent.c +++ b/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); diff --git a/src/battle_controller_player_partner.c b/src/battle_controller_player_partner.c index 6127e2d655..236a66ce83 100644 --- a/src/battle_controller_player_partner.c +++ b/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)) diff --git a/src/battle_gfx_sfx_util.c b/src/battle_gfx_sfx_util.c index 55ed328b89..d98809be46 100644 --- a/src/battle_gfx_sfx_util.c +++ b/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 numValidMoveGroups var2 - #define validMoveGroup var2 + #define numMovesPerGroup var1 + #define numMultipleMoveGroups 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; - gBattleStruct->palaceFlags |= (selectedMoves << 4); + // Lower 4 bits of palaceFlags are flags for each battler. + // 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 - // first 4 bits are for attack (1 for each move), then 4 bits for defense, and 4 for support + // Count the number of usable moves the battler has in each move group. + // 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 - if ((validMoveFlags & 0xF) > 1) - numValidMoveGroups++; - if ((validMoveFlags & 0xF0) > 0x1F) - numValidMoveGroups++; - if ((validMoveFlags & 0xF0) > 0x1FF) - numValidMoveGroups++; + // Count the number of move groups for which the battler has at least 2 usable moves. + // This is a roundabout way to determine if there is a move group that should be + // preferred, because it has multiple move options and the others do not. + // The condition intended to check the total for the Support group is accidentally + // checking the Defense total, and is never true. As a result the preferences for + // random move selection here will skew away from the Support move group. + 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 - // then choose move randomly - if (numValidMoveGroups > 1 || numValidMoveGroups == 0) + // By this point we already know the battler only has usable moves from at most 2 of the 3 move groups, + // because they had no usable moves from the move group that was selected based on Nature. + // + // 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) - validMoveGroup = PALACE_MOVE_GROUP_ATTACK; - if ((validMoveFlags & 0xF0) > 0x1F) - validMoveGroup = PALACE_MOVE_GROUP_DEFENSE; - if ((validMoveFlags & 0xF0) > 0x1FF) - validMoveGroup = PALACE_MOVE_GROUP_SUPPORT; + // The battler has just 1 move group with multiple move options to choose from. + // Choose a move randomly from this group. + + // Same bug as the previous set of conditions (the condition for Support is never true). + // This bug won't cause a softlock below, because if Support is the only group with multiple + // 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) - // then there's a 50% chance it won't be used anyway - if (Random() % 100 > 49) + // Because the selected move was not from the Nature-chosen move group there's a 50% chance + // that it will be unable to use it. This could have been checked earlier to avoid the above work. + 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 numValidMoveGroups -#undef validMoveGroup +#undef numMovesPerGroup +#undef numMultipleMoveGroups +#undef randSelectGroup static u8 GetBattlePalaceMoveGroup(u16 move) { diff --git a/src/battle_pyramid.c b/src/battle_pyramid.c index a1f1ee542c..e51f804981 100644 --- a/src/battle_pyramid.c +++ b/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) { diff --git a/src/battle_script_commands.c b/src/battle_script_commands.c index 3c4b221ddf..7246a11d16 100644 --- a/src/battle_script_commands.c +++ b/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]; diff --git a/src/battle_util.c b/src/battle_util.c index ae87d6931b..f71c825c1b 100644 --- a/src/battle_util.c +++ b/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; } diff --git a/src/contest_ai.c b/src/contest_ai.c index 8753ed170d..a4bb5b4549 100644 --- a/src/contest_ai.c +++ b/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++) diff --git a/src/intro.c b/src/intro.c index e5e6a23dd2..6a6a58ddd5 100644 --- a/src/intro.c +++ b/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); diff --git a/src/naming_screen.c b/src/naming_screen.c index 6dd45aa7e1..6acb29ffbf 100644 --- a/src/naming_screen.c +++ b/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; } }