mirror of
https://github.com/imjasonh/nescript
synced 2026-07-08 17:06:04 +00:00
Bug B: runtime OAM cursor so draw inside loops actually works
`IrCodeGen::next_oam_slot` incremented at *compile time*: one
`draw` statement = one fixed OAM slot, baked into absolute-mode
stores at codegen. A `draw` inside a `while`/`for`/`loop` body
was lowered once and then always wrote to the same four OAM
bytes every iteration, so only the last iteration was ever
visible. The writeup in the earlier PR called this "bug B".
Fix: reserve ZP `$09` as `ZP_OAM_CURSOR`, reset it to 0 at the
top of every frame handler (right after the existing OAM clear
loop), and lower each `DrawSprite` IR op to:
LDY $09 ; load cursor
LDA <y_temp>
STA $0200,Y ; sprite Y
LDA #tile
STA $0201,Y ; tile
LDA #0
STA $0202,Y ; attr
LDA <x_temp>
STA $0203,Y ; sprite X
INC $09 x4 ; bump cursor by 4
Cost is ~+6 bytes per `draw` over the old static form. At 64
slots the u8 cursor wraps naturally, giving classic NES
"too many sprites" flicker instead of a silent compile-time
drop. `next_oam_slot` and its resets are gone from the IR
codegen entirely.
Secondary fix: `for i in 0..N` counters are now registered as
handler locals. `lower_statement` created a `VarId` for the
counter via `get_or_create_var` but never pushed it onto
`current_locals`, so the IR codegen's `var_addrs` lookup
returned `None` for every `StoreVar(i)` / `LoadVar(i)` and
silently emitted nothing. The counter stayed at 0 forever,
the loop spun indefinitely, and every iteration wrote the
first array element into OAM — turning all 64 sprites into
the same smiley. Same class as the handler-local `var` decl
bug from the earlier PR, just for for-loop variables.
Smoke-test deltas (all 14/14 still pass):
- arrays_and_functions: 104 -> 260 (player + 4 enemies)
- bitwise_ops: 104 -> 416 (player + flag sprites + pips)
- loop_break_continue: 208 -> 208 (already fixed by the earlier pass)
- structs_enums_for: 104 -> 260 (player + 4 enemies)
Regression tests:
- `ir_codegen::more_tests::ir_codegen_draw_sprite` — checks a
single `draw` emits `LDY cursor`, four `STA $020N,Y`, and
four `INC cursor`.
- `ir_codegen::more_tests::ir_codegen_multi_oam_uses_sequential_slots`
— rewritten for the new form: each draw gets its own
`LDY cursor` + 4 `INC cursor`.
- `ir_codegen::more_tests::ir_codegen_draw_in_loop_...` —
proves a `draw` inside a `while` compiles to ONE cursor-based
draw (not N unrolled statics and not zero), and asserts no
stray `STA $0204`/`$0208`/... absolute stores — those would
indicate bug B has regressed.
- `ir::tests::for_loop_counter_is_registered_as_handler_local`
— verifies `for i in 0..N` pushes `i` onto `current_locals`
so the IR codegen allocates it.
Smoke-test tightening: `tests/emulator/run_examples.mjs` now
has per-example `minNonBlack` floors. `arrays_and_functions`,
`structs_enums_for`, `loop_break_continue`, and `bitwise_ops`
all require multi-sprite rendering — if the OAM cursor bug
comes back, the smoke test fails loudly instead of passing on
the default `nonBlack > 0` check.
The legacy AST codegen in `src/codegen/mod.rs` still uses the
compile-time `next_oam_slot` approach. It's only reachable via
`--use-ast`, none of the examples use it, and its integration
tests only check iNES structure — left alone on purpose.
https://claude.ai/code/session_014Z5y3Q9krLcAxYpZQJhZ5V
This commit is contained in:
parent
f49dbce686
commit
54acb9ee38
8 changed files with 274 additions and 39 deletions
|
|
@ -17,6 +17,27 @@ const harnessUrl = pathToFileURL(path.join(__dirname, "harness.html")).toString(
|
|||
const FRAMES_TO_RUN = 180; // ~3 seconds at 60 fps, enough to get past a title/boot
|
||||
const SCREENSHOT_FRAME = 180;
|
||||
|
||||
// Per-example non-black pixel floors, used to catch silent
|
||||
// render regressions. A bare smiley sprite contributes ~52
|
||||
// non-black pixels; the default floor below assumes one visible
|
||||
// sprite. Examples that draw more sprites override the floor
|
||||
// with a tighter value so bugs like "only one of the four
|
||||
// enemies actually shows up" fail CI instead of silently
|
||||
// slipping past the base `nonBlack > 0` check.
|
||||
//
|
||||
// Each entry is `[minNonBlack, note]`. The note is printed when
|
||||
// the floor fails so it's easy to tell what the example was
|
||||
// supposed to show.
|
||||
const DEFAULT_MIN_NON_BLACK = 40; // one small sprite, conservative
|
||||
const EXAMPLE_FLOORS = {
|
||||
arrays_and_functions: [200, "player + 4 enemies drawn by a while loop"],
|
||||
bitwise_ops: [150, "player + multiple flag/pip sprites across if branches and a while loop"],
|
||||
loop_break_continue: [150, "player + 3 active hazards (one slot is inactive)"],
|
||||
structs_enums_for: [200, "player + 4 enemies drawn by a `for` loop"],
|
||||
sprites_and_palettes: [60, "custom CHR tiles visible"],
|
||||
scanline_split: [80, "banner + player"],
|
||||
};
|
||||
|
||||
async function listRoms() {
|
||||
const entries = await fs.readdir(examplesDir);
|
||||
return entries
|
||||
|
|
@ -25,6 +46,12 @@ async function listRoms() {
|
|||
.map((f) => ({ name: f.replace(/\.nes$/, ""), file: path.join(examplesDir, f) }));
|
||||
}
|
||||
|
||||
function floorFor(name) {
|
||||
const entry = EXAMPLE_FLOORS[name];
|
||||
if (entry) return entry;
|
||||
return [DEFAULT_MIN_NON_BLACK, "generic single-sprite floor"];
|
||||
}
|
||||
|
||||
async function main() {
|
||||
await fs.mkdir(screenshotsDir, { recursive: true });
|
||||
const roms = await listRoms();
|
||||
|
|
@ -90,13 +117,25 @@ async function main() {
|
|||
}
|
||||
|
||||
const lastStats = frameHashes[frameHashes.length - 1];
|
||||
const firstStats = frameHashes[0];
|
||||
const uniqueHashes = new Set(frameHashes.map((h) => h.hash)).size;
|
||||
const rendered = booted && lastStats && lastStats.nonBlack > 0;
|
||||
const animated = uniqueHashes > 1;
|
||||
|
||||
const status = rendered ? "OK" : "FAIL";
|
||||
if (!rendered) failures++;
|
||||
const [minNonBlack, floorNote] = floorFor(rom.name);
|
||||
const meetsFloor = rendered && lastStats.nonBlack >= minNonBlack;
|
||||
const pass = rendered && meetsFloor;
|
||||
|
||||
const status = pass ? "OK" : "FAIL";
|
||||
if (!pass) failures++;
|
||||
|
||||
let failReason = null;
|
||||
if (!booted) {
|
||||
failReason = `boot error: ${bootError ?? "unknown"}`;
|
||||
} else if (!rendered) {
|
||||
failReason = "rendered a fully black screen (nonBlack=0)";
|
||||
} else if (!meetsFloor) {
|
||||
failReason = `nonBlack=${lastStats.nonBlack} below floor=${minNonBlack} (${floorNote})`;
|
||||
}
|
||||
|
||||
results.push({
|
||||
name: rom.name,
|
||||
|
|
@ -104,6 +143,10 @@ async function main() {
|
|||
bootError,
|
||||
rendered,
|
||||
animated,
|
||||
meetsFloor,
|
||||
minNonBlack,
|
||||
floorNote,
|
||||
failReason,
|
||||
frames: frameHashes,
|
||||
consoleErrors,
|
||||
screenshot: booted ? path.relative(repoRoot, screenshotPath) : null,
|
||||
|
|
@ -112,9 +155,14 @@ async function main() {
|
|||
console.log(
|
||||
`${status.padEnd(4)} ${rom.name.padEnd(28)} ` +
|
||||
(rendered
|
||||
? `nonBlack=${lastStats.nonBlack}/${lastStats.totalPixels} uniqueHashes=${uniqueHashes} animated=${animated}`
|
||||
? `nonBlack=${lastStats.nonBlack}/${lastStats.totalPixels} (floor=${minNonBlack}) uniqueHashes=${uniqueHashes} animated=${animated}`
|
||||
: `boot=${booted} bootError=${bootError ?? "none"}`),
|
||||
);
|
||||
if (failReason && !rendered) {
|
||||
console.log(` reason: ${failReason}`);
|
||||
} else if (failReason) {
|
||||
console.log(` reason: ${failReason}`);
|
||||
}
|
||||
if (consoleErrors.length > 0) {
|
||||
for (const e of consoleErrors) console.log(" console:", e);
|
||||
}
|
||||
|
|
|
|||
Binary file not shown.
|
Before Width: | Height: | Size: 882 B After Width: | Height: | Size: 998 B |
Binary file not shown.
|
Before Width: | Height: | Size: 884 B After Width: | Height: | Size: 908 B |
Binary file not shown.
|
Before Width: | Height: | Size: 854 B After Width: | Height: | Size: 911 B |
Loading…
Add table
Add a link
Reference in a new issue