Repository navigation
feat: editor_api — wasm editor control surface for studio Play mode (contract v1) - #662
Conversation
…contract v1) Engine side of labelle-studio's Play mode (Phase 3). The studio runs the game compiled to wasm in an iframe and drives it through plain `export fn editor_*` symbols; the assembler-generated main binds the concrete Game/Runner into the module under its preview flag. - src/editor_api.zig: bind/vtable dispatch, pause/step sim gating (shouldTick), post-tick camera override re-assert (frame), scene digest JSON with cap-safe truncation, editor_alloc/free on the C allocator (== emscripten malloc on wasm). Every export is a safe no-op before bind. Non-preview builds emit no editor_* symbols (lazy pub const, verified by nm on paired probe binaries). - Runtime scene-source override map (editor_load_scene): Game-owned copies keyed by scene name, consulted before the embedded source in loadSceneFromSource (via loading_scene_name, set around the three loader_fn call sites) and in loadSceneFile (exact path, then stem). Replaced entries free the previous copy. - Tests: test/editor_api_test.zig + test/scene_source_override_test.zig wired into build.zig test_files. - Version 1.66.0 -> 1.67.0. Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
📝 WalkthroughWalkthroughThis PR adds an editor play-mode API, runtime scene-source override storage and lookup, loader integration for override-aware scene loading, and tests plus build/version updates. ChangesEditor API and scene override flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Host
participant editor_api
participant Game
participant SceneProcess
Host->>editor_api: editor_load_scene(name, src)
editor_api->>Game: setSceneSourceOverride(name, src)
alt current scene reload
editor_api->>Game: setScene(name)
Game->>Game: loading_scene_name = name
Game->>SceneProcess: entry.loader_fn(self)
SceneProcess->>Game: sceneSourceOverride(name)
Game-->>SceneProcess: override source
Game->>Game: loading_scene_name = null
else non-current scene
editor_api-->>Host: override stored
end
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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: 59d7c6be61
ℹ️ 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".
| //! g.render(); | ||
| //! editor_api.frame(&g); // AFTER the tick: re-assert camera |
There was a problem hiding this comment.
If the assembler follows this documented touchpoint sequence, editor_api.frame runs only after g.render(). In unpaused Play mode where a tick script updates the gameplay camera, the render for that frame has already used the script camera, and the editor override written here is overwritten again on the next tick before the next render, so studio camera control only reliably works while paused. The reassertion needs to happen after the tick but before rendering.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7a6ba8d. Valid: with frame() documented after g.render(), an unpaused frame always rendered the script's camera write and the override only held while paused. The documented touchpoint sequence (this module doc is the contract the assembler splices from) is now tick → editor_api.frame → g.render(), and frame's doc spells out why the ordering is load-bearing. Added a loop-order test (camera override: documented loop order…) that simulates 3 unpaused iterations of tick-writes-camera → frame → render and asserts the override is what render observes every frame.
| fn loadSceneImpl(name: []const u8, src: []const u8) i32 { | ||
| game.setSceneSourceOverride(name, src) catch return -1; | ||
| if (game.getCurrentSceneName()) |cur| { | ||
| if (std.mem.eql(u8, cur, name)) return setSceneImpl(name); |
There was a problem hiding this comment.
When the studio sends malformed or partially typed JSONC for the current scene, this path stores the override and immediately calls setSceneImpl, whose setScene unloads the current scene before the loader can fail. The export then returns -1 with current_scene_name cleared and the bad override still installed, so the preview goes blank and a later corrected editor_load_scene no longer reloads automatically because there is no current scene to match.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7a6ba8d. Valid — same teardown-before-validate class as the editor_set_scene issue: a malformed source for the current scene blanked the preview (setScene unloads before the loader parses), left the bad override installed, and cleared current_scene_name so a corrected editor_load_scene no longer auto-reloaded. The current-scene reload is now transactional: snapshot the previous override (exact key), install the new source, and on reload failure roll the map back (new Game.removeSceneSourceOverride) and reload from the last-good source. Returns -1 with the scene intact; a corrected follow-up auto-reloads. Covered by editor_load_scene: malformed source for the current scene rolls back instead of blanking (both the no-previous-override and previous-good-override cases).
| const pos = game.getComponent(ent, core.Position); | ||
| const x: f32 = if (pos) |p| jsonSafeFloat(p.x) else 0; | ||
| const y: f32 = if (pos) |p| jsonSafeFloat(p.y) else 0; | ||
| try appendFmt(buf, cur, ",\"x\":{d},\"y\":{d}}}", .{ x, y }); |
There was a problem hiding this comment.
For parented entities, the Position component is local to the parent, but this digest publishes it directly while editor_set_entity_position explicitly consumes world coordinates. In scenes with nested prefab/child entities, the studio will render and edit those entities at their local offsets instead of their actual world positions, causing selections/drags to appear in the wrong place or jump when written back.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7a6ba8d. Valid: the digest published the raw (parent-relative) Position while editor_set_entity_position consumes world coordinates, so nested prefab children would draw/drag at local offsets. The digest now emits game.getWorldPosition(ent) and the export doc states x/y are world coords. Covered by digest: parented entities report WORLD positions…, including the round-trip assertion that feeding the digested coords back through editor_set_entity_position leaves the local Position unchanged.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/game/loop_mixin.zig (1)
160-164: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMinor style inconsistency: no
defer/block-scope guard here unlike the other two call sites.Currently correct only because
entry.loader_fn(self) catch {}swallows all errors, so the followingself.loading_scene_name = null;always executes. But this differs fromsetScene/setSceneAtomic, which wrap the same pattern in adefer-guarded block. A future edit that changescatch {}totry(or adds an early return) would silently leaveloading_scene_namestuck, corrupting the next override resolution. Matching the other two sites removes that footgun cheaply.♻️ Proposed fix for consistency
- self.loading_scene_name = name; - entry.loader_fn(self) catch {}; - self.loading_scene_name = null; + { + self.loading_scene_name = name; + defer self.loading_scene_name = null; + entry.loader_fn(self) catch {}; + }🤖 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/game/loop_mixin.zig` around lines 160 - 164, The scene-loading override block in `loop_mixin` should match the `setScene` and `setSceneAtomic` pattern by using a `defer`-style guard around `loading_scene_name`. Update the `entry.loader_fn(self)` call site so `self.loading_scene_name` is always cleared through scoped cleanup rather than relying on the current `catch {}` path. This keeps the behavior consistent across all loader entry points and prevents `loading_scene_name` from remaining set if the call flow changes later.
🤖 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.
Nitpick comments:
In `@src/game/loop_mixin.zig`:
- Around line 160-164: The scene-loading override block in `loop_mixin` should
match the `setScene` and `setSceneAtomic` pattern by using a `defer`-style guard
around `loading_scene_name`. Update the `entry.loader_fn(self)` call site so
`self.loading_scene_name` is always cleared through scoped cleanup rather than
relying on the current `catch {}` path. This keeps the behavior consistent
across all loader entry points and prevents `loading_scene_name` from remaining
set if the call flow changes later.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7bab0ca6-7c8a-492b-ad76-f1d991ef57d0
📒 Files selected for processing (12)
build.zigbuild.zig.zonsrc/editor_api.zigsrc/game.zigsrc/game/lifecycle_mixin.zigsrc/game/loop_mixin.zigsrc/game/misc_mixin.zigsrc/game/scene_mixin.zigsrc/jsonc/scene_loader/scene_process.zigsrc/root.zigtest/editor_api_test.zigtest/scene_source_override_test.zig
…al editor_load_scene, world-space digest - P1 (touchpoint ordering): the documented generated-main sequence now places editor_api.frame(&g) AFTER the tick but BEFORE g.render() — with frame after render, every unpaused frame rendered the script's camera and the override only held while paused. Doc-contract fix + a loop-order test asserting the override is visible at render time on every unpaused frame. - P1 (blank preview on bad reload): editor_load_scene for the CURRENT scene is now transactional — snapshot the previous override (exact key), install the new source, and on reload failure roll the map back (new Game.removeSceneSourceOverride) and reload the last-good source. A malformed edit reports -1 with the scene intact and a corrected follow-up still auto-reloads. - P2 (digest coordinate space): entity x/y in editor_scene_digest now come from getWorldPosition, matching the world space editor_set_entity_position consumes; parented prefab children no longer report parent-local offsets. Round-trip test added. Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/editor_api_test.zig (1)
98-106: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing
unbind()isolation calls.Every other test in this file brackets itself with
editor_api.unbind()at start anddefer editor_api.unbind(), per the file's own documented convention (module-scope dispatch state). This test omits both. Low practical risk since alloc/free are likely independent of bind state, but it's inconsistent with the stated isolation pattern and could bite ifeditor_alloc/editor_freestate is ever coupled to bind/unbind internals.♻️ Suggested fix
test "editor_alloc / editor_free round-trip" { + editor_api.unbind(); + defer editor_api.unbind(); + const ptr = editor_api.editor_alloc(128) orelse return error.TestUnexpectedResult;🤖 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/editor_api_test.zig` around lines 98 - 106, The editor_alloc / editor_free round-trip test is missing the standard bind-state isolation used elsewhere in this file. Update the test body to follow the same pattern as the other editor_api tests by calling editor_api.unbind() at the start and deferring editor_api.unbind() at the end. Keep the rest of the assertions unchanged so the test remains focused on allocator behavior while preserving module-scope dispatch state isolation.
🤖 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.
Nitpick comments:
In `@test/editor_api_test.zig`:
- Around line 98-106: The editor_alloc / editor_free round-trip test is missing
the standard bind-state isolation used elsewhere in this file. Update the test
body to follow the same pattern as the other editor_api tests by calling
editor_api.unbind() at the start and deferring editor_api.unbind() at the end.
Keep the rest of the assertions unchanged so the test remains focused on
allocator behavior while preserving module-scope dispatch state isolation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fc9469a0-08c3-4012-84ee-cfb82901eb03
📒 Files selected for processing (5)
src/editor_api.zigsrc/game.zigsrc/game/misc_mixin.zigtest/editor_api_test.zigtest/scene_source_override_test.zig
🚧 Files skipped from review as they are similar to previous changes (3)
- src/game.zig
- test/scene_source_override_test.zig
- src/editor_api.zig
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a6ba8de63
ℹ️ 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".
| // …and reload the scene from the last-good source so the | ||
| // preview doesn't stay blank. If even that fails we're no | ||
| // worse off than before this rollback existed. | ||
| _ = setSceneImpl(name); |
There was a problem hiding this comment.
When editor_load_scene receives malformed JSONC for the current scene and that scene has a non-empty asset manifest, the failed setSceneImpl call has already acquired the target assets and cleared current_scene_name before returning -1. Retrying through setSceneImpl here restores the scene, but because there is no previous scene name anymore the success path never releases the speculative acquire, so each bad editor save pins the scene's asset refcounts one higher. The rollback path should release/clear the pending manifest acquire before reloading the last-good source.
Useful? React with 👍 / 👎.
| // request must not nuke the running scene. | ||
| if (game.scenes.get(name) == null and game.jsonc_scenes.get(name) == null) | ||
| return -1; | ||
| game.setScene(name) catch return -1; |
There was a problem hiding this comment.
When an override has been stored for a non-current scene, switching to that scene through editor_set_scene reaches this raw game.setScene call without the rollback used by editor_load_scene. If the stored source is malformed or partially typed, setScene unloads the currently running scene before the loader fails, so the export returns -1 with the preview blank and the bad override still installed. This is fresh evidence beyond the current-scene reload path: the non-current override path is not covered by loadSceneImpl's transaction.
Useful? React with 👍 / 👎.
| pending_steps -= 1; | ||
| return true; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
When the studio is paused, this false is intended to gate only simulation, but the documented generated main wraps the whole g.tick(dt) in shouldTick(). I checked src/game/loop_mixin.zig: Game.tick pumps assets and processes pending_scene_change before its own pause gate, so a paused editor_set_scene that defers on asset readiness queues a retry but neither the asset pump nor the retrier runs until unpaused or stepped. The editor pause path needs to keep the always-run tick work alive while freezing gameplay simulation.
Useful? React with 👍 / 👎.
Engine side of labelle-studio's Play mode (Phase 3). The studio runs the game compiled to wasm in an iframe and drives it via exported functions (validated by the working spike on FP's bgfx-wasm build: plain
export fn+ emcc-sEXPORTED_FUNCTIONS).The contract (v1 — assembler + studio build against exactly this)
src/editor_api.zig, exported from the root aspub const editor_api. Non-preview builds emit noeditor_*symbols (see verification below).How bind/dispatch works
bindis comptime-generic: it instantiatesHolder(GP, RP)for the concrete*Game/Runner types, stores the pointers in that instantiation's container-levelvars, and publishes a*const VTableof plain function pointers (set_scene/load_scene/set_entity_position/scene_digest/apply_camera). The exports are non-generic and dispatch through the vtable; beforebind, every export is a safe no-op (digest writes{}, scene ops return -1, void setters ignore;editor_pause/editor_stepare pure editor state and work pre-bind).Details worth noting:
editor_set_camerastores the override and applies immediately;frame(g)re-asserts it after every tick (FP'scamera_controlre-asserts the gameplay camera each tick — the editor wins by writing last). Comptime no-op for camera-less games (Game.CameraType == void).editor_set_scenevalidates the name against the scene registries before callingsetScene— a rawsetScenetears the current scene down before discoveringSceneNotFound, so a typo'd editor request must not nuke the running scene. Asset-gate deferrals queue a retry viaqueueSceneChange(same commit detection as fix: queued scene change dropped when its assets aren't ready yet #635).setWorldPosition(world coords → local for parented entities), which routes throughsetPosition→markPositionDirtyWithChildren.Scene-source override map
New Game-owned
scene_source_overrides(keys and values are allocator-owned copies; replacing an entry frees the previous source, freed on deinit). Consulted before the embedded source at both loader seams:loadSceneFromSource(the assembler-generated per-scene loader) resolves by the name of the scene being loaded — a newloading_scene_nameis set around the threeloader_fncall sites (setScene,setSceneAtomic, hot-reload).loadSceneFile(include fragments) resolves by exact path, then by the path's stem (an override stored under"frag"also replaces"scenes/frag.jsonc").How "no editor symbols in non-preview builds" was verified
root.zigexposes the module only as a lazypub const editor_api = @import("editor_api.zig")and nothing else in the engine references it. Built two otherwise-identical probe executables against this branch's engine module:noeditor(usesGameinit/tick/render, never referenceseditor_api):nmshows 0editor_*symbols.witheditor(callsbind/shouldTick/frame):nmshows all 10 exports as globalT _editor_*symbols.Both binaries also run correctly.
Tests
zig build+zig build testexit 0 (all 63 test binaries green).test/editor_api_test.zig— pause/step tick gating, pre-bind no-op safety, alloc/free round-trip, digest JSON shape (parsed withstd.json), truncation validity (including a cap sweep 2..400, every output parses),editor_set_entity_position(incl. bad ids),editor_set_scene, camera override state machine (engage → re-assert-after-tick → release; comptime no-op on StubRender).test/scene_source_override_test.zig— set → load consults override (both loader seams), replace frees old + takes effect (leak-checked), exact-key-vs-stem lookup rules,editor_load_sceneend-to-end (stores + reloads current scene now).Deviations from the task spec
std.json.Stringify.valueAllocconvention — the export is allocation-free with cap-bounded truncation semantics; escaping is implemented explicitly and validated by parsing every emitted digest in tests.editor_load_scene/editor_set_scenereturn-1pre-bind (the contract's "setters return/ignore" left the i32 value open).https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw