Merge pull request #1434 from sanjinpepic/upstream-fixes

src/battle/BattleState.lua

@@ -1103,6 +1103,15 @@ function BattleState:stepHPDrain()
       if not b.shownPx then b.shownPx = targetPx end
       if (b.drainHold or 0) > 0 then
         b.drainHold = b.drainHold - 1
+        -- Once the count runs out with nothing left pending (bar and
+        -- number already on the final total), the drain is over, not just
+        -- between steps: leave the field at 0 and BattleSafety.inspect
+        -- reads it as still mid-animation for the rest of the battle,
+        -- since drainHold ~= nil is its settled-presentation gate.
+        if b.drainHold <= 0 and b.shownPx == targetPx and b.shownHP == goal
+            and not b.draining then
+          b.drainHold = nil
+        end
         busy = true
       elseif b.shownPx ~= targetPx then
         -- .barAnimationLoop redraws the bar one pixel at a time, `ld c, 2 /

src/battle/gen2/Mon.lua

@@ -112,7 +112,13 @@ function Mon.syncIdentity(mon, data)
   if mon.dvs then
     mon.gender = Mon.gender(def, mon.dvs,
       { species = mon.species, level = mon.level })
-    mon.shiny = Mon.isShiny(mon.dvs,
+    -- shiny is monotonic once true, the same as opts.shiny winning over
+    -- shiny.roll at Mon.new: a forced shiny (the scripted-shiny path, DVs
+    -- that do not themselves read as shiny) must not un-shiny the moment
+    -- this runs again, and it runs on every SummaryMenu open via
+    -- refreshStats.  A mon not already shiny still promotes normally if
+    -- its DVs justify it, e.g. after an edit.
+    mon.shiny = mon.shiny or Mon.isShiny(mon.dvs,
       { species = mon.species, def = def, level = mon.level })
     if mon.species == Unown.SPECIES then
       mon.unownLetter = Unown.letterFromDVs(mon.dvs)

src/core/Game2.lua

@@ -683,7 +683,10 @@ end
 -- .SelectMon / PPRestoreItem_Cancel carry path: nothing spent.
 function Game2:usePartyItem(itemId)
   local ItemEffects = require("src.core.gen2.ItemEffects")
-  local action = ItemEffects.partyAction(itemId)
+  -- without the merged dataset this can only ever see RECORDS, the
+  -- module's own built-ins, so a mod's field item resolves to no action
+  -- at all and never gets past the .Oak refusal below
+  local action = ItemEffects.partyAction(itemId, self.data)
   if not action then return end
   local party = (self.save and self.save.party) or {}
   if #party == 0 then

src/inventory/ItemEffects.lua

@@ -515,7 +515,10 @@ function ItemEffects.use(data, save, itemId, target, battle, moveIndex, ow)
     if not target then return "failed", { noEffect(data) } end
     local speciesDef = data.pokemon[target.species]
     local ok = false
-    for _, m in ipairs(speciesDef.tmhm) do
+    -- a species record with no tmhm list at all is "teaches nothing", the
+    -- same as one whose list just does not name this move -- not a reason
+    -- to crash instead of refusing normally
+    for _, m in ipairs(speciesDef.tmhm or {}) do
       if m == itemDef.machine.move then ok = true break end
     end
     if not ok then

src/mods/Sandbox.lua

@@ -28,8 +28,14 @@ local DENIED = {
 }
 
 -- Same idea one level up: love.filesystem is reachable by name, and
--- love.thread starts a Lua state this sandbox has no say over.
-local DENIED_PREFIX = { ["love"] = true, ["ffi"] = true }
+-- love.thread starts a Lua state this sandbox has no say over.  jit.util
+-- is the LuaJIT-specific equal of the debug library above -- funcbc,
+-- funck and friends read the bytecode and constants of any function a
+-- chunk can reach, which is enough to walk back to upvalues (the real
+-- _G, love, io) the rest of this file exists to keep out of reach.  The
+-- bare `jit` table stays -- env.jit above hands it over directly for
+-- jit.on/off/flush -- so only the submodule require is denied.
+local DENIED_PREFIX = { ["love"] = true, ["ffi"] = true, ["jit"] = true }
 
 -- The wire, which is what the network permission governs.
 local NETWORK = { socket = true, enet = true, http = true, https = true,

src/mods/Schemas.lua

@@ -91,6 +91,28 @@ function f.rec(fields, opts)
     desc = "{" .. table.concat(parts, ", ") .. "}" }
 end
 
+-- An open record: the listed fields are typed (including f.id
+-- cross-references) and everything else on the value passes through
+-- unexamined, unlike f.rec's nested shapes, which reject any key they do
+-- not name.  Map objects are why this exists -- NPCs, signs, items, warps
+-- and static encounters all share one array, and only a full union of
+-- every kind's shape could describe it as f.rec; that is a lot of surface
+-- to keep in sync with the loader for fields nothing here needs to check.
+-- f.partial types just the field that actually names another registry and
+-- leaves every kind-specific field around it alone.
+function f.partial(fields)
+  local names = {}
+  for name in pairs(fields) do names[#names + 1] = name end
+  table.sort(names)
+  local parts = {}
+  for _, name in ipairs(names) do
+    local ft = fields[name]
+    parts[#parts + 1] = name .. (ft.kind == "opt" and "?" or "")
+  end
+  return { kind = "partial", fields = fields,
+    desc = "{" .. table.concat(parts, ", ") .. ", ...}" }
+end
+
 function f.union(alts)
   local parts = {}
   for _, alt in ipairs(alts) do parts[#parts + 1] = alt.desc end
@@ -197,6 +219,23 @@ checkValue = function(t, value, path, patchMode, errors, top)
     end
     return
   end
+  if kind == "partial" then
+    -- the open counterpart of "rec": listed fields are checked exactly
+    -- like a rec's, and any key not listed is left alone rather than
+    -- flagged, so a heterogeneous blob (map objects) can have one field
+    -- typed without every other shape sharing the array being rejected
+    if type(value) ~= "table" then return fail(errors, path, t.desc, value) end
+    for key, ft in pairs(t.fields) do
+      local sub = value[key]
+      if sub ~= nil then
+        checkValue(ft, sub, path .. "." .. tostring(key), patchMode, errors)
+      elseif ft.kind ~= "opt" and not patchMode then
+        errors[#errors + 1] = ("%s.%s: missing required field (%s)")
+          :format(path, key, ft.desc)
+      end
+    end
+    return
+  end
   if kind == "union" then
     for _, alt in ipairs(t.alts) do
       local scratch = {}
@@ -299,7 +338,7 @@ collectRefs = function(t, value, path, out)
     for k, v in pairs(value) do
       collectRefs(t.value, v, path .. "." .. tostring(k), out)
     end
-  elseif kind == "rec" and type(value) == "table" then
+  elseif (kind == "rec" or kind == "partial") and type(value) == "table" then
     for key, ft in pairs(t.fields) do
       collectRefs(ft, value[key], path .. "." .. tostring(key), out)
     end
@@ -377,11 +416,19 @@ function Schemas.crossValidate(loader, data)
             and loader.content[ref.registry]
           -- A registry with no home in this generation has no id space to
           -- check against: its base view resolves to nothing, so EVERY
-          -- reference into it would read as dangling.  Gold's species carry a
-          -- growthRate and an evolution method like Red's do; the ids are
-          -- fine, it is the Gen 1 `growth_rates` / `evolution_methods`
-          -- namespaces that are not there to confirm them.  Skipped for the
-          -- same reason an undeclared registry is: unknown, not wrong.
+          -- reference into it would read as dangling.  `transitions` is the
+          -- standing example -- Gold draws its own battle intro and never
+          -- reads the merged table, so a mod's transition id there is
+          -- unconfirmable, not wrong.  `growth_rates` and `evolution_methods`
+          -- used to sit in that category too, back when Gold had no id space
+          -- for either.  Both are routed now: `growth_rates` keeps its Gen 1
+          -- path and is seeded from data.pokemon.growthRates (the extractor's
+          -- Gold curves, src/battle/gen2/Mon.lua), and `evolution_methods`
+          -- routes to gen2EvolutionMethods (src/core/gen2/Evolution.lua's
+          -- literal EVOLVE_* ids, present with or without a ROM import).  A
+          -- Gold species' growthRate or evolution method is checked against
+          -- real ids exactly like a Red one's, so a genuine typo is still
+          -- caught here rather than waved through as "unknown, not wrong."
           if refRegistry and Schemas.gatedFor(ref.registry, loader.generation) then
             refRegistry = nil
           end
@@ -887,7 +934,15 @@ R.maps = {
                                 destMap = f.str, destWarp = f.int(0),
                                 destGroup = f.opt(f.int(0)),
                                 destMapNum = f.opt(f.int(0)) })),
-    objects = f.opt(f.list(f.any)),
+    -- NPCs, signs, items, warps and static wild encounters all share this
+    -- one array, with no field the loader could use to tell them apart
+    -- ahead of time -- an f.rec strict enough to describe every kind would
+    -- reject the others.  f.partial types only `pokemon` (the static
+    -- encounter's species, OverworldController.lua's `d.pokemon` ->
+    -- BattleState.newWild) so a bad id is a load-time error, the same as
+    -- an encounter slot's species, instead of the crash newWild has no
+    -- guard against.  Every other object field passes through untouched.
+    objects = f.opt(f.list(f.partial{ pokemon = f.opt(f.id("pokemon")) })),
     signs = f.opt(f.list(f.any)),
     connections = f.opt(f.map(f.enum{ "north", "south", "east", "west" }, f.any)),
   },

src/ui/BagMenu.lua

@@ -4,6 +4,7 @@
 
 local ItemEffects = require("src.inventory.ItemEffects")
 local ListMenu = require("src.ui.ListMenu")
+local Runtime = require("src.mods.Runtime")
 local TextBox = require("src.render.TextBox")
 
 local BagMenu = {}
@@ -46,7 +47,16 @@ end
 -- the stack, so every exit that prints has to close it afterwards.  For
 -- every other item the picker popped itself first and closePicker's identity
 -- check makes it a no-op (#252).
-local function useOn(game, battle, id, target, list, moveIndex, picker)
+--
+-- Every result string used to fall through to this one unconditional
+-- function with no seam around it: a mod could not suppress a message,
+-- delay it behind a screen of its own, or replace the outcome for one item
+-- id.  The "item.use" hook wraps the whole dispatch (not a name per
+-- result -- a mod deciding what a Poké Doll or a stone does needs the
+-- SAME reach a vanilla `if result == ...` branch has, not a narrower one),
+-- the way "battle.overlay" and "ui.party.submenu" already wrap a
+-- screen's own default behavior elsewhere in src/ui.
+local function vanillaUseOn(game, battle, id, target, list, moveIndex, picker)
   local result, payload, extra = ItemEffects.use(game.data, game.save, id, target,
                                                  battle, moveIndex, game.overworld)
   local function closePicker()
@@ -375,6 +385,11 @@ local function useOn(game, battle, id, target, list, moveIndex, picker)
   showMessages(game, payload, closePicker) -- failed
 end
 
+local function useOn(game, battle, id, target, list, moveIndex, picker)
+  return Runtime.call("item.use", vanillaUseOn,
+    game, battle, id, target, list, moveIndex, picker)
+end
+
 local function pickTargetAndUse(game, battle, id, list)
   -- pick a target from the party
   -- the ETHERs and PP UP open the move menu after picking a mon

src/ui/gen2/BattleState.lua

@@ -3024,8 +3024,10 @@ function BattleState:useItem(itemId)
     -- Everything else the pack can spend on a party mon runs the same
     -- item_effects.asm routine the field pack runs: the potion line and the
     -- drinks, the status cures and their berries, REVIVE / MAX REVIVE, and
-    -- the ETHER / ELIXER family.
-    local action = ItemEffects.partyAction(itemId)
+    -- the ETHER / ELIXER family.  Without the merged dataset this can only
+    -- ever see RECORDS, the module's own built-ins, the same gap
+    -- Game2:usePartyItem had for the field pack.
+    local action = ItemEffects.partyAction(itemId, self.game and self.game.data)
     if action then
       return self:useOnPartyMon(itemId, action)
     end