Merge pull request #16 from DramaticShape/android-night-cycle-fix

iterate version to 1.2.1

CHANGELOG.md

@@ -1,5 +1,41 @@
 # Changelog
 
+## 1.2.1
+
+### Fixed
+
+- **On Android the sky went black below its first couple of bands.** A hard-edged
+  band of black ran from partway down the gradient to the horizon point, with the
+  moon still hanging correctly inside it. Desktop was unaffected.
+
+  What gave it away is that the same colour reached the screen by two routes and
+  only one of them was wrong. The haze filling the void UNDER the horizon is the
+  sky's palest band, and it is delivered by `love.graphics.clear` -- it landed
+  correctly. The bottom of the sky above it is that same band delivered by the
+  shader, and it was black. So the palette was not reaching the fragment shader,
+  and nothing was wrong with the palette, the layout or the camera.
+
+  The bands went in as `uniform vec3 bands[8]`, filled from Lua and read through
+  a loop counter, and on Android's GLSL ES the tail of that array arrived as
+  zero -- which is black. The likeliest reason is the fragment uniform budget:
+  ES 2.0 only guarantees sixteen uniform VECTORS, and eight band slots plus the
+  twilight glow plus LOVE's own built-ins is over it. A driver that truncates a
+  partly-filled array, or one that reflects `bands[0]` and nothing after it,
+  fails identically -- so the fix removes the whole class rather than the one
+  cause.
+
+  The bands are a one-texel-per-band TEXTURE now, sampled nearest, with the
+  band index clamped against the ramp's width. One texture unit replaces eight
+  uniform vectors, there is no array to index and no budget to overrun, and a
+  sample past the last band lands on the last band instead of on nothing. It is
+  still a palette and not a picture -- one texel per band on a single row -- so
+  the sky is still computed per pixel at the size it is displayed at, with
+  nothing resampled and nothing baked.
+
+  Also gone with it: `clamp(x, 0.0, 0.999999)`, which rounds its bound to 1.0 at
+  mediump -- the fragment default on GLSL ES -- and would have indexed one past
+  the last band on the sky's bottom row for the same black result.
+
 ## 1.1.1
 
 ### Fixed

lib/Sky.lua

@@ -16,11 +16,13 @@
 -- as four stripes. No clouds, nothing moving.
 --
 -- NOTHING IS RESAMPLED, which is the whole of why it is drawn this way. There is
--- no baked 160x144 picture scaled up to the window, no downsized buffer blown
+-- no baked 160x144 picture scaled up to the window and no downsized buffer blown
--- back up, no texture of any kind: one full-region rectangle through a shader
+-- back up: one full-region rectangle through a shader that answers every pixel
--- that answers every pixel from its own canvas coordinate. A pixel of sky is
+-- from its own canvas coordinate. A pixel of sky is computed at the size it is
--- computed at the size it is displayed at, so there is nothing for a filter to
+-- displayed at, so there is nothing for a filter to soften and nothing to go
--- soften and nothing to go stale when the window or the zoom changes.
+-- stale when the window or the zoom changes. The shader does bind one texture,
+-- but it is a palette rather than an image -- the bands, one texel each, sampled
+-- nearest (see rampFor, and why it is not a uniform array).
 --
 -- THE PIXEL GRID follows the zoom for the same reason. Bands and dither cells
 -- are measured in DIORAMA pixels -- the pass's own pixels-per-world-pixel, handed
@@ -51,13 +53,11 @@ local V = ...
 local DayNight = V.require("DayNight")
 local PaletteFX = require("src.render.PaletteFX")
 
-local unpack = table.unpack or unpack
-
 local Sky = {}
 
--- The shader carries a fixed-size array, because a GLSL uniform array is a
+-- The most bands a phase palette may paint with. Eight leaves headroom over
--- fixed size; eight leaves headroom over DayNight's six-band phase palettes
+-- DayNight's six-band ones without paying for more; the ramp the shader reads
--- without paying for more.
+-- them from is built at the width actually used, so the cap costs nothing.
 Sky.MAX_BANDS = 8
 
 -- The checkerboard between bands. DITHER_START is how far down a band it begins,
@@ -84,7 +84,7 @@ Sky.SPAN = 0.23
 --
 -- Memoised, because this runs once a frame and the answer only moves when the
 -- mode does.
-local cache = { bands = nil, key = {} }
+local cache = { bands = nil, key = {}, ramp = nil }
 
 function Sky.bands()
   local pal = DayNight.palette()
@@ -102,6 +102,11 @@ function Sky.bands()
   end
   if same then return cache.bands end
 
+  -- the ramp is these bands as a texture (see rampFor); a new list is a new
+  -- ramp, and the old one is nothing's to keep
+  if cache.ramp and cache.ramp.release then pcall(cache.ramp.release, cache.ramp) end
+  cache.ramp, cache.rampFor = nil, nil
+
   local bands = {}
   for i = 1, n do
     -- backwards: the palette's darkest rung is the top band
@@ -150,17 +155,18 @@ end
 
 -- ------- the pass
 --
--- One rectangle, one shader, no texture. Every pixel answers for itself from its
+-- One rectangle, one shader. Every pixel answers for itself from its canvas
--- canvas coordinate, so the sky is drawn at exactly the resolution it is
+-- coordinate, so the sky is drawn at exactly the resolution it is displayed at
--- displayed at -- there is no image being scaled and so nothing to be soft.
+-- -- there is no image being scaled and so nothing to be soft. The one texture
+-- bound is the band ramp, which is a PALETTE and not a picture: n texels wide,
+-- sampled nearest, one lookup per pixel (see rampFor).
 --
 -- `cell` quantises BOTH the band edges and the dither: the y a pixel is judged
 -- by is the top of its own cell row, so a whole cell row is one colour and every
 -- edge in the sky lands on the diorama's pixel grid.
 local SHADER_SRC = [[
-#define MAXB %d
+uniform Image ramp;     // the bands, one texel each, top of the sky first
-uniform vec3 bands[MAXB];
+uniform float count;    // how many texels wide that ramp is
-uniform int count;
 uniform float edge;     // the sky's bottom, in canvas pixels
 uniform float cell;     // the diorama's pixel size, in canvas pixels
 uniform float start;    // where the checker begins inside a band
@@ -170,26 +176,23 @@ uniform vec2 glowPos;   // the sun disc, in canvas pixels
 uniform float glowInvR; // 1 / the glow's reach
 uniform vec3 glowColor;
 
-// Indexed through a loop counter, which every GLSL ES compiler accepts for a
+// Band `i`, read from its own texel centre. The index is clamped rather than
-// uniform array; a bare bands[idx] is not portable.
+// trusted: `pos` below can land exactly on `count` when the arithmetic is
-vec3 bandAt(int idx) {
+// carried at mediump -- which is the fragment default on GLSL ES -- and a
-  vec3 c = bands[0];
+// sample past the last band must be the last band, not whatever is off the
-  for (int i = 1; i < MAXB; i++) {
+// end of the image.
-    if (i == idx) { c = bands[i]; }
+vec3 bandAt(float i) {
-  }
+  return Texel(ramp, vec2((clamp(i, 0.0, count - 1.0) + 0.5) / count, 0.5)).rgb;
-  return c;
 }
 
 vec4 effect(vec4 color, Image tex, vec2 tc, vec2 sc) {
-  float n = float(count);
   float row = floor(sc.y / cell) * cell;              // top of this cell row
-  float pos = clamp(row / max(edge, 1.0), 0.0, 0.999999) * n;
+  float pos = min(row / max(edge, 1.0), 1.0) * count;
-  float base = floor(pos);
+  float base = min(floor(pos), count - 1.0);
-  int idx = int(base);
+  vec3 c = bandAt(base);
-  vec3 c = bandAt(idx);
   float parity = mod(floor(sc.x / cell) + floor(sc.y / cell), 2.0);
-  if (idx < count - 1 && (pos - base) > start) {
+  if (base < count - 1.0 && (pos - base) > start) {
-    if (parity < 0.5) { c = bandAt(idx + 1); }
+    if (parity < 0.5) { c = bandAt(base + 1.0); }
   }
   // The sunset's warmth, radiating from the disc: posterised to a few rungs
   // and checker-dithered between them -- the same 8-bit move as the bands,
@@ -207,14 +210,66 @@ vec4 effect(vec4 color, Image tex, vec2 tc, vec2 sc) {
 }
 ]]
 
+-- ------- the ramp
+--
+-- The bands as a one-texel-per-band TEXTURE rather than as a uniform array,
+-- which is what they used to be: `uniform vec3 bands[8]`, filled from Lua and
+-- read through a loop counter. On desktop GL that is as portable as it looks.
+-- On Android it was not. The sky's lower bands came back BLACK -- a hard-edged
+-- strip running from partway down the gradient to the horizon point, with the
+-- moon still drawn correctly over it, and with the haze BELOW the sky (the
+-- palest band again, but delivered by love.graphics.clear instead of by the
+-- array) landing in exactly the right colour. Same colour, two routes, one of
+-- them black: the fault was the array, not the palette.
+--
+-- Which of the ES failure modes it was hardly matters -- a driver that
+-- truncates a partially-filled array, a fragment uniform budget the guaranteed
+-- floor of which is sixteen vectors (eight bands plus the glow plus LOVE's own
+-- built-ins is over it), a reflection that finds bands[0] and nothing after --
+-- because they all have the same shape: slots past the first few read as zero,
+-- and zero is black.
+--
+-- A sampler has none of them. One texture unit replaces eight uniform vectors,
+-- there is no array to index and no budget to overrun, and a texel that does
+-- not exist cannot read as black because the image is built at exactly the
+-- width the shader divides by. Nearest and clamped, so a sample lands on one
+-- band's own colour and an out-of-range one lands on the end band rather than
+-- on nothing.
+--
+-- Rebuilt only when the bands move, which is when the clock or the display
+-- mode does; Sky.bands drops it as it rebuilds the list it is made from.
+local function rampFor(bands)
+  if cache.ramp and cache.rampFor == bands then return cache.ramp end
+  if not (love.image and love.image.newImageData
+          and love.graphics and love.graphics.newImage) then return nil end
+  local n = #bands
+  if n < 1 then return nil end
+  local ok, data = pcall(love.image.newImageData, n, 1)
+  if not (ok and data) then return nil end
+  for i = 1, n do
+    local c = bands[i]
+    pcall(data.setPixel, data, i - 1, 0, c[1], c[2], c[3], 1)
+  end
+  local built, img = pcall(love.graphics.newImage, data)
+  if not (built and img) then return nil end
+  -- nearest: a band is a flat colour, not something to interpolate between.
+  -- clamp: the shader clamps its index too, so this is the second of two
+  -- guards against ever sampling off the end -- and it returns the edge band.
+  pcall(img.setFilter, img, "nearest", "nearest")
+  pcall(img.setWrap, img, "clamp", "clamp")
+  cache.ramp, cache.rampFor = img, bands
+  return img
+end
+
+Sky._rampFor = rampFor            -- named for the suite
+
 local shader = nil            -- nil = untried, false = unavailable
 
 local function getShader()
   if shader == nil then
     shader = false
     if love.graphics and love.graphics.newShader then
-      local ok, sh = pcall(love.graphics.newShader,
+      local ok, sh = pcall(love.graphics.newShader, SHADER_SRC)
-                           SHADER_SRC:format(Sky.MAX_BANDS))
       if ok and sh then
         shader = sh
       elseif V and V.mod and V.mod.log then
@@ -358,12 +413,13 @@ function Sky.paint(w, h, sky, horizonY, cell, body)
 
   local glowAmt = body and not body.moon and (body.glowAmt or 0) or 0
   local sh = getShader()
+  local ramp = sh and rampFor(bands)
+  if not ramp then sh = nil end       -- no ramp, no gradient: paint it flat
   if sh then
     local sent = pcall(function()
-      -- one send per band would be one uniform lookup per band; the array takes
+      -- the bands arrive as a texture, one texel each, and `count` is that
-      -- them all at once, and it must be the LAST argument or Lua truncates the
+      -- texture's width -- see rampFor for why they are not a uniform array
-      -- unpack to a single value
+      sh:send("ramp", ramp)
-      sh:send("bands", unpack(bands))
       sh:send("count", #bands)
       sh:send("edge", edge)
       sh:send("cell", cell)
@@ -399,9 +455,12 @@ function Sky.paint(w, h, sky, horizonY, cell, body)
 end
 
 -- Drop the compiled shader (window resize, hot reload), so a re-created graphics
--- context builds a new one instead of drawing with a handle from the old.
+-- context builds a new one instead of drawing with a handle from the old. The
+-- ramp is a GPU object on the same context and goes with it.
 function Sky.invalidate()
   shader = nil
+  if cache.ramp and cache.ramp.release then pcall(cache.ramp.release, cache.ramp) end
+  cache.ramp, cache.rampFor = nil, nil
 end
 
 return Sky

main.lua

@@ -745,7 +745,7 @@ mod.hooks:wrap("world.tod", function(next, tod, ctx)
   return DayNight.tod()
 end)
 
-mod.exports.version = "1.2.0"
+mod.exports.version = "1.2.1"
 -- exposed so a companion mod can pin its own tiles' shapes or read the
 -- camera without reaching into this mod's file layout
 mod.exports.lib = V

manifest.json

@@ -1,7 +1,7 @@
 {
   "id": "DRAMATIC_SHAPE",
   "name": "Dramatic Shape Voxel Mod",
-  "version": "1.2.0",
+  "version": "1.2.1",
   "api": 2,
   "entry": "main.lua",
   "profile": "content",

tests/dramatic_shape_test.lua

@@ -1278,22 +1278,45 @@ T.check(Sky.SPAN > 0.1 and Sky.SPAN < 0.5,
 
 -- ------- the pass, as it is actually issued
 --
--- One rectangle through one shader: no texture, no baked image, nothing being
+-- One rectangle through one shader: no baked picture of a sky, nothing being
 -- resampled -- which is the whole reason it is drawn this way rather than
 -- generated once and scaled. Every pixel answers from its own canvas coordinate,
 -- so it is computed at the size it is shown at.
 --
+-- The one texture bound is the band RAMP, and it is a palette rather than a
+-- picture: one texel per band, sampled nearest. It used to be a uniform array,
+-- and that is the bug this shape exists to have fixed -- on Android the array's
+-- later slots arrived as zero and painted the bottom of the sky black, while the
+-- identical colour delivered by love.graphics.clear (the haze under the horizon)
+-- landed correctly. So the assertions below pin the ramp, not an array.
+--
 -- And the depth mode is put back to what it was, which is the piece that would
 -- break the frame: a rectangle drawn under the pass's own ("lequal", true) stamps
 -- itself across the depth buffer at the near plane and hides the whole world
 -- behind the sky.
-local realGraphics = love.graphics
+local realGraphics, realImage = love.graphics, love.image
 local rects, depthCalls, sent, shaderUses = {}, {}, {}, 0
 local fakeShader = {
   send = function(_, name, a, b, c, d)
     sent[name] = { a, b, c, d }
   end,
 }
+-- enough of an image to be built and measured; the ramp only ever has pixels
+-- written into it and its dimensions read back
+local function fakeImage(w, h)
+  return {
+    pixels = {},
+    getWidth = function(self) return w end,
+    getHeight = function(self) return h end,
+    getDimensions = function(self) return w, h end,
+    setPixel = function(self, x, _, r, g, b, a)
+      self.pixels[x] = { r, g, b, a }
+    end,
+    setFilter = function() end,
+    setWrap = function() end,
+  }
+end
+love.image = { newImageData = function(w, h) return fakeImage(w, h) end }
 love.graphics = {
   getShader = function() return nil end,
   setShader = function(sh) if sh then shaderUses = shaderUses + 1 end end,
@@ -1303,14 +1326,17 @@ love.graphics = {
   end,
   setColor = function() end,
   newShader = function() return fakeShader end,
+  newImage = function(data) return data end,
   rectangle = function(_, x, y, w, h)
     rects[#rects + 1] = { x = x, y = y, w = w, h = h }
   end,
 }
+Sky.invalidate()   -- so the ramp is built through the fakes above, not held
 
 -- 320x288 canvas, horizon at 66.83, diorama pixels 7 canvas pixels square
 local painted = Sky.paint(320, 288, skyGrad, 66.83, 7)
-love.graphics = realGraphics
+local ramp = Sky._rampFor(skyGrad.bands)
+love.graphics, love.image = realGraphics, realImage
 
 T.eq(painted, true, "the sky paints")
 T.eq(shaderUses, 1, "through one shader")
@@ -1327,8 +1353,24 @@ T.eq(sent.cell[1], 7,
   .. "cells on the world's own grid")
 T.eq(sent.start[1], Sky.DITHER_START, "and where in a band the checker begins")
 T.eq(sent.alpha[1], 1, "and the tween strength")
-T.check(sent.bands[1] and sent.bands[1][1] ~= nil,
+-- The palette goes as ONE ramp texture, not as eight uniform vectors. The width
-  "the palette goes as one array rather than a send per band")
+-- is the contract the shader divides by: it samples texel (i + 0.5) / count, so
+-- a ramp of any other width reads between two bands or off the end -- and off
+-- the end is exactly the black the Android bug painted.
+T.eq(sent.ramp[1], ramp, "the palette goes to the shader as its ramp texture")
+T.eq(ramp:getWidth(), #skyGrad.bands, "one texel per band, and no spare slots")
+T.eq(ramp:getHeight(), 1, "on a single row -- it is a palette, not a picture")
+T.eq(Sky._rampFor(skyGrad.bands), ramp,
+  "and it is built once and held, not rebuilt per frame")
+-- every texel is a real colour: the failure being fixed here is a slot that
+-- was never written reading back as zero, which is black
+for i = 1, #skyGrad.bands do
+  local texel = ramp.pixels[i - 1]
+  T.check(texel ~= nil, "band " .. i .. " was written into the ramp")
+  T.check(texel[1] == skyGrad.bands[i][1] and texel[2] == skyGrad.bands[i][2]
+          and texel[3] == skyGrad.bands[i][3],
+    "and it is that band's own colour, in the order the sky reads them")
+end
 
 T.eq(depthCalls[1], "always/false", "the sky is drawn with depth writes OFF")
 T.eq(depthCalls[#depthCalls], "lequal/true",

tests/sky_ramp_probe.lua

@@ -0,0 +1,55 @@
+-- Driver: prove the banded sky still paints from the ramp texture.
+--
+-- The bands used to reach the shader as `uniform vec3 bands[8]`, which on
+-- Android delivered only its first few slots and painted the rest of the sky
+-- black (see lib/Sky.lua, rampFor).  They are a texture now.  This checks the
+-- three things that swap could have broken, on the machine it CAN be checked
+-- on: that the shader still compiles, that the ramp is built and is one texel
+-- per band, and that what lands on screen is still a gradient that pales
+-- downward rather than a flat plate or a black one.
+--
+--   SHOT_DIR=.scratchpad/skyramp POKEPORT_DRIVER=mods/DramaticShapeVoxelMod/tests/sky_ramp_probe.lua love .
+return function(game)
+  local U = dofile("tests/drivers/util.lua")
+  local DIR = os.getenv("SHOT_DIR") or ".scratchpad/skyramp"
+
+  local exports = game.mods and game.mods.exports
+  local lib = exports and exports.DRAMATIC_SHAPE and exports.DRAMATIC_SHAPE.lib
+  if not lib then
+    U.log("DRAMATIC_SHAPE is not loaded -- enable it and run again")
+    return
+  end
+  local Sky = lib.require("Sky")
+  local DayNight = lib.require("DayNight")
+
+  -- which copy of the mod is actually live: only the ramp build has this
+  U.log("live copy has _rampFor:", tostring(Sky._rampFor ~= nil))
+  U.log("shader compiled:", tostring(Sky._getShader() ~= nil))
+
+  U.teleport(game, "PALLET_TOWN", 12, 10, "up")
+  require("src.render.Pipelines").setLevel("voxel", 5)
+  U.wait(150)
+
+  for _, phase in ipairs({ "day", "dusk", "night" }) do
+    DayNight.setting:sync(phase)
+    DayNight.update(0)
+    U.wait(30)
+
+    local bands = Sky.bands()
+    local ramp = Sky._rampFor and Sky._rampFor(bands)
+    local w = ramp and ramp:getWidth() or -1
+    U.log(("%s: %d bands, ramp %dx%d"):format(
+      phase, #bands, w, ramp and ramp:getHeight() or -1))
+    -- one texel per band is the whole contract: the shader divides by `count`
+    -- and samples texel centres, so a ramp of any other width samples between
+    -- bands or off the end
+    if w ~= #bands then U.log("FAIL ramp width does not match band count") end
+    -- and the ramp is the ramp: the same table gives the same image back
+    if ramp ~= Sky._rampFor(bands) then U.log("FAIL ramp rebuilt per call") end
+
+    U.shot(game, ("%s/%s.png"):format(DIR, phase))
+  end
+
+  DayNight.setting:sync("day")
+  U.log("done -- " .. DIR)
+end