From 7ec1d9be0c8d01942b4c88d84df6ee4154b62970 Mon Sep 17 00:00:00 2001 From: Shane McGovern Date: Sat, 19 Sep 2026 13:26:02 +0100 Subject: [PATCH] fix(game3): gate the level-up stat window on the EXP sequence (#2324) The FRLG level-up stat window opened and the sequence kept pumping, so the box outlived its own step: it stopped taking input (routing is phase-gated), never closed, and drew over the win/money text. - exp_seq: update() now waits while the window is open (mirroring the already-correct busy()), and finish()/reset() tear the window down silently so a stale onDone cannot advance a sequence that no longer owns it. - stat_growth: close(opts) gains opts.silent for teardown callers. - init: the input-routing phase list moves into STAT_WINDOW_PHASES behind Battle.statWindowPhase(), and Battle.update() tears down a window whose phase can no longer dismiss it. - ui: the stat-window draw keys off the same predicate, so input and drawing can never disagree. Tests: the "In-Battle Level Up Stat Growth Window" block in tests/game3_battle_switch_and_faint_test.lua now pumps ExpSeq.update() while the box is open, and a new CI-covered tests/engine suite pins the same regression. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/core/game3/battle/exp_seq.lua | 32 +++-- src/core/game3/battle/init.lua | 26 +++- src/core/game3/battle/ui.lua | 12 +- src/ui/game3/stat_growth.lua | 6 +- .../battle_levelup_statwindow_bug2324.lua | 127 ++++++++++++++++++ tests/game3_battle_switch_and_faint_test.lua | 31 +++++ 6 files changed, 221 insertions(+), 13 deletions(-) create mode 100644 tests/engine/battle_levelup_statwindow_bug2324.lua diff --git a/src/core/game3/battle/exp_seq.lua b/src/core/game3/battle/exp_seq.lua index ab40205a..037be812 100644 --- a/src/core/game3/battle/exp_seq.lua +++ b/src/core/game3/battle/exp_seq.lua @@ -7,6 +7,17 @@ local Pokemon = require("src.core.game3.pokemon") local ExpSeq = {} +local function stat_growth() + local ok, SG = pcall(require, "src.ui.game3.stat_growth") + if ok and SG then return SG end + return nil +end + +local function stat_window_open() + local SG = stat_growth() + return (SG and SG.isOpen and SG.isOpen()) and true or false +end + ExpSeq._steps = nil ExpSeq._i = 1 ExpSeq._waiting = false @@ -34,18 +45,13 @@ function ExpSeq.reset() if okA and okS and Audio.stopSe and SE and SE.SE_EXP then Audio.stopSe(SE.SE_EXP) end - local okSG, StatGrowth = pcall(require, "src.ui.game3.stat_growth") - if okSG and StatGrowth and StatGrowth.close then - StatGrowth.close() - end + local StatGrowth = stat_growth() + if StatGrowth and StatGrowth.close then StatGrowth.close({ silent = true }) end LearnMove.reset() end function ExpSeq.busy() - local okSG, StatGrowth = pcall(require, "src.ui.game3.stat_growth") - if okSG and StatGrowth and StatGrowth.isOpen and StatGrowth.isOpen() then - return true - end + if stat_window_open() then return true end return ExpSeq._steps ~= nil or LearnMove.busy() end @@ -58,6 +64,10 @@ local function finish() ExpSeq._i = 1 ExpSeq._waiting = false ExpSeq._waitingMsg = false + -- The step that owned the stat window is over; drop it without letting its + -- stale callback advance a sequence that has already ended (#2324). + local StatGrowth = stat_growth() + if StatGrowth and StatGrowth.close then StatGrowth.close({ silent = true }) end local okA, Audio = pcall(require, "src.core.game3.audio") local okS, SE = pcall(require, "src.core.game3.se_ids") if okA and okS and Audio.stopSe and SE and SE.SE_EXP then @@ -334,6 +344,12 @@ function ExpSeq.update() if LearnMove.busy() then return false end + -- The level-up stat window waits for the player; its onDone callback clears + -- _waiting and advances. Without this the sequence ran straight past the + -- open window, leaving it on screen (#2324). + if stat_window_open() then + return false + end if not Anim.busy() then advance() else diff --git a/src/core/game3/battle/init.lua b/src/core/game3/battle/init.lua index 3cea3777..8ca1e53a 100644 --- a/src/core/game3/battle/init.lua +++ b/src/core/game3/battle/init.lua @@ -44,6 +44,21 @@ Battle._residualStepState = nil local D = {} +-- Phases in which the level-up stat window can still be dismissed by the +-- player. Input routing and drawing both key off this, so the window can never +-- linger somewhere it can no longer be dismissed (#2324). +local STAT_WINDOW_PHASES = { + awarding = true, + evolving = true, + switching = true, + shift_prompt = true, + catch_nickname_prompt = true, +} + +function Battle.statWindowPhase() + return STAT_WINDOW_PHASES[Battle._phase] == true +end + -- pokefirered/src/battle_interface.c:2168 local function hp_bar_red(hp, maxHp) local ok, BattleChrome = pcall(require, "src.ui.game3.battle_chrome") @@ -2140,6 +2155,14 @@ Battle.finishCatchFlow = finish_catch_flow function Battle.update(dt, game) if not Battle._active then return end + -- A stat window whose phase can no longer dismiss it must not linger (#2324). + if not Battle.statWindowPhase() then + local StatGrowth = package.loaded["src.ui.game3.stat_growth"] + if StatGrowth and StatGrowth.isOpen and StatGrowth.isOpen() then + StatGrowth.close({ silent = true }) + end + end + local input = game and game.input local Pokedex = package.loaded["src.ui.game3.pokedex"] if Pokedex and Pokedex.isOpen and Pokedex.isOpen() then @@ -2206,8 +2229,7 @@ function Battle.update(dt, game) end -- Choice input during award / shift prompt / evolution learn-move prompts / catch nickname prompt / evolving - if (Battle._phase == "awarding" or Battle._phase == "evolving" or Battle._phase == "switching" - or Battle._phase == "shift_prompt" or Battle._phase == "catch_nickname_prompt") + if Battle.statWindowPhase() and not Battle._auto and game and game.input then local EvolutionScene = package.loaded["src.ui.game3.evolution_scene"] if EvolutionScene and EvolutionScene.isOpen and EvolutionScene.isOpen() then diff --git a/src/core/game3/battle/ui.lua b/src/core/game3/battle/ui.lua index 1edaa2ca..709ee323 100644 --- a/src/core/game3/battle/ui.lua +++ b/src/core/game3/battle/ui.lua @@ -24,6 +24,15 @@ local BallOpen = require("src.core.game3.battle.ball_open") local Ui = {} +-- The stat window may only be on screen while the battle is in a phase that can +-- still dismiss it; init.lua owns the list (#2324). Resolved lazily because +-- init.lua requires this module. +local function stat_window_phase() + local Battle = package.loaded["src.core.game3.battle.init"] + if not (Battle and Battle.statWindowPhase) then return true end + return Battle.statWindowPhase() +end + Ui._queue = {} Ui._showing = false Ui._headless = false @@ -2032,7 +2041,8 @@ function Ui.draw(w, h) end local StatGrowth = package.loaded["src.ui.game3.stat_growth"] - if StatGrowth and StatGrowth.isOpen and StatGrowth.isOpen() and StatGrowth.draw then + if StatGrowth and StatGrowth.isOpen and StatGrowth.isOpen() and StatGrowth.draw + and stat_window_phase() then StatGrowth.draw() end diff --git a/src/ui/game3/stat_growth.lua b/src/ui/game3/stat_growth.lua index bb16f0d4..fccebd20 100644 --- a/src/ui/game3/stat_growth.lua +++ b/src/ui/game3/stat_growth.lua @@ -39,7 +39,9 @@ function StatGrowth.isOpen() return StatGrowth._open end -function StatGrowth.close() +--- opts.silent drops the window without firing its onDone callback, for callers +--- tearing down a step that no longer exists (#2324). +function StatGrowth.close(opts) local wasOpen = StatGrowth._open StatGrowth._open = false StatGrowth._mon = nil @@ -48,7 +50,7 @@ function StatGrowth.close() StatGrowth._page = 1 local cb = StatGrowth._onDone StatGrowth._onDone = nil - if wasOpen and cb then cb() end + if wasOpen and cb and not (opts and opts.silent) then cb() end end function StatGrowth.handleInput(input) diff --git a/tests/engine/battle_levelup_statwindow_bug2324.lua b/tests/engine/battle_levelup_statwindow_bug2324.lua new file mode 100644 index 00000000..f4211069 --- /dev/null +++ b/tests/engine/battle_levelup_statwindow_bug2324.lua @@ -0,0 +1,127 @@ +-- #2324: the level-up stat window gates the EXP sequence; it is never run past. +-- +-- pokefirered draws the lvlup box from Cmd_drawlvlupbox and then *waits* for the +-- player (battle_script_commands.c: Cmd_drawlvlupbox ends the command, the +-- controller only returns after LvlUpBoxInput sees A/B). The FRLG rewrite +-- opened the box and kept pumping, so the box outlived its own step: it could +-- no longer be dismissed (input routing is phase-gated) and sat on screen over +-- the win/money text. +-- +-- luajit tests/engine/battle_levelup_statwindow_bug2324.lua + +package.path = "./?.lua;./?/init.lua;" .. package.path + +local T = require("tests.modkit") +local check, eq = T.check, T.eq + +local Experience = require("src.core.game3.battle.experience") +local ExpSeq = require("src.core.game3.battle.exp_seq") +local StatGrowth = require("src.ui.game3.stat_growth") +local Ui = require("src.core.game3.battle.ui") +local Anim = require("src.core.game3.battle.anim") +local Battle = require("src.core.game3.battle.init") + +local A_PRESS = { wasPressed = function(_, key) return key == "a" end } +local NO_PRESS = { wasPressed = function() return false end } + +-- A mon one award away from a level, so the sequence always grows stats. +local function levelUpMon() + return { + species = 1, + level = 5, + hp = 20, + maxHp = 20, + attack = 10, + defense = 10, + spAtk = 12, + spDef = 12, + speed = 9, + exp = Experience.expForLevel(3, 5), + growthRate = 3, + } +end + +--- Runs the sequence up to the point the stat window opens. +--- Returns the pushMsg log and the ExpSeq step index at that moment. +local function pumpToStatWindow() + local mon = levelUpMon() + local res = Experience.apply(mon, 100) + local messages = {} + local awards = { { mon = mon, result = res, partyIndex = 1 } } + + Ui.reset({ headless = false }) + check(ExpSeq.begin(awards, function(t) messages[#messages + 1] = t end, nil, + { headless = false }), "the level-up award starts an EXP sequence") + + -- exp-gain text, then the bar, then the grew-to-LV text + for _ = 1, 4 do + ExpSeq.update() + Ui._showing = false + Ui._queue = {} + Anim.reset({ headless = false }) + end + + check(StatGrowth.isOpen(), "the stat window opened on level up") + eq(StatGrowth._page, 1, "the stat window starts on Page 1 (diffs)") + return messages, ExpSeq._i, mon, res +end + +do + local messages, stepBefore = pumpToStatWindow() + local msgsBefore = #messages + + -- The box waits for the player: idle pumps must not move the sequence. + for i = 1, 20 do + eq(ExpSeq.update(), false, "pump " .. i .. " reports busy while the box is open") + end + eq(ExpSeq._i, stepBefore, "idle pumps did not advance the sequence") + eq(#messages, msgsBefore, "idle pumps pushed no further battle text") + check(StatGrowth.isOpen(), "the box is still open after 20 idle pumps") + eq(StatGrowth._page, 1, "the box is still on Page 1 after 20 idle pumps") + + -- Page 2 is still a wait. + check(StatGrowth.handleInput(A_PRESS), "A consumed on Page 1") + eq(StatGrowth._page, 2, "the box flipped to Page 2 (new values)") + check(StatGrowth.isOpen(), "the box is still open on Page 2") + for _ = 1, 20 do + eq(ExpSeq.update(), false, "pump reports busy while Page 2 is open") + end + eq(ExpSeq._i, stepBefore, "Page 2 idle pumps did not advance the sequence") + eq(#messages, msgsBefore, "Page 2 idle pumps pushed no further battle text") + + -- The second A press is what releases the sequence. + check(StatGrowth.handleInput(A_PRESS), "A consumed on Page 2") + check(not StatGrowth.isOpen(), "the second A press closed the box") + eq(ExpSeq._i, stepBefore + 1, "the sequence advanced only once the box closed") + + -- And it can be driven to the end from there without the box coming back. + local guard = 0 + while not ExpSeq.update() and guard < 200 do + guard = guard + 1 + Ui._showing = false + Ui._queue = {} + Anim.reset({ headless = false }) + end + check(guard < 200, "the sequence finishes after the box is dismissed") + check(not StatGrowth.isOpen(), "no stat window survives the finished sequence") + check(ExpSeq.busy() == false, "ExpSeq.busy() is false once the sequence is done") +end + +do + -- A box whose phase can no longer dismiss it must not linger: input routing is + -- phase-gated, so a box left open outside those phases would be undismissable + -- and would draw over the win/money text. + local _, _, mon, res = pumpToStatWindow() + + local savedActive, savedPhase, savedHeadless = Battle._active, Battle._phase, Battle._headless + Battle._active, Battle._phase, Battle._headless = true, nil, true + StatGrowth.open(mon, res.steps[1].oldStats, res.steps[1].newStats, function() + error("a torn-down stat window must not fire its onDone callback") + end) + check(StatGrowth.isOpen(), "a stale box is open before Battle.update") + Battle.update(1 / 60, { input = NO_PRESS }) + check(not StatGrowth.isOpen(), "Battle.update tears down a box outside its phases") + Battle._active, Battle._phase, Battle._headless = savedActive, savedPhase, savedHeadless +end + +T.finish("battle_levelup_statwindow_bug2324") diff --git a/tests/game3_battle_switch_and_faint_test.lua b/tests/game3_battle_switch_and_faint_test.lua index 966c2c4a..5b1be02e 100644 --- a/tests/game3_battle_switch_and_faint_test.lua +++ b/tests/game3_battle_switch_and_faint_test.lua @@ -568,6 +568,17 @@ do check(StatGrowth.isOpen(), "StatGrowth window opened on level up") eq(StatGrowth._page, 1, "StatGrowth starts on Page 1 (diffs)") + -- #2324: the sequence must WAIT on the window, not run past it. + local stepBefore = ExpSeq._i + local msgsBefore = #messages + for _ = 1, 20 do + eq(ExpSeq.update(), false, "ExpSeq.update() reports busy while the stat window is open") + end + eq(ExpSeq._i, stepBefore, "ExpSeq did not advance past the open stat window") + eq(#messages, msgsBefore, "no battle text pushed while the stat window is open") + check(StatGrowth.isOpen(), "StatGrowth still open after idle pumps") + eq(StatGrowth._page, 1, "StatGrowth still on Page 1 after idle pumps") + -- Advance to Page 2 local fakeInput = { wasPressed = function(self, key) return key == "a" end, @@ -576,9 +587,29 @@ do check(StatGrowth.isOpen(), "StatGrowth still open on Page 2") eq(StatGrowth._page, 2, "StatGrowth on Page 2 (new values)") + -- #2324: Page 2 is still a wait + for _ = 1, 20 do + eq(ExpSeq.update(), false, "ExpSeq.update() reports busy on stat window Page 2") + end + eq(ExpSeq._i, stepBefore, "ExpSeq still did not advance on Page 2") + check(StatGrowth.isOpen(), "StatGrowth still open after idle pumps on Page 2") + eq(#messages, msgsBefore, "still no battle text while Page 2 is up") + -- Confirm Page 2 -> closes window and advances sequence StatGrowth.handleInput(fakeInput) check(not StatGrowth.isOpen(), "StatGrowth closed after confirmation") + eq(ExpSeq._i, stepBefore + 1, "ExpSeq advanced only once the window closed") + + -- #2324: a window whose phase can no longer dismiss it must be torn down. + local savedActive, savedPhase, savedHeadless = Battle._active, Battle._phase, Battle._headless + Battle._active, Battle._phase, Battle._headless = true, nil, true + StatGrowth.open(mon, res.steps[1].oldStats, res.steps[1].newStats, function() + error("[FAIL] a torn-down stat window must not fire its onDone callback") + end) + check(StatGrowth.isOpen(), "stale stat window open before Battle.update") + Battle.update(1 / 60, { input = fakeInput }) + check(not StatGrowth.isOpen(), "Battle.update closed a stat window outside its phases") + Battle._active, Battle._phase, Battle._headless = savedActive, savedPhase, savedHeadless end print("\nALL BATTLE SWITCH & FAINT TESTS PASSED! (100%)")