Merge pull request #1086 from thibautbus/fix/yellow-pallet-oak-music

Bring Oak's Pallet Town cutscene in line with the original Yellow game

data/scripts/story2.lua

@@ -206,6 +206,13 @@ M.PALLET_TOWN = {
     end
 
     local function escortToLab(oak)
+      -- PalletMovementScript_OakMoveLeft
+      -- (engine/overworld/auto_movement.asm) starts MUSIC_MUSEUM_GUY
+      -- when the escort begins in Yellow. Until then, Pallet Town plays
+      -- after the battle; Red/Blue leave MUSIC_MEET_PROF_OAK playing.
+      if yellow then
+        Music.play(game.data, "Music_MuseumGuy")
+      end
       local numSteps = x - 10
       if oak and numSteps > 0 then
         ow:scriptMove(oak, "left", numSteps, function()
@@ -249,14 +256,24 @@ M.PALLET_TOWN = {
         function()
           -- Oak turns toward the horizontally adjacent grass (left exit
           -- looks right, right exit looks left -- the
-          -- EVENT_PLAYER_AT_RIGHT_EXIT_TO_PALLET_TOWN branch)
+          -- EVENT_PLAYER_AT_RIGHT_EXIT_TO_PALLET_TOWN branch).
+          -- In pokeyellow, PalletTownOakGreetsPlayerScript turns Oak and
+          -- PalletTownPikachuBattleScript arms the battle on the next
+          -- overworld iteration. OverworldLoopLessDelay
+          -- (home/overworld.asm) burns two DelayFrame calls at the top
+          -- of each iteration and calls RunMapScript before checking
+          -- wCurOpponent, so those two DelayFrame calls are what keep
+          -- Oak's turn on screen before the battle check fires.
           if oak then oak.facing = x == 10 and "right" or "left" end
-          local battle = BattleState.newWild(game, "PIKACHU", 5)
-          battle:makeOldManDemo("PROF.OAK")
-          battle.onFinish = function()
-            afterPikaBattle()
-          end
-          game.stack:push(battle)
+          hold(2, nil, function()
+            local battle = BattleState.newWild(game, "PIKACHU", 5)
+            battle:makeOldManDemo("PROF.OAK")
+            battle.onFinish = function()
+              afterPikaBattle()
+            end
+            -- Use the standard wild-battle entry transition.
+            Commands.pushBattle(ctx, battle)
+          end)
         end))
     end
 

src/battle/BattleState.lua

@@ -2203,8 +2203,13 @@ function BattleState:openOldManBag()
   self.afterQueue = "menu"
   self:ui(function()
     local list
+    -- The canned bag (POKE_BALL, not read from the player's real
+    -- inventory) differs by version: pokered's OldManItemList has 50
+    -- POKé BALLs; pokeyellow's SimulatedInputBattleItemList, shared by
+    -- the Viridian tutorial and Oak's catch, has one.
+    local qty = require("src.core.GameVersion").isYellow() and "x1" or "x50"
     list = ListMenu.new(game, "ITEMS", {
-      { value = "POKE_BALL", label = Strings("POKé BALL"), right = "x50" },
+      { value = "POKE_BALL", label = Strings("POKé BALL"), right = qty },
     }, {
       script = function(l)
         l.scriptTimer = (l.scriptTimer or 0) + 1

src/script/Commands.lua

@@ -280,6 +280,22 @@ function Commands.save_end_battle_text(ctx, textId)
   ctx.endBattleText = TextBox.substitute(ctx.game, text or textId)
 end
 
+-- Route scripted battles through the standard entry transition. In the
+-- originals, InitWildBattle (engine/battle/init_battle.asm) always calls
+-- DoBattleTransitionAndInitBattleVariables (engine/battle/core.asm), with
+-- no old-man or Pikachu-demo exception; BattleTransition then selects the
+-- wipe for the battle kind. Some tests provide only a partial overworld
+-- double, so retain a logged fallback even though it skips the transition
+-- and battle music.
+function Commands.pushBattle(ctx, battle)
+  if ctx.overworld and ctx.overworld.pushBattle then
+    ctx.overworld:pushBattle(battle)
+  else
+    Logger.warn("pushBattle: no overworld:pushBattle, skipping the transition wipe")
+    ctx.game.stack:push(battle)
+  end
+end
+
 -- start_battle "wild" species level | start_battle "trainer" OPP_CLASS partyIndex
 function Commands.start_battle(ctx, kind, a, b)
   local BattleState = require("src.battle.BattleState")
@@ -312,19 +328,8 @@ function Commands.start_battle(ctx, kind, a, b)
     end
     runner:resume()
   end
-  -- Every battle enters through the transition wipe, script-driven ones
-  -- included: BattleTransition (engine/battle/battle_transitions.asm:1) runs
-  -- from DoBattleTransitionAndInitBattleVariables for all of them, and
-  -- GetBattleTransitionID_WildOrTrainer picks the style from the battle kind.
-  -- Pushing the BattleState straight onto the stack skipped the wipe
-  -- entirely, so every scripted trainer -- gym leaders, the rival, Giovanni --
-  -- and every scripted wild battle simply cut to the battle screen.  The
-  -- trainer-sight path already went through pushBattle; this one did not.
-  if ctx.overworld and ctx.overworld.pushBattle then
-    ctx.overworld:pushBattle(battle)
-  else
-    ctx.game.stack:push(battle)
-  end
+  -- A direct stack push would skip the battle-entry transition.
+  Commands.pushBattle(ctx, battle)
   runner:yield()
 end
 
@@ -818,15 +823,7 @@ function Commands.old_man_demo(ctx, outcome)
   local battle = BattleState.newWild(ctx.game, om.species, om.level)
   battle:makeOldManDemo(nil, outcome == "fail")
   battle.onFinish = function() runner:resume() end
-  -- InitWildBattle calls DoBattleTransitionAndInitBattleVariables
-  -- unconditionally (core.asm:6699) -- there is no BATTLE_TYPE_OLD_MAN
-  -- special case -- so the catch tutorial gets the wipe like any other
-  -- wild battle
-  if ctx.overworld and ctx.overworld.pushBattle then
-    ctx.overworld:pushBattle(battle)
-  else
-    ctx.game.stack:push(battle)
-  end
+  Commands.pushBattle(ctx, battle)
   runner:yield()
 end
 

src/world/OverworldController.lua

@@ -453,9 +453,10 @@ function OverworldState:setMap(mapId, x, y, facing, opts)
   -- walks out of the warp, not beside him (#863)
   require("src.world.PikachuFollower").onMapEntered(Game, self, opts, true)
 
-  -- opts.keepMusic: the Oak-escort warp keeps MUSIC_MEET_PROF_OAK
-  -- playing into the lab (BIT_NO_MAP_MUSIC in wStatusFlags7);
-  -- keepMusicOnce is the play_music opts.keep one-shot of the same bit
+  -- opts.keepMusic preserves the Oak-escort song across the lab warp,
+  -- matching BIT_NO_MAP_MUSIC: MUSIC_MUSEUM_GUY in Yellow and
+  -- MUSIC_MEET_PROF_OAK in Red/Blue. keepMusicOnce is the equivalent
+  -- one-shot set by play_music opts.keep.
   local keepMusic = (opts and opts.keepMusic) or self.keepMusicOnce
   self.keepMusicOnce = nil
   if not keepMusic then

tests/engine/push_battle_transition.lua

@@ -0,0 +1,76 @@
+-- Commands.pushBattle (src/script/Commands.lua): the shared entry point
+-- for start_battle, old_man_demo, and the PALLET_TOWN Pikachu catch
+-- (data/scripts/story2.lua). Every one of those call sites used to carry
+-- its own copy of "if ctx.overworld.pushBattle then ... else
+-- ctx.game.stack:push(battle) end"; this locks the dedup in so a future
+-- edit to one call site can't silently drop the transition wipe for the
+-- others.
+--   luajit tests/engine/push_battle_transition.lua
+
+package.path = "./?.lua;./?/init.lua;" .. package.path
+love = love or require("tests.love_stub")
+
+local T = require("tests.harness")
+local check = T.check
+local eq = T.eq
+
+local Commands = require("src.script.Commands")
+local Logger = require("src.core.Logger")
+
+local battle = { id = "the battle" }
+
+-- Runs fn() with Logger.warn spied instead of hitting the real
+-- print()/Logger.history ring buffer, and returns the last formatted
+-- warning (or nil if none). pcall-wrapped so an error inside fn() still
+-- restores Logger.warn before propagating -- a leaked spy would swallow
+-- every later warning in the same process silently.
+local function withWarnSpy(fn)
+  local warned
+  local origWarn = Logger.warn
+  Logger.warn = function(fmt, ...) warned = string.format(fmt, ...) end
+  local ok, err = pcall(fn)
+  Logger.warn = origWarn
+  if not ok then error(err, 0) end
+  return warned
+end
+
+-- ctx.overworld has a real pushBattle: it must be used, not a bare stack
+-- push, so the flash/wipe transition and the battle-theme start survive.
+do
+  local pushed
+  local ow = { pushBattle = function(self, b) pushed = b end }
+  local stackPushed
+  local ctx = { overworld = ow, game = { stack = {
+    push = function(_, b) stackPushed = b end } } }
+  Commands.pushBattle(ctx, battle)
+  eq(pushed, battle, "ctx.overworld:pushBattle is called with the battle")
+  check(stackPushed == nil, "the bare stack push is not also taken")
+end
+
+-- ctx.overworld without a pushBattle method (a partial test double, per
+-- BattleState:finish's "no live children" contract) falls back to a
+-- bare stack push and logs, rather than silently skipping the transition.
+do
+  local stackPushed
+  local ctx = { overworld = {}, game = { stack = {
+    push = function(_, b) stackPushed = b end } } }
+  local warned = withWarnSpy(function() Commands.pushBattle(ctx, battle) end)
+  eq(stackPushed, battle, "falls back to ctx.game.stack:push")
+  check(warned ~= nil, "the fallback logs a warning")
+  check(warned and warned:find("pushBattle") ~= nil,
+        "the warning names pushBattle")
+end
+
+-- ctx.overworld absent entirely (headless script tests that never set
+-- one up): same fallback, no crash on the ctx.overworld.pushBattle read
+-- -- and still spied, since this also takes the warning path.
+do
+  local stackPushed
+  local ctx = { game = { stack = {
+    push = function(_, b) stackPushed = b end } } }
+  local warned = withWarnSpy(function() Commands.pushBattle(ctx, battle) end)
+  eq(stackPushed, battle, "falls back to ctx.game.stack:push with no overworld")
+  check(warned ~= nil, "this fallback also logs a warning")
+end
+
+T.finish("push_battle_transition")

tests/parity_J.lua

@@ -452,4 +452,50 @@ do
         "the caught Weedle is NOT added to the dex")
 end
 
+-- (5b) Yellow's SimulatedInputBattleItemList (core.asm:2316-2319) drops
+-- the same canned bag to a single POKé BALL, x1 -- pokered's
+-- OldManItemList (core.asm:2212-2214, checked above) stays x50.  Only
+-- the item count changes; the scripted no-input menu flow is identical
+-- and already covered above, so this jumps straight to the bag.
+do
+  local GameVersion = require("src.core.GameVersion")
+  local oldVersion = GameVersion.get()
+  GameVersion.set("yellow")
+  local ok, err = pcall(function()
+    local pressed = {}
+    local stack = { states = {} }
+    function stack:push(s) table.insert(self.states, s) end
+    function stack:pop() return table.remove(self.states) end
+    function stack:top() return self.states[#self.states] end
+    local fg = {
+      data = Data,
+      save = require("src.core.SaveData").newGame(),
+      input = { wasPressed = function(_, k) return pressed[k] or false end,
+                isDown = function(_, k) return pressed[k] or false end },
+      stack = stack,
+    }
+    fg.save.party = { Pokemon.new(Data, "BULBASAUR", 20) }
+    local demo = BattleState.newWild(fg, "PIKACHU", 5)
+    demo:makeOldManDemo("PROF.OAK")
+    stack:push(demo)
+    demo:enter()
+    for _ = 1, 300 do
+      if demo.phase == "menu" then break end
+      pressed.a = true
+      demo:update(1 / 60)
+    end
+    pressed.a = false
+    eq(demo.phase, "menu", "Yellow: the demo reaches the battle menu")
+    for _ = 1, 200 do
+      if stack:top() ~= demo then break end
+      demo:update(1 / 60)
+    end
+    local bag = stack:top()
+    eq(bag.items and bag.items[1] and bag.items[1].right, "x1",
+       "Yellow's old-man-style bag lists x1 (SimulatedInputBattleItemList)")
+  end)
+  GameVersion.set(oldVersion)
+  if not ok then error(err, 0) end
+end
+
 S.finish()

tests/parity_yellow_pallet_pikachu.lua

@@ -0,0 +1,201 @@
+-- Parity test for Yellow's Pallet Town intro (Oak stops the player,
+-- catches a wild Pikachu, walks them to the lab). Drives the real
+-- onStep closure in data/scripts/story2.lua through the real
+-- StateStack/OverworldState, with no mocked battle or overworld, up
+-- through the point Oak's Pikachu catch is armed: covers two of the
+-- four fixes on this branch -- Music_MuseumGuy staying off in
+-- Red/Blue, and the 2-frame hold before the Pikachu battle handing
+-- off to BattleTransition rather than a bare push.
+--
+-- Deliberately stops there rather than also driving the demo battle
+-- to completion to assert Music_MuseumGuy fires in Yellow (the third
+-- fix): that would mean re-deriving frame budgets for the battle
+-- menu/bag/throw sequence tests/parity_J.lua already exercises, on
+-- top of everything already driven here, for one more assertion --
+-- more coupling to unrelated timing than the fix is worth. That side
+-- is manually verified instead (see the PR description).
+--
+-- Sources: scripts/PalletTown.asm, engine/overworld/auto_movement.asm,
+-- home/overworld.asm, engine/battle/core.asm (see the commits on this
+-- branch for the exact citations).
+--   luajit tests/parity_yellow_pallet_pikachu.lua
+package.path = "./?.lua;./?/init.lua;" .. package.path
+if not _G.love then _G.love = require("tests.love_stub") end
+
+local Data = require("src.core.Data")
+if not (Data.maps and Data.maps.PALLET_TOWN) then Data:load() end
+
+local S = require("tests.harness").suite("parity Yellow Pallet Town Pikachu")
+local check, eq = S.check, S.eq
+
+local GameVersion = require("src.core.GameVersion")
+local SaveData = require("src.core.SaveData")
+local Game = require("src.core.Game")
+local StateStack = require("src.core.StateStack")
+local OverworldState = require("src.world.OverworldController")
+local BattleTransition = require("src.render.BattleTransition")
+local TextBox = require("src.render.TextBox")
+local Music = require("src.core.Music")
+local mapScripts = require("data.scripts.init")
+
+local pallet = mapScripts.get("PALLET_TOWN")
+check(pallet and pallet.onStep, "PALLET_TOWN exposes onStep")
+
+local oldVersion = GameVersion.get()
+local prevGame = { data = Game.data, save = Game.save, stack = Game.stack,
+                   input = Game.input, renderer = Game.renderer,
+                   overworld = Game.overworld }
+
+local function freshGame(mapX, mapY)
+  Game.data = Data
+  Game.save = SaveData.newGame(Data)
+  Game.save.player.name = "RED"
+  StateStack:init()
+  Game.stack = StateStack
+  local pressed = {}
+  Game.input = {
+    isDown = function() return false end,
+    wasPressed = function(_, b) return pressed[b] or false end,
+    step = function() end, state = {}, pressQueue = {},
+  }
+  Game.renderer = {
+    beginWorldPass = function() end, endWorldPass = function() end,
+    beginUIPass = function() end, endUIPass = function() end,
+    worldViewSize = function() return 160, 144 end,
+    setSGBZones = function() end,
+  }
+  StateStack:push(OverworldState, "PALLET_TOWN", mapX, mapY, "up")
+  Game.overworld = OverworldState
+  return pressed
+end
+
+-- mash "a" whenever anything but the overworld is on top (dismisses
+-- text boxes; scriptMove/hold/BattleTransition/BattleState's own
+-- scripted-menu phases all ignore it)
+local function pump(pressed, n)
+  for _ = 1, n do
+    pressed.a = Game.stack:top() ~= OverworldState
+    Game.stack:update(1 / 60)
+  end
+end
+
+local function mashUntil(pressed, cond, cap)
+  for _ = 1, cap do
+    if cond() then return true end
+    pressed.a = Game.stack:top() ~= OverworldState
+    Game.stack:update(1 / 60)
+  end
+  return false
+end
+
+-- Every scenario mutates the Game/GameVersion/StateStack singletons the
+-- rest of the aggregate tests/run_tests.lua run depends on; a genuine
+-- error partway through (plausible -- this drives real scriptMove,
+-- pathfinding, TextBox paging and BattleState code, not test doubles)
+-- must not skip the restore, or it cascades into every later suite in
+-- the same process. StateStack:init() leaves it empty rather than
+-- pointed at this scenario's pushed OverworldState/TextBox/
+-- BattleTransition instances.
+local function scenario(fn)
+  local ok, err = pcall(fn)
+  GameVersion.set(oldVersion)
+  for k, v in pairs(prevGame) do Game[k] = v end
+  StateStack:init()
+  if not ok then error(err, 0) end
+end
+
+-- =====================================================================
+-- (A) Red/Blue: MUSIC_MUSEUM_GUY must never play. Only "red" is driven
+-- here -- story2.lua's onStep branches on GameVersion.isYellow() alone
+-- and never calls isBlue(), so Red and Blue share this exact code path
+-- and a separate Blue run would exercise nothing new. pokered's copy of
+-- PalletMovementScript_OakMoveLeft only sets BIT_NO_MAP_MUSIC and never
+-- calls PlayMusic -- MUSIC_MEET_PROF_OAK (started when Oak appears)
+-- keeps running straight into the lab.
+-- =====================================================================
+scenario(function()
+  GameVersion.set("red")
+  local pressed = freshGame(8, 2)
+  local played = {}
+  local origPlay = Music.play
+  Music.play = function(data, song, ...)
+    table.insert(played, song)
+    return origPlay(data, song, ...)
+  end
+  local ok, err = pcall(function()
+    local ow = OverworldState
+    check(pallet.onStep(Game, ow, 8, 1) == true,
+          "Red: onStep claims the trigger tile")
+    -- HeyWaitDontGoOutText (auto) -> "!" bubble hold(50) -> Oak
+    -- approaches -> "It's unsafe!" text -> escortToLab -> the walk to
+    -- the lab door. 3000 frames (50s of game time) covers it; the
+    -- negative Music_MuseumGuy checks below only mean something once
+    -- the escort has actually finished, not merely started.
+    pump(pressed, 3000)
+    check(Game.save.flags.EVENT_FOLLOWED_OAK_INTO_LAB == true,
+          "Red: the escort actually reaches the lab within the frame budget")
+  end)
+  Music.play = origPlay
+  if not ok then error(err, 0) end
+  local sawMuseumGuy = false
+  for _, s in ipairs(played) do
+    if s == "Music_MuseumGuy" then sawMuseumGuy = true end
+  end
+  check(not sawMuseumGuy, "Red/Blue: Music_MuseumGuy never plays for this escort")
+  check(played[1] == "Music_MeetProfOak",
+        "Red/Blue: Music_MeetProfOak is still the only cutscene cue played")
+end)
+
+-- =====================================================================
+-- (B) Yellow: the turn-then-battle timing and the transition wipe.
+-- =====================================================================
+scenario(function()
+  GameVersion.set("yellow")
+  local pressed = freshGame(10, 1)
+  local ok, err = pcall(function()
+    local ow = OverworldState
+    check(pallet.onStep(Game, ow, 10, 0) == true,
+          "Yellow: onStep claims the north-exit tile")
+    local heyWaitBox = Game.stack:top()
+    check(getmetatable(heyWaitBox) == TextBox,
+          "Yellow: onStep opens with a text box (HeyWaitDontGoOutText)")
+
+    -- HeyWaitDontGoOutText (auto, ~20 frames) -> "!" bubble hold(50) ->
+    -- Oak approaches (findPath(10,4,10,1): 3 steps) -> hold(6) -> the
+    -- next real text box is ThatWasClose (button-dismissed, unlike the
+    -- first).
+    local sawThatWasClose = mashUntil(pressed, function()
+      local top = Game.stack:top()
+      return top ~= heyWaitBox and getmetatable(top) == TextBox
+    end, 3000)
+    check(sawThatWasClose, "Yellow: reaches the ThatWasClose text")
+
+    local oak
+    for _, n in ipairs(ow.npcs) do
+      if n.def and n.def.name == "PALLETTOWN_OAK" then oak = n end
+    end
+    check(oak ~= nil, "Yellow: PALLETTOWN_OAK is spawned")
+
+    -- dismiss ThatWasClose (a multi-page text: mash through the
+    -- typewriter cadence and the page turn); oak.facing flips
+    -- synchronously in the close callback (x == 10 -> "right"), then
+    -- hold(2) starts
+    local sawHold = mashUntil(pressed, function() return ow.emote ~= nil end, 2000)
+    check(sawHold, "Yellow: ThatWasClose closes and the hold before battle arms")
+    eq(oak and oak.facing, "right", "Oak turns to face the grass (x==10 -> right)")
+    check(ow.emote ~= nil and ow.emote.frames == 2,
+          "the hold before the battle is armed for exactly 2 frames")
+
+    -- tick 1: 2 -> 1, battle not pushed yet
+    Game.stack:update(1 / 60)
+    check(getmetatable(Game.stack:top()) ~= BattleTransition,
+          "1 frame in: the battle has not started yet")
+    -- tick 2: 1 -> 0, the demo battle is pushed through the transition
+    Game.stack:update(1 / 60)
+    check(getmetatable(Game.stack:top()) == BattleTransition,
+          "2 frames in: Oak's catch enters through BattleTransition, not a bare push")
+  end)
+  if not ok then error(err, 0) end
+end)
+
+S.finish()