Add SVG and PNG rendering capabilities to projects#276
Conversation
The recent commit introduced PNG rendering in libsimlin's C FFI. This change threads that capability through the full stack: - src/engine: adds simlin_project_render_png WASM binding, EngineBackend.projectRenderPng, DirectBackend/WorkerBackend implementations, worker protocol message, and Project.renderPng() - src/server: replaces the resvg-wasm + worker thread pipeline with a single call to the engine's renderPng, removing render-inner.ts, render-worker.ts, and the resvg-wasm dependency - src/pysimlin: adds render_svg/render_png FFI helpers and Project.render_svg/render_svg_string/render_png methods Tests cover PNG signature validation, dimension scaling with aspect ratio preservation, and error handling for nonexistent models. https://claude.ai/code/session_012WrnfoGCS23uqHEkhxyxFX
|
@codex review |
Code ReviewI reviewed the full diff across all 21 changed files covering the Rust FFI, TypeScript WASM bindings, worker protocol, Python bindings, server simplification, and all test files. FindingsNo bugs found. Notes
Overall CorrectnessCorrect. The patch is well-structured, follows established patterns, and includes comprehensive tests across all layers. Existing code and tests should not break. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 009e05985c
ℹ️ 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".
src/server/render.ts
Outdated
| } | ||
| }); | ||
| try { | ||
| return await engineProject.renderPng('main', PREVIEW_WIDTH); |
There was a problem hiding this comment.
Scale previews by max dimension to avoid oversized PNGs
The new preview path always calls renderPng('main', PREVIEW_WIDTH) with width-only scaling, which makes portrait/tall diagrams render at very large heights because the engine preserves aspect ratio from width. The previous implementation scaled by the larger SVG dimension, so outputs stayed bounded near the preview target; with this change, tall models can produce much larger PNG payloads (and potentially fail rendering for extreme aspect ratios), increasing DB/storage and response size for /preview in production.
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #276 +/- ##
=======================================
Coverage 76.52% 76.52%
=======================================
Files 118 118
Lines 27473 27473
=======================================
Hits 21023 21023
Misses 6450 6450 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The previous change always constrained by width, which meant portrait/tall diagrams could produce very large PNGs. This matches the original resvg-wasm behavior: scale by the larger of width/height so neither dimension exceeds the target. Extracts previewDimensions() and parseSvgDimensions() as pure functions with dedicated unit tests covering landscape, portrait, square, edge cases, and extreme aspect ratios. https://claude.ai/code/session_012WrnfoGCS23uqHEkhxyxFX
Code ReviewReviewed all 21 changed files in this PR. The change replaces the multi-step server-side rendering pipeline (engine → JSON → DataModel → SVG via AnalysisEngine TypeScript bindings ( Server simplification ( Python bindings ( Dependency cleanup: Tests: Good coverage across direct-backend, high-level API, Python rendering, preview dimension logic, and SVG parsing. The PNG signature and IHDR dimension verification is a solid approach. FindingsNo bugs found. Overall CorrectnessCorrect. The patch is clean, follows existing patterns consistently, has proper error handling and resource cleanup, and is well-tested. Existing code and tests should not break. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5a79a7b60
ℹ️ 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".
| const svg = await engineProject.renderSvgString('main'); | ||
| const intrinsic = parseSvgDimensions(svg); | ||
| const dims = previewDimensions(intrinsic.width, intrinsic.height, MAX_PREVIEW_SIZE); | ||
| return await engineProject.renderPng('main', dims.width, dims.height); |
There was a problem hiding this comment.
Pass only one constrained dimension to renderPng
renderToPNG computes bounded {width,height} but then passes both into renderPng; the new PNG API gives precedence to non-zero width, so the height bound is ignored. For portrait diagrams where previewDimensions rounds width up with Math.ceil, this can produce previews larger than MAX_PREVIEW_SIZE (e.g., intrinsic 101x2000 gives dims 41x800, but width-precedence rendering yields height 812). That breaks the size cap guarantee and can increase preview storage/transfer costs for narrow/tall models.
Useful? React with 👍 / 👎.
The engine gives width precedence when both width and height are
non-zero, which means the height bound is silently ignored for
portrait diagrams. For example, intrinsic 101x2000 with max 800
previously computed {width: 41, height: 800}, but the engine used
width=41 and derived height=812, exceeding the limit.
previewDimensions now returns only the constraining dimension as
non-zero (the other as 0), letting the engine derive the
unconstrained dimension from the aspect ratio.
https://claude.ai/code/session_012WrnfoGCS23uqHEkhxyxFX
Code ReviewReviewed the full diff adding native SVG/PNG rendering to the engine and simplifying the server rendering pipeline. FindingsNo bugs found. Notes
Overall CorrectnessCorrect. The patch follows existing patterns consistently, has proper resource cleanup, and introduces no bugs. Tests cover the new functionality including edge cases and error paths. |
Summary
This PR adds native SVG and PNG rendering functionality to the Simlin engine, allowing projects to render their stock-and-flow diagrams directly without requiring external diagram processing pipelines.
Key Changes
Core Rendering Implementation
simlin_project_render_png()FFI function to render diagrams as PNG images with optional width/height parametersrender_svg(),render_svg_string(), andrender_png()methods on theProjectclass with proper thread-safety and error handlingServer Simplification
render.ts: Now directly callsengineProject.renderPng()instead of deserializing to JSON, converting to DataModel, rendering SVG, and using worker threadsrender-inner.tsandrender-worker.tsfiles; removed@simlin/diagrampackage dependency from serverTesting
Implementation Details
https://claude.ai/code/session_012WrnfoGCS23uqHEkhxyxFX