feat(svg-builder): build SVGs after Suzuki-Abe topology extraction - #238
Conversation
- simplify useWasmWorker convenience functions - add type checks for arguments and return types - add better typing and guarding system against bad values - no TypeScript as its benefit here would not be felt
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughReplaces the ImageTracer-based SVG path with an integrated C++ labels→SVG pipeline ( Changes
Sequence Diagram(s)sequenceDiagram
participant React as React (WasmImageProcessor)
participant Hook as useWasmWorker Hook
participant Worker as wasmWorker
participant WASM as Image WASM Module
participant Mem as WASM Memory
React->>Hook: call({ funcName: "labels_to_svg", args, bufferKeys, returnType: "string" })
Hook->>Worker: postMessage({ funcName, args, bufferKeys, returnType })
Worker->>Mem: WASM_TYPES.alloc / write buffers
Worker->>WASM: _labels_to_svg(ptr_data, ptr_labels, width, height, ...)
WASM->>WASM: region_labeling → Graph construction → compute_contours() → contoursResultToSVG()
WASM-->>Worker: returns char* (SVG pointer)
Worker->>Mem: UTF8ToString(svg_ptr)
Worker->>Mem: free allocated pointers
Worker-->>Hook: postMessage({ output, returnValue: svgString })
Hook-->>React: { svg: svgString, visualization }
React->>React: render SVG
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Ryan-Millard
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/WasmImageProcessor.jsx (1)
3-3: Remove unused import to fix pipeline failure.The
uint8ClampedArrayToSVGimport is no longer used since the processing pipeline was replaced withtestSvg().🔧 Proposed fix
-import { loadImageToUint8Array, uint8ClampedArrayToSVG } from '@utils/image-utils'; +import { loadImageToUint8Array } from '@utils/image-utils';
🤖 Fix all issues with AI agents
In `@src/components/WasmImageProcessor.jsx`:
- Line 131: The useCallback for processImage in the WasmImageProcessor component
lists unused dependencies causing unnecessary re-renders; remove
bilateralFilter, blackThreshold, kmeans, and findContours from the dependency
array of the useCallback (the array currently passed to useCallback that
includes fileData, bilateralFilter, blackThreshold, kmeans, findContours,
testSvg, navigate, step) so it only contains the actually used variables (e.g.,
fileData, testSvg, navigate, step) and re-run tests to ensure no missing
dependencies remain.
In `@src/wasm/modules/image/include/test_svg.h`:
- Line 6: Change the exported function declaration/definition for test_svg:
remove the constexpr specifier and change the return type to const char* so it
returns a string literal correctly for WASM/C linkage; update the declaration
EXPORTED constexpr char* test_svg() to EXPORTED const char* test_svg() and make
the corresponding function definition/signature for test_svg match this return
type (and keep the EXPORTED/C linkage as-is).
In `@src/wasm/modules/image/src/test_svg.cpp`:
- Around line 3-5: The function test_svg is declared incorrectly: remove the
constexpr specifier (exported C-linkage functions cannot be constexpr) and
change its return type to const char* so it returns a string literal safely;
update the definition of test_svg() to be a non-constexpr function with
signature returning const char* and return the SVG literal as a const char*.
🧹 Nitpick comments (3)
src/components/WasmImageProcessor.jsx (1)
16-16: Consider removing unused destructured functions.If the commented-out code won't be restored soon, consider also removing the unused destructured functions from the hook call:
- const { bilateralFilter, blackThreshold, kmeans, findContours, testSvg } = useWasmWorker(); + const { testSvg } = useWasmWorker();This keeps the code clean and matches the actual usage.
src/workers/wasmWorker.js (2)
89-94: Consider safeguarding against prototype pollution.CodeQL flags potential property injection via user-controlled
keyvalues. While this worker only receives messages from the same-origin main thread, adding a simple guard prevents prototype pollution attacks if the code is ever repurposed.🔒 Proposed safeguard
bufferKeys?.forEach(({ key, type }) => { if (!(type in WASM_TYPES)) throw new Error(`Unsupported type (${type}) in wasmWorker.js\nSee WASM_TYPES for the supported types`); + if (key === '__proto__' || key === 'constructor' || key === 'prototype') { + throw new Error(`Invalid buffer key: ${key}`); + } const ptr = WASM_TYPES[type].alloc(args[key]); pointers[key] = { ptr, type, length: args[key].length || undefined }; args[key] = ptr; });
101-105: Apply same property validation for output keys.For consistency with the input safeguard, validate
keybefore writing tooutput:🔒 Proposed safeguard
const output = {}; bufferKeys?.forEach(({ key, type }) => { + if (key === '__proto__' || key === 'constructor' || key === 'prototype') { + throw new Error(`Invalid buffer key: ${key}`); + } output[key] = WASM_TYPES[type].read(pointers[key].ptr, pointers[key].length); });
8b902c1 to
348c581
Compare
|
@Krasner, it now maps to SVGs fairly nicely. I need to add the colors and fix a few things about it, but it is mostly there. I'll make a new function, export the ContoursResult over the C ABI and also get rid of We're almost there.😁 |
|
It now works quite nicely.🥳🥳 
One minor issue - we have doubled-up borders: It isn't a big deal because we've come this far and can probably fix it quite easily. It's probably a tiny bug. |
|
it's not a bug. neighboring regions will have contours that follow each other because their pixels can't overlap. we need a way to merge the shared border: (hope the diagrams make sense) This can be handled by the Graph class but need to think about it a bit deeper... |
So the pixels that are actually part of the border aren't counted as part of the contour? |
yeah at least for the purposes of generating polygons for SVG... i need to think about this some more but that's my current understanding. |
|
i'm able to get this branch running post merge. and bilateralFilter: async ({ pixels, width, height, sigma_spatial = 3, sigma_range = 50, color_space = 0, n_threads = 8 }) => {
const result = await call({
funcName: 'bilateral_filter',
args: { pixels, width, height, sigma_spatial, sigma_range, color_space, n_threads },
bufferKeys: [{ key: 'pixels', type: 'Uint8ClampedArray' }]
});
return result.output.pixels;
},and kmeans: async ({
pixels,
out_pixels = new Uint8ClampedArray(pixels.length),
out_labels = new Int32Array(pixels.length / 4),
width,
height,
num_colors,
max_iter = 100,
color_space = 0,
n_threads = 8
}) => {
const result = await call({
funcName: 'kmeans',
args: { pixels, out_pixels, out_labels, width, height, num_colors, max_iter, color_space, n_threads },
bufferKeys: [
{ key: 'pixels', type: 'Uint8ClampedArray' },
{ key: 'out_pixels', type: 'Uint8ClampedArray' },
{ key: 'out_labels', type: 'Int32Array' }
]
});
return { pixels: result.output.out_pixels, labels: result.output.out_labels };
}, |
|
@Ryan-Millard yes the problem is verifiably the tangent contours. Since the contour points are currently int type and non-overlapping you will currently always see double lines. They need to be stitched to subpixel precision prior to handing over the SVG. A few changes i made on my fork:
https://github.com/Krasner/Img2Num/tree/feat/svg-builder https://github.com/Krasner/Img2Num/blob/26ce3700862770bd3d4ad3c3c1c847cd519585b9/src/wasm/modules/image/src/node.cpp#L99 Still a few stitching errors but much better |
|
some kind of bezier smoothing would be nice, but must be careful not to introduce holes between contour edges |
|
yeah it's worth understand what imagetracer.js is doing under the hood. there is quadratic spline smoothing and interpolation happening to get smoother contours |
|
Hi @Krasner! I'm sorry for not replying sooner. I took a break over the weekend from this to avoid burning out - because that's my biggest fear when it comes to this project - and forgot to reply to this thread. What you showed looks promising and I'm pulling your changes now. Thank you! |
That's the main reason why it needs to be addressed at a later stage. We need to fix this problem, then move onto things like that. |
|
this is actually a much harder problem that I anticipated... I think i'm getting close to a solution though. Will update you when i've got it working... hopefully soon |
This looks really good! We definitely lost some of the data about the paths here, but there are fewer gaps in this version.
What I noticed is that some borders seem to be thicker than others when the contours are visualized. From my understanding of how Suzuki-Abe traces contours, they should all end up being 1px wide borders. The below shows a strange bunching of a pink border. This could be the cause of our pain: |
I truly thought it would be simpler, too. A simple quick patch could be to use the stroke attribute on each path, but that would definitely be a temporary hack that won't work everywhere. |
- vectors -> vector
|
@CodeRabbit please generate the most important doc strings and comment them as proposed changes on this PR so I can easily add them. |
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@docs/docs/introduction/contributing.md`:
- Line 17: Replace the raw URL with a Markdown link for readability and
consistency: change the line containing
"https://github.com/Ryan-Millard/Img2Num/issues" to use the Markdown link
format, e.g. "[open an issue](https://github.com/Ryan-Millard/Img2Num/issues)"
so the sentence reads "If you encounter a bug or have a feature request, please
open an issue at: [open an
issue](https://github.com/Ryan-Millard/Img2Num/issues)" or similar phrasing
using the Markdown link.
In `@docs/docs/reference/wasm/modules/image/graph/api.md`:
- Line 48: Fix the grammar in the sentence describing region_labeling: change "a
collections of pixels" to "a collection of pixels" so the sentence reads "In
`region_labeling` each Node is assigned an id and a collection of pixels";
update the doc entry that references `region_labeling` and `Node` accordingly.
- Line 13: Add a missing period to the sentence describing Node neighbor
references: update the sentence "`Node`s reference neighbors through node
pointers (`shared_ptr`)" to end with a period so it reads "`Node`s reference
neighbors through node pointers (`shared_ptr`)." — edit the markdown content
where the Node/shared_ptr sentence is defined (look for the line mentioning Node
and shared_ptr).
In `@src/hooks/useWasmWorker.js`:
- Around line 42-47: The call function (useCallback named call) can run before
workerRef.current is initialized or after cleanup, which would throw; update
call to check workerRef.current before posting: when invoked, allocate id and
store callbacks.current.set(id, {resolve,reject}) as now, but if
workerRef.current is falsy immediately reject the promise (and remove the stored
callback) with a clear Error like "Worker not initialized" instead of calling
postMessage; this keeps promise semantics consistent and avoids exceptions from
workerRef.current.postMessage.
In `@src/wasm/modules/image/include/SavitskyGolay.h`:
- Around line 4-8: Remove the unused `#include` <iomanip> from the header; edit
SavitskyGolay.h to delete the <iomanip> include line (keeping Point.h,
<numeric>, <stdexcept>, and <vector>), and verify that no functions or the
SavitskyGolay class use iomanip features so the header still compiles without
that include.
In `@src/wasm/modules/image/src/SavitskyGolay.cpp`:
- Around line 1-4: The file uses std::copy but does not include the required
header; add `#include` <algorithm> to the top of
src/wasm/modules/image/src/SavitskyGolay.cpp alongside the existing includes so
std::copy is declared (this will fix the build error where std::copy is used in
the SavitskyGolay implementation).
- Around line 5-12: The constructor SavitzkyGolay currently uses assert for
input validation which is disabled in release; replace those asserts with
runtime checks that throw std::invalid_argument when inputs are invalid (e.g.,
if radius < 0, or if window_size_ <= poly_order_, and also validate poly_order_
>= 0), so compute_coefficients(), coeffs_.resize(), and filter() never run with
bad sizes; update the SavitzkyGolay::SavitzkyGolay(int radius, int poly_order)
to perform these checks and throw descriptive std::invalid_argument messages
before initializing members or calling compute_coefficients().
- Around line 40-64: In SavitzkyGolay::filter_wrap the index arithmetic mixes
size_t and int (i + j), causing unsigned underflow; change the index math to use
a signed type (e.g. std::ptrdiff_t) for k and for comparisons: compute
std::ptrdiff_t k = static_cast<std::ptrdiff_t>(i) + j, do wrapping with k += or
-= static_cast<std::ptrdiff_t>(data.size()) as needed, and finally access data
with data[static_cast<size_t>(k)]; update any related comparisons to cast
data.size() to std::ptrdiff_t and keep coeffs_[j + m_] usage and result
assignment unchanged.
- Around line 14-38: The loop indexes in SavitzkyGolay::filter mix signed and
unsigned types causing implicit conversions; change the outer loop and any index
arithmetic to use a signed type (e.g., std::ptrdiff_t) instead of size_t so
expressions like i + j (with j signed) remain signed and clear—update the
convolution loop (currently using size_t i in for (size_t i = m_; i <
data.size() - m_; ++i)), the edge-copy loops, and any comparisons against
window_size_/m_ to use std::ptrdiff_t (or cast data.size() to std::ptrdiff_t
when comparing) so all index math with coeffs_, data, result, m_, and coeffs_[j
+ m_] is consistently signed and safe.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 6728
Plan: Add Docstrings to Key FunctionsI'll add comprehensive documentation to the most critical functions and classes introduced in this PR:
These docstrings provide comprehensive documentation covering:
The documentation emphasizes:
🧠 Learnings used💡 Use |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/wasm/modules/image/include/labels_to_svg.h`:
- Around line 1-2: The header guard macro is mismatched: replace the existing
KMEANS_GRAPH_H guard in src/wasm/modules/image/include/labels_to_svg.h with
LABELS_TO_SVG_H (update both `#ifndef` and `#define`) and ensure the file ends with
a closing `#endif` /* LABELS_TO_SVG_H */ comment so the guard name matches the
filename and is clear.
In `@src/wasm/modules/image/src/labels_to_svg.cpp`:
- Around line 168-187: The code emits one <path> per contour and ignores
holes/hierarchy, causing donut shapes to render filled; update
contoursResultToSVG to group contours into regions using result.is_hole or
result.hierarchy and emit outer contour plus any hole contours as subpaths
inside the same SVG path "d" attribute (concatenate the contour pathData strings
so holes are sibling subpaths), and add fill-rule="evenodd" on that <path> (or
on the root) so holes are subtracted; specifically change the loop over
result.curves in contoursResultToSVG to build per-region combined path data
(using result.is_hole or hierarchy to decide which curves are holes) and output
one <path d="...outer+holeSubpaths..." fill="... " fill-rule="evenodd" />
instead of a path-per-contour.
🧹 Nitpick comments (2)
src/wasm/modules/image/include/labels_to_svg.h (1)
8-10: Document ownership of the returned SVG string.The function returns a heap-allocated C string; the header should state that the caller must free it (e.g.,
free/_free) and thatnullptrcan be returned.📝 Suggested header doc
-EXPORTED char *labels_to_svg(uint8_t *data, int32_t *labels, const int width, - const int height, const int min_area, - const bool draw_contour_borders); +/** + * `@return` Dynamically allocated SVG string. Caller must free with free/_free. + * Returns nullptr when draw_contour_borders is true or on allocation failure. + */ +EXPORTED char *labels_to_svg(uint8_t *data, int32_t *labels, const int width, + const int height, const int min_area, + const bool draw_contour_borders);src/wasm/modules/image/src/labels_to_svg.cpp (1)
238-248: Hierarchy indices become invalid when concatenating per-node contours.
node_contours.hierarchyindices are local to each node. Appending them directly toall_contours.hierarchy(Line 246–248) leaves parent/child indices pointing at the wrong contours once multiple nodes are merged.🔧 Suggested offset when appending
ColoredContours all_contours; for (auto &n : G.get_nodes()) { if (n->area() == 0) continue; ColoredContours node_contours = n->get_contours(); + const int base = static_cast<int>(all_contours.contours.size()); for (auto &c : node_contours.contours) { all_contours.contours.push_back(c); } - for (auto &c : node_contours.hierarchy) { - all_contours.hierarchy.push_back(c); + for (auto &h : node_contours.hierarchy) { + auto h2 = h; + for (int &idx : h2) { + if (idx != -1) idx += base; + } + all_contours.hierarchy.push_back(h2); }
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@coderabbitai, please create the docstrings for this. |
|
Tip For best results, initiate chat on the files or code changes.
Task: Add Comprehensive Docstrings to Key FunctionsI'll add detailed documentation to the 6 most critical components introduced in this PR:
🧠 Learnings used💡 Use |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
3 participants
What was changed & why
See #245 first.
After gathering the topology of the image from
findContours, the topology is now mapped into an SVG string.This completes the library.
Fixes: #190
Changes
There are many changes:
Testing & Verification
Images produce proper SVGs.
Additional Resources
Summary by CodeRabbit
Refactor
New Features
Documentation
Chores