fixed water not animating in red++

lib/TerrainAtlas.lua

@@ -29,12 +29,32 @@
 
 local Assets = require("src.render.Assets")
 local TileRenderer = require("src.render.TileRenderer")
+local PaletteFX = require("src.render.PaletteFX")
 
 local TerrainAtlas = {}
 
 local cache = {}
 local cacheData = {}    -- the pixels behind the atlases we baked ourselves
 local animated = {}     -- key -> one map's private, mutable animated atlas
+                        -- false = given up on; nil = not built (or retrying)
+local attempts = {}     -- key -> consecutive failures, for the retry budget
+
+-- A failure that might not repeat -- a driver refusing one readback, an
+-- asset briefly unreadable, a patch that threw once mid-reload -- must not
+-- cost the animation for the rest of the session. It used to: the key was
+-- condemned to `false` on the first miss and nothing ever rebuilt it, so
+-- water stopped moving and stayed stopped until a hot reload.
+--
+-- Retry a few times, then give up for good so a genuinely broken atlas is
+-- not rebuilt on every frame forever.
+local MAX_ATTEMPTS = 3
+
+local function attemptFailed(key)
+  local n = (attempts[key] or 0) + 1
+  attempts[key] = n
+  if n >= MAX_ATTEMPTS then return false end   -- condemn it
+  return nil                                    -- rebuild next frame
+end
 
 local function paletteKey(colors)
   local parts = {}
@@ -207,6 +227,9 @@ local function readback(image)
     love.graphics.setColor(1, 1, 1, 1)
     love.graphics.draw(image, 0, 0)
     love.graphics.setBlendMode("alpha", "alphamultiply")
+    -- LOVE refuses newImageData on the currently-active canvas, so the
+    -- previous target has to come back BEFORE the read, not just after
+    love.graphics.setCanvas(prev)
     local out = canvas:newImageData()
     if canvas.release then canvas:release() end
     return out
@@ -215,6 +238,72 @@ local function readback(image)
   return ok and data or nil
 end
 
+-- RED++'s per-map atlas, rebuilt on the CPU.
+--
+-- This is the case that has no pixels anywhere: `getGbcAtlas` bakes one
+-- ImageData per map, hands the texture to the renderer and drops the
+-- pixels on the floor. Without them the animated tiles cannot be patched,
+-- which is why water and flowers stood still under RED++ and nowhere else.
+--
+-- The readback below can recover them from the texture, but it is at the
+-- mercy of whether the driver will read a canvas back, and it costs a GPU
+-- sync mid-frame. Everything the engine baked FROM is public, so bake it
+-- again instead: the raw art, the per-tile palette group, the group's
+-- colours, and the same recolorSample cutoffs. Deterministic, no driver
+-- involved, and it can be tested without a GPU.
+--
+-- It does mirror engine logic and could drift from getGbcAtlas if that
+-- changes -- the readback stays behind it as the exact-but-fragile route.
+local function gbcPixels(map)
+  local renderer, tileset = map.renderer, map.tileset
+  local data = renderer and renderer.data
+  if not (data and tileset and love.image and love.image.newImageData) then
+    return nil
+  end
+  local ok, out = pcall(function()
+    local groupColors = PaletteFX.worldGroupColors(data, tileset.id, map.id, nil)
+    if not groupColors then return nil end
+    local src = Assets.imageData(tileset.image)
+    local iw, ih = src:getDimensions()
+    local perRow = tileset.tilesPerRow or 16
+    local total = (iw / 8) * (ih / 8)
+    local dst = love.image.newImageData(iw, ih)
+
+    local function bake(from, to, colors)
+      local sxo, syo = (from % perRow) * 8, math.floor(from / perRow) * 8
+      local dxo, dyo = (to % perRow) * 8, math.floor(to / perRow) * 8
+      for py = 0, 7 do
+        for px = 0, 7 do
+          local r, g, b, a = src:getPixel(sxo + px, syo + py)
+          r, g, b, a = TileRenderer.recolorSample(r, g, b, a, colors)
+          dst:setPixel(dxo + px, dyo + py, r, g, b, a)
+        end
+      end
+    end
+
+    local tileColors = {}
+    for t = 0, total - 1 do
+      local colors = tileColors[t]
+      if colors == nil then
+        local group = PaletteFX.worldGroupAt(tileset.id, map.id, t)
+        colors = (group and groupColors[group + 1]) or false
+        tileColors[t] = colors
+      end
+      bake(t, t, colors)
+    end
+    -- duplicate-tile aliases: the same graphic baked into a spare slot
+    -- under a second palette group, so cells drawing the alias colour apart
+    for _, al in ipairs(PaletteFX.TILE_ALIASES
+                        and PaletteFX.TILE_ALIASES[map.id] or {}) do
+      if al.alias < total then
+        bake(al.tile, al.alias, groupColors[al.group + 1])
+      end
+    end
+    return dst
+  end)
+  return ok and out or nil
+end
+
 -- The pixels behind the atlas texture the engine is drawing with, for the
 -- frames where we did not bake one ourselves (staticAtlas returns `false`
 -- for its own bake whenever the palette is absent, RED++ already baked, or
@@ -238,7 +327,9 @@ local function rendererPixels(map)
     local ok, data = pcall(TileRenderer.atlasImageData, renderer)
     if ok and data then return data end
   end
-  if renderer.gbcAtlas then return readback(renderer.image) end
+  if renderer.gbcAtlas then
+    return gbcPixels(map) or readback(renderer.image)
+  end
   local ok, data = pcall(Assets.imageData, map.tileset.image)
   return ok and data or nil
 end
@@ -294,18 +385,23 @@ end
 
 TerrainAtlas._animFrame = animFrame   -- named for the suite
 
+-- false = this can never work and asking again is waste; nil = it did not
+-- work THIS time and might next. The caller latches the first and retries
+-- the second (see attemptFailed).
 local function newEntry(map, base, baked)
   local tileset = map.tileset
   local specs = specsFor(tileset)
-  if not specs then return false end
+  if not specs then return false end        -- nothing on this tileset animates
   if not (love.image and love.image.newImageData
           and base.replacePixels) then
-    return false
+    return false                            -- no pixel access on this machine
   end
   -- the pixels the atlas texture was built from: our own SGB bake when we
-  -- made one, else whatever the engine's renderer is drawing with
+  -- made one, else whatever the engine's renderer is drawing with. A
+  -- readback can fail for one frame and work the next, so this is a
+  -- retryable miss rather than a verdict.
   local src = baked or rendererPixels(map)
-  if not src then return false end
+  if not src then return nil end
 
   local ok, entry = pcall(function()
     local w, h = src:getDimensions()
@@ -351,8 +447,14 @@ function TerrainAtlas.animate(map, colors, base, baked)
   local entry = animated[key]
   if entry == nil then
     entry = newEntry(map, base, baked)
-    if entry then entry.mapId = perMap end
-    animated[key] = entry
+    if entry then
+      entry.mapId = perMap
+      animated[key] = entry
+    else
+      -- false from newEntry is a verdict (nothing animates on this tileset,
+      -- no pixel access at all); nil is a miss that may not repeat
+      animated[key] = (entry == false) and false or attemptFailed(key)
+    end
   end
   if not entry then return nil end
 
@@ -375,10 +477,17 @@ function TerrainAtlas.animate(map, colors, base, baked)
       entry.image:replacePixels(entry.data)
     end)
     if not ok then
-      animated[key] = false
+      -- drop the entry rather than condemning the key: the next frame
+      -- rebuilds and tries again, and attemptFailed gives up eventually
+      animated[key] = attemptFailed(key)
       return nil
     end
   end
+  -- Only a frame that got all the way here counts as healthy. Clearing the
+  -- budget on a successful BUILD instead would never let it run out: an
+  -- entry that builds fine and fails on upload would rebuild every frame,
+  -- forever, which is worse than either animating or giving up.
+  if attempts[key] then attempts[key] = nil end
   return entry.image
 end
 
@@ -446,6 +555,7 @@ end
 function TerrainAtlas.invalidate()
   cache = {}
   cacheData = {}
+  attempts = {}
   for _, entry in pairs(animated) do
     if entry and entry.image and entry.image.release then
       pcall(entry.image.release, entry.image)

tests/dramatic_shape_test.lua

@@ -208,11 +208,16 @@ love.image = { newImageData = function(a, b)
   return fakePixels()          -- the "decoded from a path" overload
 end }
 -- animate() only uploads when the animation step actually turns over, so
--- counting replacePixels is how the suite sees the step move from outside
-local patches = 0
+-- counting replacePixels is how the suite sees the step move from outside.
+-- builds counts entries made, and uploadFails forces the upload to throw.
+local patches, builds, uploadFails = 0, 0, false
 love.graphics.newImage = function()
+  builds = builds + 1
   return { setFilter = function() end,
-           replacePixels = function() patches = patches + 1 end }
+           replacePixels = function()
+             if uploadFails then error("transient upload failure", 0) end
+             patches = patches + 1
+           end }
 end
 
 -- a tileset whose water tile rotates, i.e. one specsFor will accept
@@ -281,6 +286,51 @@ T.eq(love.graphics.getCanvas(), passCanvas,
 love.graphics.newCanvas = realNewCanvas
 love.graphics.setCanvas()
 
+-- 3c. RED++ WITH the renderer's data in hand: the atlas is rebuilt on the
+--     CPU from the raw art and the map's palette groups, so water animates
+--     under RED++ without asking the driver for anything. This is the case
+--     that was actually broken on hardware -- RED++ is the only mode where
+--     staticAtlas declines to bake, so it was the only mode whose animated
+--     tiles depended on a readback, and it stood still.
+TerrainAtlas.invalidate()
+local realNewCanvas2 = love.graphics.newCanvas
+love.graphics.newCanvas = function() error("driver refuses canvas readback", 0) end
+local redppMap = animatedMap("REDPP",
+  { image = base, gbcAtlas = true, data = Data })
+redppMap.id = "PALLET_TOWN"          -- a map the palette groups know about
+redppMap.tileset.id = "OVERWORLD"
+local PaletteFX = require("src.render.PaletteFX")
+local modeWas = PaletteFX.mode
+PaletteFX.mode = "redpp"
+local okRedpp, redppImg = pcall(TerrainAtlas.animate, redppMap, nil, base, false)
+T.check(okRedpp and redppImg ~= nil,
+  "RED++ animates from a CPU rebuild, with no readback available at all")
+PaletteFX.mode = modeWas
+love.graphics.newCanvas = realNewCanvas2
+
+-- 3d. A failure that might not repeat must not cost the animation for the
+--     rest of the session. It used to: the key was condemned on the first
+--     miss and nothing rebuilt it, so water stopped and stayed stopped.
+TerrainAtlas.invalidate()
+uploadFails = true
+T.eq(TerrainAtlas.animate(plain, nil, base, false), nil,
+  "a patch that throws declines the frame")
+uploadFails = false
+local okRetry, retryImg = pcall(TerrainAtlas.animate, plain, nil, base, false)
+T.check(okRetry and retryImg ~= nil,
+  "and the next frame rebuilds, rather than staying dead until a hot reload")
+
+-- but a key that keeps failing is given up on, not rebuilt every frame
+TerrainAtlas.invalidate()
+uploadFails = true
+for _ = 1, 6 do TerrainAtlas.animate(plain, nil, base, false) end
+local settledBuilds = builds
+for _ = 1, 6 do TerrainAtlas.animate(plain, nil, base, false) end
+T.eq(builds, settledBuilds,
+  "a key that fails repeatedly is condemned rather than rebuilt forever")
+uploadFails = false
+TerrainAtlas.invalidate()
+
 -- 4. the reported path end to end: cycle every palette mode over a map with
 --    animated tiles. PaletteFX.pal returns nil for a mode with no world
 --    palette, which is the `colors = nil` that flips staticAtlas to no-bake.

tests/voxel_water_probe.lua

@@ -0,0 +1,119 @@
+-- Driver: WHY is the terrain animation not moving?
+--
+-- Water and flowers animate by rewriting their slots in a private copy of
+-- the tileset atlas once per step. That chain has several links and every
+-- one of them fails quietly -- the mode just keeps drawing a still atlas.
+-- This walks the chain in the LIVE game and prints where it stops, for the
+-- palette mode currently active.
+--
+--   POKEPORT_DRIVER=mods/DRAMATIC_SHAPE/tests/voxel_water_probe.lua lovec .
+--
+-- knobs (env):
+--   WATER_MAP    map id                      (default PALLET_TOWN)
+--   WATER_SPOT   "x,y[,facing]"              (default 5,6,down)
+--   WATER_MODE   palette mode to force       (default: leave as-is)
+--   WATER_LEVEL  voxel rung                  (default 3)
+return function(game)
+  local U = dofile("tests/drivers/util.lua")
+  local Pipelines = require("src.render.Pipelines")
+  local TileRenderer = require("src.render.TileRenderer")
+  local PaletteFX = require("src.render.PaletteFX")
+
+  local mapId = os.getenv("WATER_MAP") or "PALLET_TOWN"
+  local level = math.floor(tonumber(os.getenv("WATER_LEVEL")) or 3)
+  local sx, sy, facing = (os.getenv("WATER_SPOT") or "5,6,down")
+    :match("^%s*(%d+)%s*,%s*(%d+)%s*,?%s*(%a*)")
+  facing = (facing ~= "" and facing) or "down"
+
+  local function say(...) print("[water] " .. string.format(...)) end
+
+  if os.getenv("WATER_MODE") then PaletteFX.setMode(os.getenv("WATER_MODE")) end
+
+  U.teleport(game, mapId, tonumber(sx), tonumber(sy), facing)
+  U.wait(20)
+  Pipelines.setLevel("voxel", level)
+  U.wait(30)                       -- outlast the camera tween
+
+  local V = game.mods.exports["DRAMATIC_SHAPE"]
+  V = V and V.lib
+  if not V then return say("mod exports unreachable -- is it enabled?") end
+  local TerrainAtlas = V.require("TerrainAtlas")
+
+  local ow = game.overworld
+  local map = ow and ow.map
+  if not map then return say("no live map") end
+
+  say("mode=%s (%s)", tostring(PaletteFX.mode),
+      tostring(PaletteFX.modeLabel()))
+  say("map=%s tileset=%s animation=%s", tostring(map.id),
+      tostring(map.tileset.id), tostring(map.tileset.animation))
+
+  -- 1. does the engine think this tileset animates at all?
+  local specs = TileRenderer.defaultAnimatedTiles(map.tileset)
+  say("engine animated specs: %d", specs and #specs or 0)
+  if not specs or #specs == 0 then
+    return say("STOP: this tileset declares no animated tiles")
+  end
+
+  -- 2. is the clock advancing? this is the seam the engine does not export
+  local clock = TerrainAtlas._animFrame
+  if not clock then return say("STOP: no clock accessor on TerrainAtlas") end
+  local t0 = clock()
+  U.wait(30)
+  local t1 = clock()
+  say("clock: %s -> %s (%s)", tostring(t0), tostring(t1),
+      (t1 > t0) and "advancing" or "FROZEN")
+  if t1 <= t0 then
+    return say("STOP: the tile clock is not advancing -- animFrame seam lost")
+  end
+
+  -- 3. which pixel source is in play, and is an animated copy being made?
+  local gbc = map.renderer and map.renderer.gbcAtlas
+  say("renderer.gbcAtlas=%s renderer.data=%s",
+      tostring(gbc and true or false),
+      tostring(map.renderer and map.renderer.data ~= nil))
+  if gbc then
+    local groups = PaletteFX.worldGroupColors(map.renderer.data,
+                                              map.tileset.id, map.id, nil)
+    say("worldGroupColors: %s", groups and "present (CPU rebuild can run)"
+        or "MISSING (CPU rebuild cannot run)")
+  end
+
+  local colors = nil
+  local VoxelScene = V.require("VoxelScene")
+  if VoxelScene._modeColors then
+    colors = VoxelScene._modeColors(function(m)
+      return PaletteFX.pal(game.data, ow:paletteNameFor(m or map))
+    end, map)
+  end
+  say("colours for the bake: %s", colors and "present" or "nil (art as-is)")
+
+  local img = TerrainAtlas.forMap(map, colors)
+  local isCopy = img ~= nil and img ~= map.renderer.image
+  say("forMap -> %s", img == nil and "NIL"
+      or (isCopy and "an ANIMATED copy" or "the STATIC atlas (no animation)"))
+  if not isCopy then
+    return say("STOP: no animated copy -- the entry could not be built")
+  end
+
+  -- 4. does the copy actually change as the clock turns over?
+  local seen, order = {}, {}
+  for i = 1, 6 do
+    local before = clock()
+    U.wait(25)                     -- more than one 20-frame period
+    local step = math.floor(clock() / 20) % 8
+    if not seen[step] then seen[step] = true; order[#order + 1] = step end
+    say("  sample %d: clock %s -> %s, water step %d",
+        i, tostring(before), tostring(clock()), step)
+  end
+  say("distinct water steps seen: %d", #order)
+  if #order <= 1 then
+    say("STOP: the step is not turning over -- clock is moving but the")
+    say("      period maths is not reaching a new step")
+  else
+    say("OK: the atlas is being repatched; if the WATER still looks still,")
+    say("    the animated tiles are not in the terrain quads -- run")
+    say("    voxel_anim_probe.lua, which dumps the atlas and counts where")
+    say("    tile $14 ended up in the geometry")
+  end
+end