Repository navigation
tilemap: engine Tilemap component + .tmx loader + post-sprite render pass (T2 Phase 2) - #703
Conversation
The engine module itself takes NO labelle-gfx dependency — it reaches tilemap types through the renderer plugin (`RenderImpl.TileMapRendererType`), exactly as it reaches Sprite/Shape/`drawMesh`. This lazy pin is consumed only by the T2 tilemap test to decode a real `.tmx` through gfx's std-only `tilemap` sub-package and drive `TileMapRendererWith(MockBackend)` post-sprite, proving the whole path against the shipped gfx 1.21.0 API. Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
…pass (T2 Phase 2)
Adds a minimal, immutable `Tilemap` component that references an embedded
`.tmx` asset by name, decodes it through the gfx 1.21.0 tilemap API, and
renders it as a POST-SPRITE pass — the engine owns pass ordering (entities
first, tilemaps after); gfx's RetainedEngine is untouched.
The engine module stays gfx-free: it reaches gfx's `TileMap` decoder +
`TileMapRenderer` through the renderer plugin (`RenderImpl.TileMapRendererType`,
`loadTextureFromMemory`, `getTextureInfo`), exactly as it reaches
Sprite/Shape and the drawMesh seam. All the reflection is centralised in
`src/tilemap_runtime.zig`; the whole feature is comptime-gated on
`tilemap_supported` so stub renderers see a void side table + no-op methods
(purely additive).
- `Tilemap` component (`src/tilemap.zig`) — POD `{ asset_name }`; the
decoded map + per-entity draw-pass renderer live in a Game-side side
table (`tilemaps`), keyed by entity, heap-owned for pointer stability.
- `addTilemap` mixin (`src/game/tilemap_mixin.zig`) — decodes behind the
embedded tilemap-asset registry (`addEmbeddedTilemapAsset`), resolves
each tileset image through the SAME backend texture path sprites use
(the gfx 1.21.0 resolver seam, load_unresolved_from_filesystem = false).
- Scene loader: `{"Tilemap": {"asset_name": "..."}}` → `addTilemap`.
- Render orchestration: `renderTilemaps()` after `renderer.render()` in the
frame loop, gated by the post-load render gate.
- Save/load: built-in channel (like PrefabInstance — `[]const u8` can't
round-trip via registry serde) persists ONLY `asset_name`; load
rehydrates the runtime by re-decoding. Tilemap-only entities are swept
into the save set. ECS reset frees stale runtimes (`clearTilemaps`).
- Editor digest: per-entity `"tilemap":"<asset_name>"` presence flag.
Test (`test/tilemap_test.zig`) drives the ACTUALLY-shipped gfx 1.21.0
tilemap sub-package + `TileMapRendererWith(MockBackend)` (lazy test-only
pin) headless: decode, post-sprite ordering + world offset, save→load
round-trip, digest. 745/745 engine tests pass; zig fmt clean.
Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
|
Warning Review limit reached
Next review available in: 22 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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR adds a built-in ChangesTilemap Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Loader as loadGameState
participant Game
participant TilemapMixin
participant Runtime as tilemap_runtime.Runtime
Loader->>Game: addTilemap(entity, .{asset_name})
Game->>TilemapMixin: addTilemap
TilemapMixin->>TilemapMixin: acquireTilemap
TilemapMixin->>Runtime: initInPlace(tmx_bytes, images)
Runtime-->>TilemapMixin: decoded map stored
Game->>TilemapMixin: renderTilemaps (post-sprite)
TilemapMixin->>Runtime: draw(camera, position)
Game->>TilemapMixin: releaseTilemap(entity)
TilemapMixin->>Runtime: deinit
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the built-in "Tilemap" component (T2 Phase 2), integrating ".tmx" decoding, a post-sprite rendering pass, and save/load persistence into the engine. The feedback highlights a critical issue in "src/tilemap_runtime.zig" where a stack-allocated resolver context creates a dangling pointer, which should be resolved by storing it as a field in the "Runtime" struct. Additionally, it is recommended to convert file extensions to lowercase in "fileTypeZ" to ensure compatibility with image decoders.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| /// Per-tileset catalog texture id (null = image unresolved → | ||
| /// that tileset draws nothing). Read by the resolver trampoline. | ||
| tileset_ids: []?TextureId, | ||
|
|
There was a problem hiding this comment.
Add the resolver_ctx field to the Runtime struct to prevent the dangling stack pointer issue in initInPlace.
resolver_ctx: ResolverCtx,
| self.* = .{ | ||
| .allocator = allocator, | ||
| .renderer = renderer, | ||
| .map = map, | ||
| .tm = undefined, | ||
| .tileset_ids = ids, | ||
| }; | ||
|
|
||
| var ctx = ResolverCtx{ .renderer = renderer, .ids = self.tileset_ids }; | ||
| self.tm = try TmRenderer.initWithOptions(allocator, &self.map, .{ | ||
| .resolver = Resolver{ .context = &ctx, .resolveFn = resolveTexture }, | ||
| // Embedded env: never touch the filesystem for unresolved | ||
| // tilesets — the resolver is the only texture source. | ||
| .load_unresolved_from_filesystem = false, | ||
| }); |
There was a problem hiding this comment.
The local variable ctx is allocated on the stack of initInPlace. Passing its address (&ctx) to TmRenderer.initWithOptions creates a dangling pointer once initInPlace returns. When draw is later called, the resolver will access this garbage stack memory, leading to undefined behavior or crashes. To fix this, store resolver_ctx as a field in the Runtime struct so it has a stable lifetime matching the heap-allocated Runtime instance.
self.* = .{
.allocator = allocator,
.renderer = renderer,
.map = map,
.tm = undefined,
.tileset_ids = ids,
.resolver_ctx = .{
.renderer = renderer,
.ids = ids,
},
};
self.tm = try TmRenderer.initWithOptions(allocator, &self.map, .{
.resolver = Resolver{ .context = &self.resolver_ctx, .resolveFn = resolveTexture },
// Embedded env: never touch the filesystem for unresolved
// tilesets — the resolver is the only texture source.
.load_unresolved_from_filesystem = false,
});
| fn fileTypeZ(allocator: std.mem.Allocator, image_source: []const u8) ![:0]const u8 { | ||
| const dot = std.mem.lastIndexOfScalar(u8, image_source, '.'); | ||
| const ext = if (dot) |d| image_source[d + 1 ..] else ""; | ||
| const chosen = if (ext.len == 0) "png" else ext; | ||
| return allocator.dupeZ(u8, chosen); | ||
| } |
There was a problem hiding this comment.
The docstring states that fileTypeZ returns a 'lowercase-ish file-type token', but it currently duplicates the extension exactly as-is (e.g., .PNG remains PNG). Since many image decoders and texture loaders expect lowercase file types, convert the duplicated extension to lowercase using std.ascii.toLower to ensure robustness.
fn fileTypeZ(allocator: std.mem.Allocator, image_source: []const u8) ![:0]const u8 {
const dot = std.mem.lastIndexOfScalar(u8, image_source, '.');
const ext = if (dot) |d| image_source[d + 1 ..] else "";
const chosen = if (ext.len == 0) "png" else ext;
const duped = try allocator.dupeZ(u8, chosen);
for (duped) |*c| {
c.* = std.ascii.toLower(c.*);
}
return duped;
}
…view) gemini flagged the resolver context (passed by &ctx to initWithOptions) as a dangling stack pointer. Verified against gfx v1.21.0: initWithOptions resolves tileset textures EAGERLY and does not retain the resolver, so &ctx was safe today. But that made the engine silently depend on gfx's resolution timing across the repo boundary — a future switch to lazy resolution would be a UAF with no compile error. Store the ctx inline in the heap-stable Runtime so it's correct-by-construction regardless. Also lowercase fileTypeZ's token so a .PNG tileset resolves the same decoder as .png. Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
|
Addressed in fdd9832. Dangling-pointer finding (lines 88/146): verified against gfx v1.21.0 — Lowercase token (line 187): applied |
![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: bf32c1b301
ℹ️ 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".
| pub fn deinit(self: *Self) void { | ||
| self.tm.deinit(); | ||
| self.map.deinit(); | ||
| self.allocator.free(self.tileset_ids); |
There was a problem hiding this comment.
When a scene is unloaded or a Tilemap entity is destroyed, releaseTilemap reaches this deinit path, but the tileset textures uploaded in initInPlace via renderer.loadTextureFromMemory are never released. Because those textures are handed to gfx as resolver-supplied/unowned entries, self.tm.deinit() leaves them in the renderer texture table until the renderer itself is destroyed, so repeated scene reloads or multiple tilemap entities keep allocating duplicate GPU textures for the same tileset images.
Useful? React with 👍 / 👎.
| /// pointers so the gfx tilemap renderer's `*const TileMap` into the | ||
| /// runtime stays stable. `void` on renderers without the tilemap | ||
| /// seam. Owned here; freed in `deinitTilemaps` / `releaseTilemap`. | ||
| tilemaps: if (tilemap_supported) std.AutoHashMap(Entity, *TilemapRuntimeType) else void, |
There was a problem hiding this comment.
In games that use createWorld/setActiveWorld, this single Game-level side table is not swapped with active_world even though the ECS and renderer are. After shelving a world that has tilemaps, renderTilemaps still iterates those old entity ids against the newly active ECS, so stale tilemaps can draw in the wrong world (or at default/unrelated positions), and destroyWorld/resetEcsBackend cannot clean up the correct world's tilemap state independently.
Useful? React with 👍 / 👎.
| const pos = self.getWorldPosition(e.key_ptr.*); | ||
| e.value_ptr.*.draw(0, 0, pos.x, pos.y, null, null); |
There was a problem hiding this comment.
For projects using the default GameConfig / .up Y-axis, the retained renderer flips sprite positions through the engine's core.toScreenY path, but the tilemap pass sends the logical world Position directly to the backend as a screen-space offset. Even with camera offsets intentionally deferred to T3, this makes tilemaps render vertically mirrored/misaligned relative to sprites, gizmos, and input picking in existing y-up games.
Useful? React with 👍 / 👎.
| /// that lands. | ||
| pub fn renderTilemaps(self: *Game) void { | ||
| if (comptime !supported) return; | ||
| var it = self.tilemaps.iterator(); |
There was a problem hiding this comment.
If gameplay removes Game.TilemapComp through the existing generic removeComponent API, the ECS component disappears but nothing calls releaseTilemap, and this pass keeps drawing from the side table anyway. Since there is no removeTilemap counterpart to addTilemap, that leaves a removed tilemap visible until the entity is destroyed or the ECS is reset.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
build.zig (1)
297-328: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
test/tilemap_test.zigisn't wired through thetest_filesarray.As per coding guidelines, "Wire each
src/foo.zig+test/foo_test.zigpair explicitly inbuild.zig'stest_filesarray." This test is instead wired ad hoc inside thelazyDependencyblock because it needs an extra"tilemap"import the generic loop doesn't support — same pattern already used foriosurface_test/assets_testsabove, so it's not unprecedented in this file, but it does deviate from the stated convention.🤖 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 `@build.zig` around lines 297 - 328, The tilemap test is being added outside the standard test_files wiring, so make build.zig register test/tilemap_test.zig explicitly in the test_files array like the other src/foo.zig + test/foo_test.zig pairs. Keep the special tilemap_module import handling, but move or mirror the test registration so Tilemap test coverage follows the established convention rather than only being created inside the lazyDependency("labelle_gfx") block.Source: Coding guidelines
src/jsonc/component_apply.zig (1)
115-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test coverage for the new JSONC
"Tilemap"apply path.The decode/render/save-load/digest tests in
test/tilemap_test.zigall constructTilemapviagame.addTilemapdirectly; none exercise thisapplyComponentbranch (i.e., a scene authored in.jsoncwith a"Tilemap"component key). A regression here (dispatch ordering, deserializer field mismatch) wouldn't be caught by current tests.Want me to draft a jsonc-scene-loading test reusing the existing
MockRender/TilemapGamescaffolding fromtest/tilemap_test.zig?🤖 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/jsonc/component_apply.zig` around lines 115 - 123, Add a test that exercises the new applyComponent "Tilemap" branch instead of only using game.addTilemap directly. Reuse the existing MockRender and TilemapGame scaffolding from test/tilemap_test.zig, load a scene/component payload through the JSONC path, and verify that applyComponent dispatches the "Tilemap" case and produces the same tilemap/render/save-load behavior. Cover the Tilemap-specific path in src/jsonc/component_apply.zig so regressions in dispatch ordering or GameType.TilemapComp deserialization are caught.
🤖 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/game/tilemap_mixin.zig`:
- Around line 60-83: The global Game.tilemaps side table is not being cleared or
migrated when worlds change, which can leave stale Runtime entries tied to the
old world’s RenderImpl. Update setActiveWorld and destroyWorld to purge
tilemaps, or move the tilemap cache onto World so each world owns its own
runtimes. Make sure renderTilemaps only sees runtimes for the currently active
world and that acquireTilemap/releaseTilemap continue to manage entries
consistently.
In `@src/root.zig`:
- Around line 61-66: The Tilemap subsystem is only exposing the component type,
while tilemap_runtime.zig’s public API remains hidden from the engine surface.
Update src/root.zig to add a tilemap_runtime_mod re-export alongside Tilemap,
using the same pattern as tween_mod/tween_tick_mod and animation_events_mod, so
supported, ImageProvider, and Runtime are available through engine. Keep the new
export grouped under the existing Tilemap section and reference the
tilemap_runtime.zig module import directly.
---
Nitpick comments:
In `@build.zig`:
- Around line 297-328: The tilemap test is being added outside the standard
test_files wiring, so make build.zig register test/tilemap_test.zig explicitly
in the test_files array like the other src/foo.zig + test/foo_test.zig pairs.
Keep the special tilemap_module import handling, but move or mirror the test
registration so Tilemap test coverage follows the established convention rather
than only being created inside the lazyDependency("labelle_gfx") block.
In `@src/jsonc/component_apply.zig`:
- Around line 115-123: Add a test that exercises the new applyComponent
"Tilemap" branch instead of only using game.addTilemap directly. Reuse the
existing MockRender and TilemapGame scaffolding from test/tilemap_test.zig, load
a scene/component payload through the JSONC path, and verify that applyComponent
dispatches the "Tilemap" case and produces the same tilemap/render/save-load
behavior. Cover the Tilemap-specific path in src/jsonc/component_apply.zig so
regressions in dispatch ordering or GameType.TilemapComp deserialization are
caught.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 80efe74f-cf62-41a0-8ecc-c1969eda1225
📒 Files selected for processing (17)
build.zigbuild.zig.zonsrc/editor_api.zigsrc/game.zigsrc/game/entity_mixin.zigsrc/game/game_init.zigsrc/game/lifecycle_mixin.zigsrc/game/loop_mixin.zigsrc/game/save_load/load.zigsrc/game/save_load/save.zigsrc/game/tilemap_mixin.zigsrc/game/world_mixin.zigsrc/jsonc/component_apply.zigsrc/root.zigsrc/tilemap.zigsrc/tilemap_runtime.zigtest/tilemap_test.zig
| // ── Tilemap (T2 Phase 2) ── | ||
| /// Engine built-in `Tilemap` component — references an embedded `.tmx` | ||
| /// asset by name. Reachable on a configured game as `Game.TilemapComp`. | ||
| /// See `src/tilemap.zig` / `src/tilemap_runtime.zig`. | ||
| pub const Tilemap = @import("tilemap.zig").Tilemap; | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
tilemap_runtime.zig's public API isn't re-exported on engine.
Tilemap the component is correctly re-exported here, but tilemap_runtime.zig also declares pub fn supported, pub const ImageProvider, and pub fn Runtime that aren't surfaced anywhere in this file. Other similarly-shaped subsystems (e.g. tween_mod/tween_tick_mod, animation_events_mod) expose both the component-level type and the underlying module. Consider adding a tilemap_runtime_mod re-export alongside Tilemap for consistency.
As per coding guidelines, "Add every new public type or function to src/root.zig so it is re-exported on the public engine surface as engine.Foo, grouped under a // ── Section ── heading."
♻️ Proposed addition
// ── Tilemap (T2 Phase 2) ──
/// Engine built-in `Tilemap` component — references an embedded `.tmx`
/// asset by name. Reachable on a configured game as `Game.TilemapComp`.
/// See `src/tilemap.zig` / `src/tilemap_runtime.zig`.
pub const Tilemap = `@import`("tilemap.zig").Tilemap;
+/// Renderer-agnostic tilemap decode/draw runtime module — the feature
+/// gate (`supported`), the bytes-provider seam (`ImageProvider`), and
+/// the per-renderer `Runtime(RenderImpl)` type. See
+/// `Game.tilemap_supported` / `Game.TilemapRuntimeType` for the
+/// game-bound aliases.
+pub const tilemap_runtime_mod = `@import`("tilemap_runtime.zig");📝 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.
| // ── Tilemap (T2 Phase 2) ── | |
| /// Engine built-in `Tilemap` component — references an embedded `.tmx` | |
| /// asset by name. Reachable on a configured game as `Game.TilemapComp`. | |
| /// See `src/tilemap.zig` / `src/tilemap_runtime.zig`. | |
| pub const Tilemap = @import("tilemap.zig").Tilemap; | |
| // ── Tilemap (T2 Phase 2) ── | |
| /// Engine built-in `Tilemap` component — references an embedded `.tmx` | |
| /// asset by name. Reachable on a configured game as `Game.TilemapComp`. | |
| /// See `src/tilemap.zig` / `src/tilemap_runtime.zig`. | |
| pub const Tilemap = `@import`("tilemap.zig").Tilemap; | |
| /// Renderer-agnostic tilemap decode/draw runtime module — the feature | |
| /// gate (`supported`), the bytes-provider seam (`ImageProvider`), and | |
| /// the per-renderer `Runtime(RenderImpl)` type. See | |
| /// `Game.tilemap_supported` / `Game.TilemapRuntimeType` for the | |
| /// game-bound aliases. | |
| pub const tilemap_runtime_mod = `@import`("tilemap_runtime.zig"); |
🤖 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.zig` around lines 61 - 66, The Tilemap subsystem is only exposing
the component type, while tilemap_runtime.zig’s public API remains hidden from
the engine surface. Update src/root.zig to add a tilemap_runtime_mod re-export
alongside Tilemap, using the same pattern as tween_mod/tween_tick_mod and
animation_events_mod, so supported, ImageProvider, and Runtime are available
through engine. Keep the new export grouped under the existing Tilemap section
and reference the tilemap_runtime.zig module import directly.
Source: Coding guidelines
…move, F2 guard) F1 — tileset textures leaked on teardown. The runtime uploads them via renderer.loadTextureFromMemory and hands them to gfx as UNOWNED, so tm.deinit never released them. Runtime.deinit now unloads each (deduped) tileset id through renderer.unloadTexture (added to the `supported` gate), and lifecycle deinit runs deinitTilemaps BEFORE the renderer teardown so the unload seam is still alive. F3 — tilemaps rendered vertically mismatched vs sprites in `.up` projects. renderTilemaps now flips the map's world offset through the SAME core.toScreenY sprites use (reading the renderer's logical screen_height), raising by the map's pixel height under `.up` so a tilemap and a sprite at the same Position.y align; identity under `.down`. Camera-transform alignment stays deferred to T3. F4 — no removeTilemap counterpart to addTilemap, so generic removeComponent orphaned the side-table runtime (kept drawing). Added removeTilemap (releaseTilemap + removeComponent) and made renderTilemaps reap any side-table entry whose entity no longer carries Tilemap in the active ECS (alloc-free, restart-on-remove). F2 — deferred + guarded + filed (#704). The Game-scoped side table isn't swapped with active_world; the render reap now skips ids not live in the currently active ECS, resetEcsBackend already clears the table via clearTilemaps, and the single-world limitation is documented at the field. +5 regression tests (750/750 pass): y-axis alignment (.up matches the sprite transform; .down identity), texture release/no-leak + re-acquire stability, removeTilemap teardown, and generic-removeComponent ghost reap. Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
|
Disposition of the two CodeRabbit findings (both on fdd9832, now on head 00fbf9d):
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00fbf9d920
ℹ️ 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".
| if (!self.ecs_backend.hasComponent(e.key_ptr.*, Tilemap)) { | ||
| const rt = e.value_ptr.*; | ||
| _ = self.tilemaps.remove(e.key_ptr.*); |
There was a problem hiding this comment.
In games that use createWorld/setActiveWorld with tilemaps in multiple worlds, this guard only checks whether the currently active ECS has a Tilemap at the same numeric entity id. If it does, a runtime that belongs to the shelved world is kept and drawn at the active world's position; if it doesn't, the shelved runtime is freed, so switching back leaves that world's entity with a Tilemap component but no runtime to draw. Fresh evidence beyond the prior per-world concern is this reap path: it makes the Game-wide table alias or destruct inactive-world tilemaps during ordinary world swaps instead of preserving them.
Useful? React with 👍 / 👎.
| if (std.mem.eql(u8, name, "Tilemap")) { | ||
| if (deserializer.deserialize(GameType.TilemapComp, value, comp_alloc)) |tilemap| { | ||
| game.addTilemap(entity, tilemap); |
There was a problem hiding this comment.
For existing projects that already have a registry component whose serialized name is Tilemap, this new built-in branch runs before the Components.names() dispatch and returns unconditionally. Since GameType.TilemapComp.asset_name has a default, custom Tilemap JSON that lacks asset_name still deserializes as the engine tilemap with an empty asset name, so the registered component is silently not added and scene loads lose that component data.
Useful? React with 👍 / 👎.
…t (C1)
C2 — a project-registered component named `Tilemap` was silently shadowed
by the engine built-in: the built-in scene-loader branch ran before the
registry dispatch and returned unconditionally, so custom `Tilemap` JSON
deserialized as an empty engine tilemap (asset_name defaults to "") and the
registered component never loaded — silent scene-data loss. The built-in
branch is now comptime-gated on `!Components.has("Tilemap")`, so a
registered `Tilemap` falls through to the registry dispatch and WINS.
Regression test: test/jsonc/tilemap_precedence_test.zig asserts a registered
`Tilemap` loads with its real fields and the built-in is not attached.
C1 — documented + deferred into #704. The F4 ghost-reap can't tell
"component removed" from "entity belongs to a shelved world" (the table is
Game-global), so a raw world swap would destruct a shelved world's runtime.
Added a caveat at reapGhostTilemaps noting the single-active-world
assumption + pointing at #704, and expanded the #704 body to call out this
reap-path destruction/aliasing (not just stale draws). Single-world
behavior is unchanged (correct for minimal-T2).
751/751 tests pass; zig fmt clean.
Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
…st-sprite render pass (T2 Phase 2, #703) (#705) Pins labelle-gfx v1.21.0. Adds the engine Tilemap component (asset_name, Saveable), the .tmx embedded-asset loader, addTilemap/removeTilemap, scene-loader support, the post-sprite render pass (y-axis-aligned with sprites), and editor digest presence. Additive; renderer-agnostic (reaches gfx via the RenderImpl seam). 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: 733d46cd88
ℹ️ 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".
| if (comptime !Common.isRegistered(TilemapT_load)) { | ||
| if (components.get("Tilemap")) |tm_val| blk: { |
There was a problem hiding this comment.
When a game registers its own component named Tilemap and that save data includes an asset_name string, the registry loop above restores the custom component, then this type-identity guard still lets the engine built-in branch run because Game.TilemapComp itself is not registered. That attaches a second engine Tilemap/runtime and a later save can emit duplicate "Tilemap" keys, so the C2 precedence fix is not preserved for save/load. Fresh evidence beyond the prior scene-loader finding is that loadGameState still checks Common.isRegistered(TilemapT_load) instead of the registry name.
Useful? React with 👍 / 👎.
| //! **Engine built-in, NOT a `ComponentRegistry` component.** Like | ||
| //! `Position` and `PrefabInstance`, `Tilemap` is handled by dedicated | ||
| //! built-in channels in the scene loader, save/load, and digest — its | ||
| //! `asset_name` is a `[]const u8`, which the registry-driven `serde` | ||
| //! path cannot round-trip. Do not register it in a game's |
There was a problem hiding this comment.
This declares Tilemap as a built-in that should not be registered, but only the JSONC loader was taught about it; the comptime scene writer still special-cases only Sprite/Shape before falling through to Components.has (scene/src/entity_writer.zig:55-62). In projects still using .zon/comptime scenes, a .Tilemap = .{ .asset_name = ... } field therefore cannot be registered and is silently ignored, so those scenes load without the component or decoded runtime.
Useful? React with 👍 / 👎.
| defer allocator.free(ft); | ||
| // A missing/undecodable tileset image degrades to "this | ||
| // tileset draws nothing" rather than failing the whole map. | ||
| ids[i] = renderer.loadTextureFromMemory(ft, bytes) catch null; |
There was a problem hiding this comment.
If any error occurs after one or more tileset images have been uploaded—e.g. a later fileTypeZ allocation fails or TmRenderer.initWithOptions returns OOM while inserting resolver textures—the active errdefers only free ids and map. acquireTilemap then destroys the runtime without calling deinit, so the renderer keeps those uploaded texture ids even though the tilemap failed to acquire; add an error cleanup that unloads every non-null id recorded here.
Useful? React with 👍 / 👎.
1 participant
What
Phase 2 of the minimal T2 tilemap epic: a real, immutable
Tilemapcomponent that references an embedded.tmxasset by name, decodes it through the gfx 1.21.0 tilemap API, and renders it as a POST-SPRITE pass. The engine owns pass ordering (entities first, tilemaps after); gfx'sRetainedEngineis untouched.Load-bearing design decision (the render-orchestration unknown)
The engine module takes NO labelle-gfx dependency — the existing invariant (
game.zig: "The engine does NOT depend on labelle-gfx — it accesses component types through RenderImpl"). The design brief assumed a directgfx.TileMap/ gfx pin on the module; grounding againstorigin/mainshowed that would break the invariant and the "purely additive" bar.Instead, the engine reaches gfx's
TileMapdecoder +TileMapRendererthrough the renderer plugin —RenderImpl.TileMapRendererType,loadTextureFromMemory,getTextureInfo— exactly as it reachesSprite/Shapeand thedrawMeshseam. All the type reflection (namingTileMap,Tileset, the backendTexture, the resolver) is centralised insrc/tilemap_runtime.zig. The whole feature is comptime-gated ontilemap_supported, so stub renderers get avoidside table + no-op methods — zero cost, no behaviour change for existing consumers.Texture crux: the engine catalog stores
u32texture ids, but the gfx resolver seam needs aBackend.Texture. Bridged via the SAME path sprites use —RenderImpl.loadTextureFromMemory(upload) →getTextureInfo(id).backend_texture(resolve) — so tileset images share the sprite texture registry and are unowned by the tilemap renderer (load_unresolved_from_filesystem = false).Asset lifecycle: a
LoaderKind.tilemapcatalog variant would touch ~13 files / ~86 exhaustive switch sites AND can't hold a gfxTileMapin the gfx-agnosticDecodedPayloadunion — that fails "minimal/additive". Used a dedicated embedded-source registry (embedded_tilemap_sources+addEmbeddedTilemapAsset), same idiom asembedded_scene_sources.Save:
Tilemap.asset_nameis[]const u8, which the registry-driven serde can't round-trip — so it lives in the built-in save channel alongsidePosition/PrefabInstance, persisting onlyasset_name. Load rehydrates the runtime by re-decoding. Tilemap-only entities are swept into the save set; ECS reset frees stale runtimes.The gfx pin (
build.zig.zon) is lazy + test-only: consumed solely by the tilemap test to drive the real shipped gfx 1.21.0 tilemap sub-package headless.Files
src/tilemap.zig,src/tilemap_runtime.zig,src/game/tilemap_mixin.zig,test/tilemap_test.ziggame.zig,game_init.zig,lifecycle_mixin.zig,world_mixin.zig,entity_mixin.zig,loop_mixin.zig,jsonc/component_apply.zig,editor_api.zig,save_load/{save,load}.zig,root.zig,build.zig,build.zig.zonTests
745/745engine tests pass (zig build test), incl. 5 new: seam detection,.tmxdecode, post-sprite ordering + world offset, save→load round-trip ofasset_name, digest presence.zig fmtclean; ReleaseSafe build clean.Not in scope (deferred)
camera_* = 0) + Z-interleaving with sprite layers → T3..tmx+ tileset-image embedding → Phase 4.Do not merge — no engine release/version bump (release choreography is handled post-review).
https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw