tilemap: scan scenes for Tilemap + embed .tmx and tileset images (T2 Phase 4) - #560
Conversation
…Phase 4) Scan scene JSONC for `Tilemap` components, resolve each `asset_name` to a `.tmx` under the project's `assets/` dir, and comptime-embed the `.tmx` plus every tileset `<image source>` it references so labelle-engine v1.75.0 can decode + render the tilemap. Engine contract (labelle-engine v1.75.0, game/tilemap_mixin.zig): a single `Game.addEmbeddedTilemapAsset(name, bytes)` registry is keyed by BOTH the scene `asset_name` (-> .tmx bytes) AND each tileset's verbatim `image_source` string (-> image bytes; the engine's `ImageProvider.get` looks up by that exact string). The generated `init()` now populates it. - scene_manifest: `SceneManifest.tilemap_assets` — deep-walk the entity tree for `Tilemap` components (flat/wrapped/children/bundle shapes). - tilemap_scan.zig: resolve `asset_name` -> `assets/<asset_name>.tmx`, read it, extract `<image source>` refs (tight XML scan), and build a deduped flat list of `addEmbeddedTilemapAsset` registrations. Image registry key = verbatim `image_source`; @embedfile path = resolved relative to the .tmx dir. - codegen: emit registrations before `setScene` in both lifecycle paths (loop `try`, callback `catch @panic`), via the module-level-var pattern used by pack_scans. Purely additive — empty for tilemap-free projects. - Bump default engine_version 1.60.0 -> 1.75.0. Tests: scan extractor + path convention + collect (tmpDir) + scene extraction + both emit spellings. Verified e2e: a null-backend fixture generates registrations before setScene and passes `zig ast-check`. Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds an embedded-tilemap asset pipeline: scanning TMX files and referenced tileset images (src/tilemap_scan.zig), extracting Tilemap component asset names from scene JSON (tilemap_scene_scan.zig, scene_manifest.zig), collecting registrations during generation (tilemap_phase.zig), and emitting ChangesTilemap embedding feature
Build Default Version Bump
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RootGenerate as root.zig generate()
participant TilemapPhase as tilemap_phase.collectRegistrations
participant SceneManifest as scene_manifest.zig
participant TilemapScan as tilemap_scan.collect
participant MainTemplate as main_template.zig
participant Lifecycle as callback.zig / loop.zig
RootGenerate->>SceneManifest: scanTilemapAssets(scene sources)
SceneManifest-->>RootGenerate: tilemap_assets per scene
RootGenerate->>TilemapPhase: collectRegistrations(manifests, components, prefabs)
TilemapPhase->>TilemapScan: collect(target_dir, asset_names)
TilemapScan-->>TilemapPhase: []Registration (tmx + image keys)
TilemapPhase-->>RootGenerate: registrations
RootGenerate->>MainTemplate: set tilemap_registrations (threadlocal)
MainTemplate->>Lifecycle: emitTilemapRegistrations(ctx.tilemap_registrations)
Lifecycle-->>MainTemplate: addEmbeddedTilemapAsset(...) calls before setScene
RootGenerate->>TilemapScan: freeRegistrations(regs)
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f582d8694
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (scene_manifests) |m| { | ||
| for (m.tilemap_assets) |an| try tilemap_asset_names.append(allocator, an); | ||
| } |
There was a problem hiding this comment.
When a scene instantiates a prefab whose JSONC contains a Tilemap component, this aggregation contributes no asset names because it only reads scene_manifests; prefabs are merely registered later via JsoncBridge.addEmbeddedPrefab(...) and are not parsed into tilemap_assets. The generated init() therefore omits the addEmbeddedTilemapAsset registrations for that prefab's .tmx and tileset images, so setScene will expand the prefab and the engine's tilemap lookup will miss. Please scan prefabs/ (and pack prefabs) as well, or reject/document prefab Tilemaps as unsupported.
Useful? React with 👍 / 👎.
| // Recurse into every value so a Tilemap nested under | ||
| // `components` / `children` / `root` / etc. is still found. | ||
| try collectTilemapAssets(allocator, entry.value_ptr.*, out); |
There was a problem hiding this comment.
For a scene with arbitrary component data like { "components": { "Spawner": { "Tilemap": { "asset_name": "preview" } } } }, this unconditional recursion walks into the Spawner payload and treats the nested key as an entity Tilemap component. That makes generation try to read assets/preview.tmx even though the scene did not declare a Tilemap, so valid scenes can start failing or embedding unrelated assets; the walker needs to track component-map/entity context instead of recursing into every object value.
Useful? React with 👍 / 👎.
| for (images) |image_source| { | ||
| // Dedup images by their registry key (the verbatim | ||
| // image_source): a tileset image shared by two maps, or two | ||
| // tilesets in one map, registers once. | ||
| if (seen.contains(image_source)) continue; |
There was a problem hiding this comment.
If a map asset name matches a tileset image source, for example a scene asset_name of tiles.png and a TMX containing <image source="tiles.png">, this shared seen map skips the image registration because the .tmx key was already inserted. The engine then looks up tiles.png for the tileset image and receives the TMX bytes instead of the PNG bytes, so the tilemap cannot render correctly; this should be detected as a collision or tracked separately from image deduplication.
Useful? React with 👍 / 👎.
| const tag = tmx[tag_start..tag_end]; | ||
| if (attrValue(tag, "source")) |src| { | ||
| try out.append(allocator, try allocator.dupe(u8, src)); | ||
| } |
There was a problem hiding this comment.
For valid TMX files whose image filename needs XML escaping, such as source="tiles&decor.png" for an on-disk tiles&decor.png, this stores the raw escaped text as both the registry key and the @embedFile path. The generated Zig will try to embed assets/tiles&decor.png (which normally does not exist), and even if it did, the runtime TMX parser will request the decoded tiles&decor.png key; unescape XML attribute values before resolving and registering them.
Useful? React with 👍 / 👎.
| // build of the CLI overrides each with `-D<pkg>_version=`. | ||
| const core_version: []const u8 = b.option([]const u8, "core_version", "Default core library version") orelse "1.13.0"; | ||
| const engine_version: []const u8 = b.option([]const u8, "engine_version", "Default engine library version") orelse "1.60.0"; | ||
| const engine_version: []const u8 = b.option([]const u8, "engine_version", "Default engine library version") orelse "1.75.0"; |
There was a problem hiding this comment.
This line makes newly scaffolded projects pin labelle-engine 1.75.0, but the adjacent defaults still stamp labelle-core 1.13.0 and labelle-gfx 1.10.0. The 1.75.0 engine package documents an effective labelle-core floor of v1.24.0, so labelle init && labelle build resolves an incompatible dependency set and fails before user code; please update the dependent defaults in the same change or keep the engine default on the older compatible release.
Useful? React with 👍 / 👎.
| if (std.mem.eql(u8, entry.key_ptr.*, "Tilemap") and entry.value_ptr.* == .object) { | ||
| if (entry.value_ptr.object.get("asset_name")) |an| { | ||
| if (an == .string and an.string.len > 0) { | ||
| try out.append(allocator, try allocator.dupe(u8, an.string)); |
There was a problem hiding this comment.
When a project defines its own component named Tilemap and that component has an asset_name field, the engine loader gives the project component precedence over the built-in, but this scan unconditionally treats the key as the built-in tilemap. Such scenes now fail generation unless assets/<asset_name>.tmx exists, or they embed an unrelated map for a component the engine will deserialize normally; skip this collection when the scanned component registry contains a project Tilemap.
Useful? React with 👍 / 👎.
…compile) engine 1.75.0's tilemap_runtime failed to compile against any gfx-1.21.0 backend (generic getTextureInfo seam) — the Examples integration test's null-backend asset-streaming-smoke build caught it. 1.75.1 (engine#707) hardens supported() + derives the texture type from the concrete resolver fn. Brings the fix in transitively (engine 1.75.1 pins gfx 1.21.0). Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47802454c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Dedup images by their registry key (the verbatim | ||
| // image_source): a tileset image shared by two maps, or two | ||
| // tilesets in one map, registers once. | ||
| if (seen.contains(image_source)) continue; |
There was a problem hiding this comment.
When two TMX files in different directories both use a local image such as source="tiles.png", the first registration wins and this check suppresses the second solely by the registry key, even though imageEmbedPath would point at different files like assets/maps/tiles.png vs assets/other/tiles.png. Because the runtime lookup key is still just tiles.png, the later map will render with the earlier map's image bytes; detect this key-to-path conflict instead of silently deduping it.
Useful? React with 👍 / 👎.
| while (std.mem.indexOfPos(u8, tag, search, name)) |at| { | ||
| const after = at + name.len; | ||
| // Require `="` immediately after the attribute name. | ||
| if (after + 1 < tag.len and tag[after] == '=' and tag[after + 1] == '"') { |
There was a problem hiding this comment.
This parser only recognizes source="..." with no whitespace and double quotes, but valid TMX/XML can write the same attribute as source = "tiles.png" or source='tiles.png'. In those cases extractImageSources emits no tileset image registration even though the engine's XML parser will still request that source at load time, so the tilemap loads with a missing tileset image.
Useful? React with 👍 / 👎.
| for (regs) |r| { | ||
| switch (style) { | ||
| .try_style => try w.print( | ||
| " try g.addEmbeddedTilemapAsset(\"{s}\", @embedFile(\"{s}\"));\n", |
There was a problem hiding this comment.
When a TMX image source or scene asset_name contains a backslash or quote, for example a Windows-authored source="tiles\terrain.png", these raw {s} insertions produce an invalid Zig string or reinterpret escapes like \t before calling addEmbeddedTilemapAsset. The engine still requests the original key from the TMX, so the generated registry key/path no longer matches; emit these values with the existing Zig string escaping helper instead of interpolating them directly.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| var search: usize = 0; | ||
| while (std.mem.indexOfPos(u8, tmx, search, "<image")) |tag_start| { |
There was a problem hiding this comment.
For a valid Tiled map that uses an external tileset like <tileset firstgid="1" source="terrain.tsx"/>, there is no inline <image> for this loop to collect, so the generated registry contains only the TMX. setScene still asks the engine to decode that map from embedded bytes with no filesystem/base-path support for the TSX or its image, which leaves the tilemap failing at runtime; either embed/rewrite external tilesets or fail during collect with a clear unsupported-format error.
Useful? React with 👍 / 👎.
P1 — bump core/gfx defaults to match engine 1.75.1's transitive pins so a fresh `labelle init && build` resolves a compatible set: core 1.24.0, gfx 1.21.0 (read from engine v1.75.1 → gfx v1.21.0 → core v1.24.0 build.zig.zon). P2 fixes: - Walker over-match: replace the generic deep scan with a STRUCTURED entity-tree walk (`tilemap_scene_scan.zig`) — a `Tilemap` key is only collected when it's a direct member of an entity's `components`/`overrides` map or a flat-form component key, never nested in another component's data. - tmx/image key collision: track the `.tmx` and image key spaces separately and hard-error (`TilemapKeyCollision`) when a scene `asset_name` collides with a tileset `<image source>` in the shared engine registry. - XML-unescape `<image source>` (& < > " ') before using it as the @embedfile path AND the registry key — gfx hands the engine the DECODED string. - Honor project-registered `Tilemap` (engine C2): skip built-in embedding when a project component pascal-matches `Tilemap`. Plugin/pack-registered Tilemap deferred (fails loud via missing-asset) — filed assembler#562. - Prefab-borne Tilemaps: not embedded in minimal-T2 — detect + fail loud with a clear message instead of a silently broken binary. Filed #561. Refactor to keep files < 1000 lines: extract the JSON walker into `tilemap_scene_scan.zig` and the generate-phase policy into `root/tilemap_phase.zig`. Tests: walker over-match (nested non-component Tilemap), key collision, XML-unescape, prefab-scan detection. zig build + test green, fmt clean. Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d72c8a9be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| // Embedded-tilemap registrations (T2 Phase 4): resolve scene-declared | ||
| // `Tilemap` assets to `@embedFile` registrations. See `root/tilemap_phase.zig`. | ||
| const tilemap_registrations = try tilemap_phase.collectRegistrations(allocator, target_dir, scene_manifests, component_names, prefab_names); |
There was a problem hiding this comment.
Fresh evidence in the revised code is that this collection still runs before loadPackScans and only passes the game-root prefab_names, so failOnPrefabTilemaps never inspects packs/<name>/prefabs/*.jsonc. When a scene instantiates a light-pack prefab that declares a built-in Tilemap, the generated init registers that pack prefab later via pack_scans but has no .tmx/tileset registrations for it, so scene load hits the engine's embedded tilemap lookup with missing bytes; include pack prefabs in this phase or reject them after pack scanning.
Useful? React with 👍 / 👎.
| allocator.free(tmx_embed); | ||
| return error.TilemapAssetNotFound; | ||
| }; | ||
| defer allocator.free(tmx_bytes); |
There was a problem hiding this comment.
Because this defer is inside the for (asset_names) loop without an inner block, every .tmx buffer stays allocated until collect returns rather than being released after that map is scanned; the images slice below has the same lifetime issue. Projects with many tilemaps, especially near the 8 MiB per-file limit, can retain hundreds of MiB during generation and fail with OOM even though only one map needs to be parsed at a time.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| var search: usize = 0; | ||
| while (std.mem.indexOfPos(u8, tmx, search, "<image")) |tag_start| { |
There was a problem hiding this comment.
This raw substring scan also matches <image ...> text inside XML comments, so a valid TMX that keeps an old tileset line as <!-- <image source="old.png"/> --> will still generate an @embedFile("assets/old.png") registration even though the runtime XML parser ignores the comment. If that old file is absent, the generated Zig fails to compile; if it exists, the assembler embeds an unrelated asset that the map never requests, so the scan should skip comment ranges or use an XML-aware attribute pass.
Useful? React with 👍 / 👎.
- #1 escape generated string literals: emit the registry key AND @embedfile path through std.zig.fmtString (`{f}`), not raw `{s}` — a backslash/quote in an image source or asset_name (e.g. Windows `tiles\terrain.png`) no longer produces invalid Zig or a mis-keyed literal. - #2 same image key → different paths: `img_seen` now maps key→resolved embed path; two maps in different dirs both referencing `tiles.png` (different files, same runtime key) hard-error instead of silently reusing the first's bytes. Same-key-same-path stays a benign dedup. - #3 external tilesets: detect `<tileset source="*.tsx">` (external, no inline <image>) in `collect` and fail loud with the offending .tsx named — gfx returns error.ExternalTilesetUnsupported at runtime otherwise. Filed assembler#563. - #4 attribute syntax: `attrValue` now tolerates whitespace around `=` and single OR double quotes (`source = "x"`, `source='x'`), still a tight scan. Order: locate attr → strip quotes → XML-unescape. Tests: escaping round-trips to valid Zig (ast-parsed), diff-path collision, benign same-path dedup, external-tileset error, and the new attr syntaxes. zig build + test green, goldens byte-identical, fmt clean, files < 1000. Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
test/main_zig_tests.zig (1)
1355-1360: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the callback lifecycle assertion promised here.
The suite only checks the loop backend’s
try g.addEmbeddedTilemapAsset(...)form, but this comment and PR also cover the sokol/wasm callback path. Add a callback test that setsh.setSokolLifecycle()and assertsg.addEmbeddedTilemapAsset(...) catch@Panic(...).Also applies to: 1369-1382
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/main_zig_tests.zig` around lines 1355 - 1360, The embedded tilemap registration coverage is missing the sokol/wasm callback lifecycle path; add a test that uses h.setSokolLifecycle() and verifies the generated init() emits g.addEmbeddedTilemapAsset(...) with catch `@panic`(...) instead of only the loop backend’s try form. Use the existing main_template.tilemap_registrations flow and the addEmbeddedTilemapAsset assertion pattern so both backend spellings are covered.src/tilemap_scan_test.zig (1)
356-448: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a bundle-array regression test for Tilemap collection.
src/tilemap_scan_test.zig:356-448covers flat/root-wrapped/children cases, but not a top-level[]scene. Add one bundle-root case here to pin the array path too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tilemap_scan_test.zig` around lines 356 - 448, Add a regression test for top-level bundle-array scenes in scene_manifest parsing. The current tests around parseSceneSource and scanTilemapAssets cover flat, children, and components-wrapped cases, but miss the root `[]` path; add a case that feeds a bundle-array scene containing a Tilemap and asserts the asset is collected. Use the existing test pattern in scene_manifest.parseSceneSource and/or scene_manifest.scanTilemapAssets so the array-root behavior is pinned alongside the other Tilemap collection tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/root/tilemap_phase.zig`:
- Around line 73-97: The diagnostic in failOnPrefabTilemaps uses std.log.err,
which conflicts with the codebase’s stderr-only error reporting and can be
intercepted by the test runner. Replace that logging call with a direct stderr
write, matching the pattern used in scene_manifest.zig and tilemap_scan.zig,
while keeping the same prefab name and asset context before returning
error.PrefabTilemapUnsupported.
In `@src/tilemap_scan_test.zig`:
- Around line 138-142: `imageEmbedPath` currently normalizes parent-relative
`image_source` values even when they escape the assets root, which can lead to
invalid `@embedFile` paths later in generated Zig. Update `collect()` to
validate the resolved path from `imageEmbedPath` (or the source path handling
around it) and reject any `..`-escaping image source with a clear assembler
error before emission. Add a regression test near the existing `imageEmbedPath`
tests to cover an escaping path like "../../../../outside.png" and assert it
fails instead of producing an embed path.
In `@src/tilemap_scan.zig`:
- Around line 76-98: The embed-path helpers allow paths to escape the assets
root because `tmxEmbedPath` and `imageEmbedPath` normalize inputs without
enforcing containment. Update these helpers to validate the normalized result
and reject any `asset_name` or resolved `image_source` path that does not remain
under `assets/`, while still allowing legitimate `../` traversal that stays
within that root. Apply the containment check in the `tmxEmbedPath` and
`imageEmbedPath` flow so generated `@embedFile` paths can never point outside
`assets/`.
---
Nitpick comments:
In `@src/tilemap_scan_test.zig`:
- Around line 356-448: Add a regression test for top-level bundle-array scenes
in scene_manifest parsing. The current tests around parseSceneSource and
scanTilemapAssets cover flat, children, and components-wrapped cases, but miss
the root `[]` path; add a case that feeds a bundle-array scene containing a
Tilemap and asserts the asset is collected. Use the existing test pattern in
scene_manifest.parseSceneSource and/or scene_manifest.scanTilemapAssets so the
array-root behavior is pinned alongside the other Tilemap collection tests.
In `@test/main_zig_tests.zig`:
- Around line 1355-1360: The embedded tilemap registration coverage is missing
the sokol/wasm callback lifecycle path; add a test that uses
h.setSokolLifecycle() and verifies the generated init() emits
g.addEmbeddedTilemapAsset(...) with catch `@panic`(...) instead of only the loop
backend’s try form. Use the existing main_template.tilemap_registrations flow
and the addEmbeddedTilemapAsset assertion pattern so both backend spellings are
covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 326edb8f-7bf5-4b25-9352-8d4376254b30
📒 Files selected for processing (13)
build.zigsrc/codegen/blocks/tilemap_assets.zigsrc/codegen/context.zigsrc/codegen/lifecycle/callback.zigsrc/codegen/lifecycle/loop.zigsrc/codegen/main_template.zigsrc/root.zigsrc/root/tilemap_phase.zigsrc/scene_manifest.zigsrc/tilemap_scan.zigsrc/tilemap_scan_test.zigsrc/tilemap_scene_scan.zigtest/main_zig_tests.zig
| fn failOnPrefabTilemaps( | ||
| allocator: std.mem.Allocator, | ||
| target_dir: []const u8, | ||
| prefab_names: []const []const u8, | ||
| ) !void { | ||
| const io = config.globalIo(); | ||
| const cwd = std.Io.Dir.cwd(); | ||
| for (prefab_names) |name| { | ||
| const rel = try std.fmt.allocPrint(allocator, "{s}/prefabs/{s}.jsonc", .{ target_dir, name }); | ||
| defer allocator.free(rel); | ||
| const src = cwd.readFileAlloc(io, rel, allocator, .limited(1024 * 1024)) catch continue; | ||
| defer allocator.free(src); | ||
| const assets = try scene_manifest.scanTilemapAssets(allocator, src); | ||
| defer scene_manifest.freeTilemapAssets(allocator, assets); | ||
| if (assets.len > 0) { | ||
| std.log.err( | ||
| "labelle-assembler: prefab '{s}' declares a Tilemap component ('{s}'), but prefab-borne\n" ++ | ||
| " tilemaps are not embedded yet (minimal-T2 is scene-only). Move the Tilemap into a\n" ++ | ||
| " scene, or track full prefab-tilemap support at labelle-assembler#561.", | ||
| .{ name, assets[0] }, | ||
| ); | ||
| return error.PrefabTilemapUnsupported; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
std.log.err contradicts the codebase's stderr-diagnostic convention.
scene_manifest.zig explicitly documents why std.log.err is avoided here: the test runner intercepts it and fails the test. Every other diagnostic in this feature (scene_manifest.zig, tilemap_scan.zig) writes directly to stderr instead. This function's std.log.err call breaks that convention and would turn a legitimate error.PrefabTilemapUnsupported exercise into a spurious test failure.
🔧 Proposed fix
if (assets.len > 0) {
- std.log.err(
- "labelle-assembler: prefab '{s}' declares a Tilemap component ('{s}'), but prefab-borne\n" ++
- " tilemaps are not embedded yet (minimal-T2 is scene-only). Move the Tilemap into a\n" ++
- " scene, or track full prefab-tilemap support at labelle-assembler#561.",
- .{ name, assets[0] },
- );
+ var buf: [512]u8 = undefined;
+ if (std.fmt.bufPrint(&buf,
+ "labelle-assembler: prefab '{s}' declares a Tilemap component ('{s}'), but prefab-borne\n" ++
+ " tilemaps are not embedded yet (minimal-T2 is scene-only). Move the Tilemap into a\n" ++
+ " scene, or track full prefab-tilemap support at labelle-assembler#561.\n",
+ .{ name, assets[0] },
+ )) |formatted| {
+ std.Io.File.stderr().writeStreamingAll(io, formatted) catch {};
+ } else |_| {}
return error.PrefabTilemapUnsupported;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn failOnPrefabTilemaps( | |
| allocator: std.mem.Allocator, | |
| target_dir: []const u8, | |
| prefab_names: []const []const u8, | |
| ) !void { | |
| const io = config.globalIo(); | |
| const cwd = std.Io.Dir.cwd(); | |
| for (prefab_names) |name| { | |
| const rel = try std.fmt.allocPrint(allocator, "{s}/prefabs/{s}.jsonc", .{ target_dir, name }); | |
| defer allocator.free(rel); | |
| const src = cwd.readFileAlloc(io, rel, allocator, .limited(1024 * 1024)) catch continue; | |
| defer allocator.free(src); | |
| const assets = try scene_manifest.scanTilemapAssets(allocator, src); | |
| defer scene_manifest.freeTilemapAssets(allocator, assets); | |
| if (assets.len > 0) { | |
| std.log.err( | |
| "labelle-assembler: prefab '{s}' declares a Tilemap component ('{s}'), but prefab-borne\n" ++ | |
| " tilemaps are not embedded yet (minimal-T2 is scene-only). Move the Tilemap into a\n" ++ | |
| " scene, or track full prefab-tilemap support at labelle-assembler#561.", | |
| .{ name, assets[0] }, | |
| ); | |
| return error.PrefabTilemapUnsupported; | |
| } | |
| } | |
| } | |
| fn failOnPrefabTilemaps( | |
| allocator: std.mem.Allocator, | |
| target_dir: []const u8, | |
| prefab_names: []const []const u8, | |
| ) !void { | |
| const io = config.globalIo(); | |
| const cwd = std.Io.Dir.cwd(); | |
| for (prefab_names) |name| { | |
| const rel = try std.fmt.allocPrint(allocator, "{s}/prefabs/{s}.jsonc", .{ target_dir, name }); | |
| defer allocator.free(rel); | |
| const src = cwd.readFileAlloc(io, rel, allocator, .limited(1024 * 1024)) catch continue; | |
| defer allocator.free(src); | |
| const assets = try scene_manifest.scanTilemapAssets(allocator, src); | |
| defer scene_manifest.freeTilemapAssets(allocator, assets); | |
| if (assets.len > 0) { | |
| var buf: [512]u8 = undefined; | |
| if (std.fmt.bufPrint(&buf, | |
| "labelle-assembler: prefab '{s}' declares a Tilemap component ('{s}'), but prefab-borne\n" ++ | |
| " tilemaps are not embedded yet (minimal-T2 is scene-only). Move the Tilemap into a\n" ++ | |
| " scene, or track full prefab-tilemap support at labelle-assembler#561.\n", | |
| .{ name, assets[0] }, | |
| )) |formatted| { | |
| std.Io.File.stderr().writeStreamingAll(io, formatted) catch {}; | |
| } else |_| {} | |
| return error.PrefabTilemapUnsupported; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/root/tilemap_phase.zig` around lines 73 - 97, The diagnostic in
failOnPrefabTilemaps uses std.log.err, which conflicts with the codebase’s
stderr-only error reporting and can be intercepted by the test runner. Replace
that logging call with a direct stderr write, matching the pattern used in
scene_manifest.zig and tilemap_scan.zig, while keeping the same prefab name and
asset context before returning error.PrefabTilemapUnsupported.
| test "imageEmbedPath normalises a parent-relative image source" { | ||
| const p = try tilemap_scan.imageEmbedPath(testing.allocator, "assets/maps/level.tmx", "../textures/tiles.png"); | ||
| defer testing.allocator.free(p); | ||
| try testing.expectEqualStrings("assets/textures/tiles.png", p); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd tilemap_scan.zig
rg -n "fn normalizePosix" -A 40 src/tilemap_scan.zigRepository: labelle-toolkit/labelle-assembler
Length of output: 2053
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file first
ast-grep outline src/tilemap_scan.zig --view expanded
# Inspect the functions that build and consume embed paths
rg -n "fn imageEmbedPath|pub fn collect|`@embedFile`|target_dir|embed_path|normalizePosix" -A 80 -B 20 src/tilemap_scan.zig
# Inspect any tests around escaping / invalid paths
rg -n "escape|outside|embed|imageEmbedPath|collect" -A 4 -B 4 src/tilemap_scan_test.zigRepository: labelle-toolkit/labelle-assembler
Length of output: 32773
Reject ..-escaping image_source paths before @embedFile emission
imageEmbedPath can collapse ../../../../outside.png into a path outside assets/; add a regression test and return a clear assembler error in collect() instead of letting generated Zig fail later at @embedFile.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/tilemap_scan_test.zig` around lines 138 - 142, `imageEmbedPath` currently
normalizes parent-relative `image_source` values even when they escape the
assets root, which can lead to invalid `@embedFile` paths later in generated
Zig. Update `collect()` to validate the resolved path from `imageEmbedPath` (or
the source path handling around it) and reject any `..`-escaping image source
with a clear assembler error before emission. Add a regression test near the
existing `imageEmbedPath` tests to cover an escaping path like
"../../../../outside.png" and assert it fails instead of producing an embed
path.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ef789e7b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| while (std.mem.indexOfPos(u8, tmx, search, "<tileset")) |tag_start| { | ||
| const tag_end = std.mem.indexOfPos(u8, tmx, tag_start, ">") orelse tmx.len; | ||
| const tag = tmx[tag_start..tag_end]; | ||
| if (attrValue(tag, "source")) |src| return src; |
There was a problem hiding this comment.
When a valid TMX keeps an old external tileset in an XML comment, e.g. <!-- <tileset firstgid="1" source="old.tsx"/> -->, this raw substring scan still returns old.tsx; collect then aborts with ExternalTilesetUnsupported even though the runtime XML parser ignores the comment and the active map may contain only inline tilesets. The external-tileset detector needs to skip XML comment ranges or use the same XML-aware pass as the engine.
Useful? React with 👍 / 👎.
| const joined = if (base.len == 0) | ||
| try allocator.dupe(u8, image_source) | ||
| else | ||
| try std.fmt.allocPrint(allocator, "{s}/{s}", .{ base, image_source }); | ||
| defer allocator.free(joined); | ||
| return normalizePosix(allocator, joined); |
There was a problem hiding this comment.
When a TMX source uses Windows path separators such as source="tiles\terrain.png", the registry key should remain that exact source string, but the @embedFile path must resolve to the copied asset path (assets/tiles/terrain.png) on non-Windows builders. Passing image_source through unchanged here preserves the backslash, so generated Zig tries to embed a literal filename containing \ and fails on POSIX CI despite the asset existing in the expected directory.
Useful? React with 👍 / 👎.
| const project_registers_tilemap = blk: { | ||
| var pascal_buf: [128]u8 = undefined; | ||
| for (component_names) |name| { | ||
| if (std.mem.eql(u8, idents.pathToPascal(name, &pascal_buf), "Tilemap")) break :blk true; |
There was a problem hiding this comment.
When a decl-module plugin contributes its own Tilemap component, the generated registry uses ComponentRegistryWithPlugins(...), but this check only looks at game-root components/. A scene using that plugin component is therefore treated as the built-in tilemap and generation requires assets/<asset_name>.tmx or embeds an unrelated map if one happens to exist, even though the runtime bridge will deserialize the plugin component instead; the shadowing decision needs to include plugin-registered component names or fail explicitly for that case.
Useful? React with 👍 / 👎.
| while (std.mem.indexOfPos(u8, tmx, search, "<image")) |tag_start| { | ||
| // Bound the attribute search to this element so a later element's | ||
| // `source="..."` can't be attributed to a `<image>` that had none. | ||
| const tag_end = std.mem.indexOfPos(u8, tmx, tag_start, ">") orelse tmx.len; | ||
| const tag = tmx[tag_start..tag_end]; | ||
| if (attrValue(tag, "source")) |src| { |
There was a problem hiding this comment.
When a TMX contains a non-tileset image tag, such as an <imagelayer><image source="draft_bg.png"/></imagelayer>, this scan still registers it even though the engine contract here only fetches images from decoded tilesets. That can make generation require or collide on files the runtime will never request (for example an authoring-only image layer whose source matches a map asset key), so the extractor should only consider <image> elements that belong to inline <tileset> entries.
Useful? React with 👍 / 👎.
- #1 free TMX buffers per iteration: extract the per-map read+scan into `processMap`, so each map's `.tmx` bytes (up to 8 MiB) + extracted image slice free at that frame's end instead of accumulating until `collect` returns. Registrations/keys are dup'd into `regs` first, so they outlive the per-map buffers. Fixes OOM risk for projects with many large maps. - #2 skip XML comments in the tag scan: `indexOfTagSkippingComments` skips `<!-- ... -->` spans, so a commented-out `<image>`/`<tileset source>` no longer emits a bogus @embedfile (which could fail the build) or a false external-tileset error. - #3 include pack prefabs in the prefab fail-loud: move the tilemap phase after `loadPackScans` and thread `pack_scans` into `collectRegistrations`; `failOnPrefabTilemaps` now also walks `<import_prefix>/prefabs/*.jsonc`, so a Tilemap in a light-pack prefab aborts with the same #561 message instead of shipping a silently-broken binary. Updated #561 body. Tests: commented-out <image>/<tileset> ignored (real one still found); existing collect tests exercise per-map frees under testing.allocator. Verified e2e: pack-prefab Tilemap fails loud; happy-path unchanged + ast-checks. zig build + test green, goldens byte-identical, fmt clean, files < 1000. Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
…ckslash key/path) + revert unescape per gfx Grounded in gfx v1.21.0 tilemap/src/root.zig: its TMX parser stores `image_source` as a RAW dupe of the attribute-value bytes (parseAttributes reads verbatim between double-quotes; `tileset.image_source = dupe(src)`) — NO XML-entity decoding, NO separator normalization. The engine's ImageProvider.get looks up by that raw string, so the registry key must match it byte-for-byte. - #A tileset-scoped image scan: `extractImageSources` now only collects `<image>` INSIDE a `<tileset>…</tileset>` span. An `<imagelayer><image/>` background is ignored — the engine fetches images only for decoded TILESETS, so embedding an imagelayer image would require an absent file / collide though the runtime never requests it. - #B Windows backslash: the registry KEY stays the RAW `image_source` (backslash intact, matching gfx's lookup); only the @embedfile PATH normalizes `\`→`/` (+ existing `.`/`..` collapse) so it resolves to the copied asset `assets/tiles/terrain.png`. - Revert round-2 xmlUnescape: gfx does NOT decode entities, so decoding the key was a silent-mismatch bug (engine keys by raw `&…`). Keys/paths are now raw; the ambiguous XML-entity-in-path edge is deferred + filed as assembler#564. Tests: imagelayer image ignored (tileset image still found); raw source preserved (no decode); backslash → raw key + normalized path (unit + collect e2e). zig build + test green, goldens byte-identical, fmt clean, files < 1000. Happy-path fixture regenerates identical + ast-checks. Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
… embedding (T2 Phase 4, #560) (#565) Scans scenes for Tilemap components and comptime-embeds the referenced .tmx + its tileset images via engine game.addEmbeddedTilemapAsset. Default framework pins moved to the compatible trio core 1.24.0 / engine 1.75.1 / gfx 1.21.0. Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
…592) (#595) Engine v2.0.0 (#592) removed three pre-#560 legacy aliases; legacy example scenes now hard-fail at load with error.InvalidFormat, breaking the "Examples integration test" CI job on asset-streaming-smoke. Migrate every affected example scene to the unified format: - top-level "entities" -> "children" - non-empty top-level "assets" -> bundle "meta.assets" header - empty top-level "assets" -> removed No prefab reference carried a "components" wrapper, so rule 2 (reference "components" -> "overrides") had no sites. fantasy-dungeon and camera-builtin were already unified and left untouched. Verified: asset-streaming-smoke generates + builds + runs headless on the null backend against engine v2.0.0 (exit 0), with SceneAssetManifests.main = &.{ "sprites", "jump" } preserved from the bundle meta.assets header. Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
…relay) (#610) Advance the #607 showcase set with three more small complete games, each scaffolded following the parallax-scroll conventions and verified end to end (generate -> zig build -> deterministic headless run on .null): - coin-collector: a full gameplay loop — per-frame ECS queries, homing movement via getPosition/setPosition, radius-overlap pickup, destroyEntity, and a win condition. Clears the 5-coin field by frame 87 and logs `[collector] cleared` (verified). - ruby-orbit: the ticket's Ruby scripting seed — a ruby/orbit.rb script drives the real engine through the Script Runtime Contract via the labelle-scripting plugin (local sibling, language=ruby) over the labelle-null sibling backend. Emits the exact ordered RUBY_* transcript (verified after compiling the vendored mruby). - event-relay: the pure-Zig game event bus — a script emits a custom Pulse event (events/pulse.zig) that a game-root hook (hooks/pulse_watcher.zig) receives at dispatchEvents, proving the assembler's events/ + hooks/ auto-scan end to end (verified). Also: - Fix parallax-scroll for engine v2.0 (#592): the pre-#560 top-level "entities" scene key is now REJECTED at runtime; migrate it (and author the new scenes) with file-level entities under "children". - Update docs/showcase-plan.md: expand the grid to reflect the games now in-repo (incl. camera-builtin / fantasy-dungeon / scripting-smoke that landed separately), document the unified scene-format requirement, and note the labelle-null-sibling pin the scripting games need. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
What
Phase 4 of the minimal-T2 tilemap epic: the assembler now scans scenes for
Tilemapcomponents and comptime-embeds the referenced.tmxmap plus its tileset images, registering them so labelle-engine v1.75.0 can decode + render the tilemap. Purely additive — no change to existing scene/atlas/prefab embedding.Engine contract targeted (labelle-engine v1.75.0, T2 Phase 2)
A single registry on
Game,embedded_tilemap_sources, populated via:keyed by BOTH:
asset_name→ the raw.tmxbytes, andimage_source(the exact<image source="...">string) → that image's bytes.At load the engine decodes the
.tmx(TileMap.loadFromMemoryWithBasePath(alloc, bytes, "")) and, per tileset, callsImageProvider.get(image_source)— which reads back from this same registry. So the image registry key must be the byte-identicalimage_sourcestring (verified against the engine test:<image source="tiles.png">→image_source == "tiles.png", registered as"level.tmx"/"tiles.png"verbatim).How
scene_manifest.zig— newSceneManifest.tilemap_assets; a deep-walk of the parsed scene JSON collects everyTilemapcomponent'sasset_name(handles flat-form,componentswrapper,children/entities, and bundle shapes).tilemap_scan.zig(new) — resolves eachasset_nametoassets/<asset_name>.tmx, reads it, extracts every<image source>(tight XML scan; external<tileset source=".tsx">skipped — no inline image), and builds a deduped flat list ofRegistration { key, embed_path }. Image key = verbatimimage_source;@embedFilepath = resolved relative to the.tmx's dir.codegen/blocks/tilemap_assets.zig(new) — emits theaddEmbeddedTilemapAssetcalls (tryfor loop backends,catch @panicfor sokol/wasm callback), beforesetScenein both lifecycle paths.root.zig/main_template.zig/context.zig— aggregate +collectafter theassets/link, thread via the module-level-var pattern used bypack_scans.build.zig— defaultengine_version1.60.0 → 1.75.0.Path convention
asset_name→assets/<asset_name>.tmx(an explicit.tmxsuffix is respected). Registry key stays the verbatimasset_name. Tileset image key stays the verbatimimage_source; its@embedFilepath is resolved relative to the.tmxdir.Verification
zig build+zig build testgreen (0.16.0);zig fmtclean on all source. Files under 1000 lines.collect(tmpDir e2e + dedup + missing-asset error), scene extraction shapes, and both emit spellings.Tilemapscene + hand-authored CSV.tmx+ tiny tileset PNG generates both registrations beforesetScene, links the assets into the target tree, and the generatedmain.zigpasseszig ast-check.Do NOT merge — release/pin choreography handled separately.
https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
Summary by CodeRabbit
New Features
Bug Fixes
Tests