Feat/svg builder3 - #254
Conversation
… labels (Ryan-Millard#248) * ci(stale.yml): Configure stale workflow with new settings Updated stale issue and PR messages, added days before stale and close settings, and exempt labels. * style(stale-workflow): format properly * deps(stale-workflow): bump to newer v10 * ci(stale.yml): fix label inputs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * docs(PR-template): update PR template * ci: add PR auto-label workflow and labeler config * ci: PR heading and body checker * fix: workflow does not contain permissions Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> * style(workflows): format new workflow files * ci(deps): bump labeler to v6 * ci(pr-check): fix permissions --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
- 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
📝 WalkthroughWalkthroughAdds cubic Bezier support and constrained smoothing to the image pipeline: new data structures and fitting for CubicBezier, constrained Savitzky–Golay filtering with corner/locked-point handling, updated contour processing and SVG generation, plus minor JS formatting and docs additions. Changes
Sequence DiagramsequenceDiagram
participant Client as Graph::compute_contours
participant Contours as Contour Pipeline
participant SG as SavitskyGolay
participant Bezier as Bezier Fitting
participant SVG as labels_to_svg
Client->>Contours: provide raw contours + locked masks
Contours->>Contours: detectCorners (2-step neighbors)
Contours->>SG: filter_wrap_with_constraints(contours, locked, cornerMasks)
SG->>SG: solveQuadraticAtZero per-sample constrained solve
SG->>Contours: return smoothed contours
Contours->>Contours: coupledSmooth / selectiveSmooth (radius, pairRadiusSq)
Contours->>Bezier: fit_curve_reduction (quad & cubic overloads)
Bezier->>Bezier: generateCubicBezier / fitRecursive
Bezier->>Client: return QuadBezier & CubicBezier sets
Client->>SVG: contourToSVGCurve(all_curves, all_ccurves)
SVG->>SVG: emit path data (Q and C commands) and finalize SVG
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/wasm/modules/image/src/labels_to_svg.cpp (1)
169-189:⚠️ Potential issue | 🟠 MajorThe
contourToSVGCurve(CubicBezier)overload is dead code andccurvesare never rendered.
contoursResultToSVG(line 198) loops overresult.curves(QuadBezier) only. Meanwhile,result.ccurves(CubicBezier) are accumulated at lines 283–284 but never passed to any rendering function. The cubic Bézier overload at line 169 is never called.Either update
contoursResultToSVGto useccurves(replacing or preferring them overcurves), or remove the unused accumulation and overload.
🤖 Fix all issues with AI agents
In `@docs/docs/reference/wasm/modules/image/graph/explained.md`:
- Around line 39-67: Delete the duplicated block that repeats the "Diagram",
graph visualization, "Small Area Node Merging (Small Region Pruning)" and
"Example" sections (the repeated Slide1/Slide2/Slide3 discussion and images),
remove the consecutive duplicate image reference (![slide1] to Slide1.SVG),
normalize image filenames to lowercase (.svg) so they match the original
references, and fix the typo "Similarily" → "Similarly" in the Small Area Node
Merging paragraph so only the first occurrence of these sections and images
remain (look for headings "Diagram" and "Small Area Node Merging" and image
names Slide1, Slide2, Slide3).
- Around line 135-146: Remove the duplicated <details> block that contains the
"Pull the code locally from PR `#238`" summary and the bash snippet; keep only one
instance of that block (the original on lines earlier in the file) so the
instructions aren’t repeated, and ensure the surrounding Markdown remains valid
(matching opening and closing <details> and ``` fences).
In `@docs/docs/reference/wasm/modules/image/graph/explained2.md`:
- Around line 105-130: Add documentation for the newly introduced CubicBezier
struct and the ContoursResult::ccurves member: insert a "### `struct
CubicBezier`" section after the QuadBezier section that documents members `p0`,
`p1`, `p2`, `p3` (each as Point with start/control/control/end descriptions),
and add a row to the ContoursResult member table describing `ccurves` with type
`vector<vector<CubicBezier>>` and a short description like "Vectorized cubic
Bezier representation of contours; ccurves[k] contains the cubic segments for
the k-th contour."
In `@src/components/WasmImageProcessor.jsx`:
- Around line 96-99: The code contains a duplicated progress update call
step(95) in the WasmImageProcessor component — remove the redundant second
invocation so progress is only set once; locate the duplicate step(95) calls in
the function where step is used (e.g., the progress update block inside
WasmImageProcessor.jsx) and delete the repeated call, leaving a single step(95)
to avoid the no-op and clarify intent.
In `@src/hooks/useWasmWorker.js`:
- Around line 61-69: Prettier CI is failing for src/hooks/useWasmWorker.js due
to formatting in the bilateralFilter parameter destructuring; run the formatter
(e.g., prettier --write src/hooks/useWasmWorker.js) or reformat the
bilateralFilter async ({ ... }) block to match the project's Prettier style used
elsewhere (see kmeans function parameter formatting) so the file passes CI.
In `@src/wasm/modules/image/src/bezier.cpp`:
- Around line 289-306: fit_curve_reduction currently calls chains[i].front()
unconditionally which is UB for empty chains; modify fit_curve_reduction to
check if chains[i].empty() before accessing front(): if empty, either push an
empty vector<CubicBezier> into results (to preserve indexing) or continue to the
next chain, otherwise build c by copying points and appending chains[i].front()
and then call fitRecursive(c, tolerance, result); ensure you refer to
fit_curve_reduction, chains, results, fitRecursive, Point and CubicBezier when
making the change.
In `@src/wasm/modules/image/src/contours.cpp`:
- Around line 742-744: Remove the debug stdout prints inside selectiveSmooth
that print kernel coefficients; locate the loop that outputs coeff[k] and the
trailing std::endl (the std::cout << coeff[k] << " "; and std::cout <<
std::endl; statements) and delete them so no kernel coefficients are printed to
stdout on each call.
- Around line 700-710: The loop uses size_t i causing unsigned underflow when
computing previ = i - 2; change the loop to use a signed index (e.g., for (int i
= 0; i < (int)pts.size(); ++i)) or cast i to int before arithmetic (int ii =
(int)i; int previ = ii - 2; int nexti = ii + 2;) and keep the existing wrap
logic for previ/nexti so negative values are handled correctly; adjust all uses
of i in this loop to the signed index (ii) or the new int i.
- Around line 806-810: The next-index conditional is inverted and the prev/next
Point variables are dead: change the nextp computation to use "p <
contours[c].size() - 1 ? p + 1 : 0" instead of the current comparison, and then
either remove the unused Point prev and Point next declarations (prevp/nextp may
remain if indexes are used) or keep them if you will reference prev/next later;
specifically update the variables shown (Point myPos, int prevp, int nextp,
Point prev, Point next) so there are no out-of-bounds index calculations and no
unused variables left.
In `@src/wasm/modules/image/src/labels_to_svg.cpp`:
- Around line 283-285: The loop that pushes node_contours.ccurves into
all_contours.ccurves is doing unnecessary work because contoursResultToSVG does
not use all_contours.ccurves; remove the accumulation to avoid wasted work
(delete the for-loop that copies node_contours.ccurves into
all_contours.ccurves) or, if you prefer to keep the intent, wrap it behind a
clear TODO/feature flag and comment referencing future cubic curve rendering;
update or remove any dependent comments and ensure contoursResultToSVG continues
to operate on the used contours fields only.
In `@src/wasm/modules/image/src/SavitskyGolay.cpp`:
- Around line 82-84: The comment above the weight declarations in
SavitskyGolay.cpp is inconsistent: it says "Fixed = 10000.0 (Hard constraint)"
while the code defines float wFixed = 100.0; update either the comment or the
value so they match—preferably change the comment to "Fixed = 100.0 (Hard
constraint)" or, if the intention was 10000.0, change the initializer for wFixed
to 10000.0; ensure you update the adjacent comment referencing wNormal and
wFixed so the message and the actual values remain consistent.
- Around line 86-98: The loop uses size_t i and int j then computes int k = i +
j, causing signed/unsigned mismatch and UB; fix by converting i to a signed type
before arithmetic (e.g., int idx = static_cast<int>(i); then use int k = idx +
j) and apply the same change in the other occurrence in filter_wrap; update uses
of i in this loop (and any wrap-around adjustments) to use idx to avoid implicit
conversions.
- Around line 214-236: The function solveQuadraticAtZero currently returns 0.0
on near-singular matrices which maps the point to the origin; change its
signature to accept a fallback value (e.g., float fallback) and when
std::abs(det) < 1e-9 return that fallback instead of 0.0, and update all call
sites that invoke solveQuadraticAtZero to pass the center sample (original data
value) as the fallback so ill-conditioned solves preserve the original point
rather than teleporting it to (0,0).
🧹 Nitpick comments (10)
src/components/WasmImageProcessor.jsx (1)
107-107: Commented-outstep(100)— clean up or restore.Leaving commented-out code in the pipeline is a maintenance smell. If the intent is to skip the 100% step because
navigatefires immediately afterfindContours, remove the line entirely and add a brief comment explaining why. If it was disabled temporarily for debugging, restore it.Proposed fix (if intentionally skipped)
- //step(100); + // Progress intentionally skipped — navigate fires immediately after findContourssrc/wasm/modules/image/include/SavitskyGolay.h (1)
25-25:cornerparameter is accepted but unused in the implementation.Looking at the implementation in
SavitskyGolay.cpp(lines 68–137 in the relevant snippets), thecornervector is never read — both usages are commented out (// if (locked[i] || corner[i]) { continue; }and// || corner[k]). If corner detection is planned for later, consider documenting that intent; otherwise this is dead surface area in the public API.Also, the implementation comment says
"Fixed = 10000.0 (Hard constraint)"but the code setswFixed = 100.0.src/wasm/modules/image/include/contours.h (1)
14-14: Move#include <numeric>from the header tocontours.cpp.The header does not use any
<numeric>symbols, butcontours.cppusesstd::accumulateat line 739 and currently relies on the transitive include. Moving the include directly to the translation unit that needs it reduces unnecessary header dependencies.src/wasm/modules/image/src/labels_to_svg.cpp (1)
200-201: Remove commented-out code.These lines are debug leftovers. If the QuadBezier path is the intended code path, remove the commented lines to reduce noise.
🧹 Proposed cleanup
for (size_t i = 0; i < result.curves.size(); ++i) { std::string pathData = contourToSVGCurve(result.curves[i]); - //for (size_t i = 0; i < result.contours.size(); ++i) { - //std::string pathData = contourToSVGPath(result.contours[i]);src/wasm/modules/image/src/graph.cpp (1)
295-307: Redundant.resize()before direct assignment; clean up commented-out code.Lines 299 and 304 call
.resize()onc0->curves[i]andc0->ccurves[i]respectively, but the immediately following=assignment (lines 302, 307) replaces the entire vector, making the resize unnecessary.Also, the commented-out
std::copyblocks (lines 295–296, 300–301, 305–306) should be removed.🧹 Proposed cleanup
for (size_t i = 0; i < c0->contours.size(); ++i) { - //std::copy(all_contours[j].begin(), all_contours[j].end(), - // c0->contours[i].begin()); c0->contours[i] = all_contours[j]; - c0->curves[i].resize(all_curves[j].size()); - //std::copy(all_curves[j].begin(), all_curves[j].end(), - // c0->curves[i].begin()); c0->curves[i] = all_curves[j]; - c0->ccurves[i].resize(all_ccurves[j].size()); - //std::copy(all_ccurves[j].begin(), all_ccurves[j].end(), - // c0->ccurves[i].begin()); c0->ccurves[i] = all_ccurves[j]; j++; }src/wasm/modules/image/src/SavitskyGolay.cpp (1)
69-73:cornerparameter is unused.The
cornervector is accepted but never referenced in the function body (the check at line 91 is commented out). Either use it or remove it from the signature to avoid confusion.src/wasm/modules/image/src/contours.cpp (4)
725-738: Recursivefactorialviastd::functionis fragile and slow.
std::functionhas overhead from type-erasure. More importantly, the integerfactorialwill overflowintfor inputs ≥ 13 (andintis typically 32-bit). Currently safe withradius=2(window=5, max factorial(4)=24), but any future caller passing a larger radius will silently overflow.Consider a simple iterative approach or just precompute binomial coefficients directly (e.g., Pascal's triangle row), which avoids factorial entirely.
♻️ Alternative: build binomial row iteratively
- std::function<int(int)> factorial = [&factorial](int x) -> int { - if (x <= 1) return 1; - return x * factorial(x-1); - }; - std::vector<Point> original = pts; int window = 2 * radius + 1; std::vector<float> coeff(window); - // binomial coefficients - for (int k = 0; k < window; ++k){ - //n!/((n-k)! * k!) - coeff[k] = static_cast<float>(factorial(window-1)/ (factorial(k) * factorial(window-1-k))); - } + // Binomial coefficients via iterative construction (no overflow risk for moderate window) + coeff[0] = 1.0f; + for (int k = 1; k < window; ++k) { + coeff[k] = coeff[k - 1] * static_cast<float>(window - k) / static_cast<float>(k); + }
763-767: Remove commented-out code.The old Laplacian smoothing block is commented out. Clean it up to reduce noise.
776-783:SavitzkyGolay(5, 2)constructor computes coefficients thatfilter_wrap_with_constraintsignores.The constructor at line 776 precomputes smoothing coefficients (
coeffs_) for radius=5, poly_order=2. However,filter_wrap_with_constraintsbuilds its own per-sample normal equations using onlywindow_radius_for the loop range and hardcodes a 3×3 (quadratic) system. The precomputedcoeffs_are never used by this code path.This is not a bug, but the wasted computation and the coupling between the constructor's polynomial order and the hardcoded 3×3 matrix in
filter_wrap_with_constraintsis a maintainability concern. If someone changespoly_order_expecting it to affect the constrained filter, it won't.
846-847: Commented-out code in partner target calculation.Lines 846–847 are remnants of the old direct-neighbor approach. Remove them.
| ## Diagram | ||
| Assume a region partition image as shown. Each partition (and the pixels contained within) are represented as a Node. Nodes exist in the heap. A Graph is a collection of shared_ptr(s) to these Nodes. A Graph has unique ownership over these Nodes pointers. | ||
|  | ||
|
|
||
|  | ||
| _Figure 1: Partitioned Image_ | ||
|
|
||
| Below is a visualization of the Graph with its connections forming a undirected graph (a). In reality each `Node` contains a list of pointers to their neighbors, represented as edges (b). Note that adjacent nodes point to each other. Edge management is handled by the Graph. | ||
|  | ||
| _Figure 2: Visualizing a Graph. Note in (b) only 3 nodes are displayed 0,1,3. The remaining nodes have a similar structure_ | ||
|
|
||
| ## Small Area Node Merging (Small Region Pruning) | ||
|
|
||
| One of the Graph's main operations is Node merging. Suppose we want to absorb/merge region 0 (Node 0) into region 2 (Node 2). Node 2 will assume ownership of Nodes 0's pixels and neighbors. The following figure shows the step-by-step process that happens in Graph. | ||
|
|
||
|  | ||
| _Figure 3: Step-by-step visualization of node merging_ | ||
|
|
||
| **a**. Consider merging Node 0 into Node 2 | ||
|
|
||
| **b**. Disconnect edges to Node 0 neighbors. This requires iterating over Nodes 1, 2, and 3, and removing Node 0 from their edge set. | ||
|
|
||
| **c**. Transferring edges to absorbing node (Node 2). Again, iterate over Nodes 1 and 3 to assign Node 2 as an edge. Similarily Node 2 adds Node 1 and 3 as edges. In this case Node 2 and Node 3 already share an edge, but Node 1 gets a new edge. | ||
|
|
||
| **d**. Pixels owned by Node 0 are passed to Node 2. Node 0 is removed from the graph. | ||
|
|
||
| **e**. In image space, Node 2 (region 2) now contains the area that used to be Node 0's. | ||
|
|
||
| ## Example |
There was a problem hiding this comment.
Remove duplicated content sections.
Lines 39–67 duplicate the existing content from lines 9–36 (Diagram, Graph visualization, Small Area Node Merging, and Example headings). This looks like a rebase artifact. Additionally:
- Line 41/43:
.SVG(uppercase) vs.svg(lowercase, line 13) — will break on case-sensitive filesystems. - Line 43:
![slide1]image is rendered twice consecutively. - Line 61: Typo — "Similarily" → "Similarly".
- Lines 50 and 67: Duplicate headings trigger markdownlint MD024.
Remove the entire block (lines 39–67) to eliminate the duplication.
🧰 Tools
🪛 LanguageTool
[grammar] ~61-~61: Ensure spelling is correct
Context: ...es 1 and 3 to assign Node 2 as an edge. Similarily Node 2 adds Node 1 and 3 as edges. In t...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.20.0)
[warning] 50-50: Multiple headings with the same content
(MD024, no-duplicate-heading)
[warning] 67-67: Multiple headings with the same content
(MD024, no-duplicate-heading)
🤖 Prompt for AI Agents
In `@docs/docs/reference/wasm/modules/image/graph/explained.md` around lines 39 -
67, Delete the duplicated block that repeats the "Diagram", graph visualization,
"Small Area Node Merging (Small Region Pruning)" and "Example" sections (the
repeated Slide1/Slide2/Slide3 discussion and images), remove the consecutive
duplicate image reference (![slide1] to Slide1.SVG), normalize image filenames
to lowercase (.svg) so they match the original references, and fix the typo
"Similarily" → "Similarly" in the Small Area Node Merging paragraph so only the
first occurrence of these sections and images remain (look for headings
"Diagram" and "Small Area Node Merging" and image names Slide1, Slide2, Slide3).
| <details> | ||
| <summary>Pull the code locally from PR #238</summary> | ||
| ```bash | ||
| # 1. Clone the repo (if you haven't already) | ||
| git clone https://github.com/Ryan-Millard/Img2Num.git | ||
| cd Img2Num | ||
| # 2. Fetch the specific commit from the PR | ||
| git fetch origin 9eb23f9a56edaeec95e2dfcfc8389b11bfd777b6 | ||
| # 3. Create a local branch pointing at it | ||
| git checkout -b try-pr-238 9eb23f9a56edaeec95e2dfcfc8389b11bfd777b6 | ||
| ``` | ||
| </details> |
There was a problem hiding this comment.
Remove duplicated PR checkout instructions.
This <details> block is identical to the one on lines 122–133. Likely another rebase artifact.
Proposed fix
-<details>
-<summary>Pull the code locally from PR `#238`</summary>
-```bash
-# 1. Clone the repo (if you haven't already)
-git clone https://github.com/Ryan-Millard/Img2Num.git
-cd Img2Num
-# 2. Fetch the specific commit from the PR
-git fetch origin 9eb23f9a56edaeec95e2dfcfc8389b11bfd777b6
-# 3. Create a local branch pointing at it
-git checkout -b try-pr-238 9eb23f9a56edaeec95e2dfcfc8389b11bfd777b6
-```
-</details>
-📝 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.
| <details> | |
| <summary>Pull the code locally from PR #238</summary> | |
| ```bash | |
| # 1. Clone the repo (if you haven't already) | |
| git clone https://github.com/Ryan-Millard/Img2Num.git | |
| cd Img2Num | |
| # 2. Fetch the specific commit from the PR | |
| git fetch origin 9eb23f9a56edaeec95e2dfcfc8389b11bfd777b6 | |
| # 3. Create a local branch pointing at it | |
| git checkout -b try-pr-238 9eb23f9a56edaeec95e2dfcfc8389b11bfd777b6 | |
| ``` | |
| </details> |
🤖 Prompt for AI Agents
In `@docs/docs/reference/wasm/modules/image/graph/explained.md` around lines 135 -
146, Remove the duplicated <details> block that contains the "Pull the code
locally from PR `#238`" summary and the bash snippet; keep only one instance of
that block (the original on lines earlier in the file) so the instructions
aren’t repeated, and ensure the surrounding Markdown remains valid (matching
opening and closing <details> and ``` fences).
| ### `struct QuadBezier` | ||
|
|
||
| Represents a Quadratic Bezier curve segment. This is used when the raw pixel contours are approximated or smoothed into vector paths. | ||
|
|
||
| | Member | Type | Description | | ||
| | :----- | :------ | :--------------------------------------------------------------------------------------------------------------------------------- | | ||
| | `p0` | `Point` | **Start Point:** The anchor point where the curve begins. | | ||
| | `p1` | `Point` | **Control Point:** The handle that determines the curve's tangent and shape. The curve generally does not pass through this point. | | ||
| | `p2` | `Point` | **End Point:** The anchor point where the curve ends. | | ||
|
|
||
| --- | ||
|
|
||
| ## 2. Contour Results | ||
|
|
||
| ### `struct ContoursResult` | ||
|
|
||
| The primary container for the output of the contour extraction algorithm (e.g., Suzuki-Abe). It separates the raw pixel data from the topological relationship data. | ||
|
|
||
| #### Member Variables | ||
|
|
||
| | Member Variable | Type | Description | | ||
| | :-------------- | :--------------------------- | :-------------------------------------------------------------------------------------------------------------------- | | ||
| | `contours` | `vector<vector<Point>>` | A list of contours. `contours[k]` is a vector of `Point`s tracing the boundary of the $k$-th region. | | ||
| | `curves` | `vector<vector<QuadBezier>>` | A vectorized representation of `contours`. `curves[k]` contains the Bezier segments approximating the $k$-th contour. | | ||
| | `hierarchy` | `vector<array<int, 4>>` | Topological tree structure describing how contours nest within each other (see **Hierarchy Structure** below). | | ||
| | `is_hole` | `vector<bool>` | Flags the type of border. `true` if `contours[k]` is an internal hole; `false` if it is an external boundary. | |
There was a problem hiding this comment.
Documentation is missing the new CubicBezier struct and ccurves member.
This PR adds struct CubicBezier and ContoursResult::ccurves in contours.h, but this documentation page only covers QuadBezier and omits ccurves from the ContoursResult member table (line 125–130). Consider adding:
- A
### struct CubicBeziersection (afterQuadBezier) documentingp0,p1,p2,p3. - A row for
ccurves(vector<vector<CubicBezier>>) in theContoursResulttable.
🤖 Prompt for AI Agents
In `@docs/docs/reference/wasm/modules/image/graph/explained2.md` around lines 105
- 130, Add documentation for the newly introduced CubicBezier struct and the
ContoursResult::ccurves member: insert a "### `struct CubicBezier`" section
after the QuadBezier section that documents members `p0`, `p1`, `p2`, `p3` (each
as Point with start/control/control/end descriptions), and add a row to the
ContoursResult member table describing `ccurves` with type
`vector<vector<CubicBezier>>` and a short description like "Vectorized cubic
Bezier representation of contours; ccurves[k] contains the cubic segments for
the k-th contour."
| bilateralFilter: async ({ | ||
| pixels, | ||
| width, | ||
| height, | ||
| sigma_spatial = 3, | ||
| sigma_range = 50, | ||
| color_space = 0, | ||
| n_threads = 8, | ||
| }) => { |
There was a problem hiding this comment.
Formatting change looks good, but Prettier CI is failing on this file.
The multi-line destructuring aligns with other functions in this file (e.g., kmeans at Line 87). However, the CI pipeline reports a Prettier formatting issue on this file. Please run prettier --write to resolve it before merging.
🤖 Prompt for AI Agents
In `@src/hooks/useWasmWorker.js` around lines 61 - 69, Prettier CI is failing for
src/hooks/useWasmWorker.js due to formatting in the bilateralFilter parameter
destructuring; run the formatter (e.g., prettier --write
src/hooks/useWasmWorker.js) or reformat the bilateralFilter async ({ ... })
block to match the project's Prettier style used elsewhere (see kmeans function
parameter formatting) so the file passes CI.
| for (auto &c : node_contours.ccurves) { | ||
| all_contours.ccurves.push_back(c); | ||
| } |
There was a problem hiding this comment.
Accumulating ccurves with no consumer.
As noted above, all_contours.ccurves is populated here but contoursResultToSVG does not use it. This is unnecessary work until cubic curve rendering is wired up.
🤖 Prompt for AI Agents
In `@src/wasm/modules/image/src/labels_to_svg.cpp` around lines 283 - 285, The
loop that pushes node_contours.ccurves into all_contours.ccurves is doing
unnecessary work because contoursResultToSVG does not use all_contours.ccurves;
remove the accumulation to avoid wasted work (delete the for-loop that copies
node_contours.ccurves into all_contours.ccurves) or, if you prefer to keep the
intent, wrap it behind a clear TODO/feature flag and comment referencing future
cubic curve rendering; update or remove any dependent comments and ensure
contoursResultToSVG continues to operate on the used contours fields only.
| // Weights: Normal = 1.0, Fixed = 10000.0 (Hard constraint) | ||
| float wNormal = 1.0; | ||
| float wFixed = 100.0; |
There was a problem hiding this comment.
Misleading comment: wFixed is 100.0, not 10000.0.
The comment reads "Hard constraint" with 10000.0, but the actual value is 100.0. Either update the comment or the value.
✏️ Fix comment
- // Weights: Normal = 1.0, Fixed = 10000.0 (Hard constraint)
+ // Weights: Normal = 1.0, Fixed = 100.0 (Hard constraint)
float wNormal = 1.0;
float wFixed = 100.0;📝 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.
| // Weights: Normal = 1.0, Fixed = 10000.0 (Hard constraint) | |
| float wNormal = 1.0; | |
| float wFixed = 100.0; | |
| // Weights: Normal = 1.0, Fixed = 100.0 (Hard constraint) | |
| float wNormal = 1.0; | |
| float wFixed = 100.0; |
🤖 Prompt for AI Agents
In `@src/wasm/modules/image/src/SavitskyGolay.cpp` around lines 82 - 84, The
comment above the weight declarations in SavitskyGolay.cpp is inconsistent: it
says "Fixed = 10000.0 (Hard constraint)" while the code defines float wFixed =
100.0; update either the comment or the value so they match—preferably change
the comment to "Fixed = 100.0 (Hard constraint)" or, if the intention was
10000.0, change the initializer for wFixed to 10000.0; ensure you update the
adjacent comment referencing wNormal and wFixed so the message and the actual
values remain consistent.
| for (size_t i = 0; i < data.size(); ++i) { | ||
| Point val{0.0, 0.0}; | ||
| float A[3][3] = {0}; | ||
| float Bx[3] = {0}; | ||
| float By[3] = {0}; | ||
| // if (locked[i] || corner[i]) { continue; } | ||
| for (int j = -window_radius_; j <= window_radius_; ++j) { | ||
| int k = i + j; | ||
| if (k < 0) { | ||
| k = data.size() + k; | ||
| } else if (k >= data.size()) { | ||
| k = k - data.size(); | ||
| } |
There was a problem hiding this comment.
Signed/unsigned mismatch in wrap-around index arithmetic.
int k = i + j; where i is size_t and j is int. When j is negative, j is implicitly converted to size_t (a huge unsigned value), producing an incorrect intermediate before truncation to int. This is technically undefined behavior if the result doesn't fit in int.
The same pattern exists in filter_wrap (line 55), but it's worth fixing here. Cast i to int first.
🛡️ Proposed fix
- int k = i + j;
+ int k = static_cast<int>(i) + j;📝 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.
| for (size_t i = 0; i < data.size(); ++i) { | |
| Point val{0.0, 0.0}; | |
| float A[3][3] = {0}; | |
| float Bx[3] = {0}; | |
| float By[3] = {0}; | |
| // if (locked[i] || corner[i]) { continue; } | |
| for (int j = -window_radius_; j <= window_radius_; ++j) { | |
| int k = i + j; | |
| if (k < 0) { | |
| k = data.size() + k; | |
| } else if (k >= data.size()) { | |
| k = k - data.size(); | |
| } | |
| for (size_t i = 0; i < data.size(); ++i) { | |
| Point val{0.0, 0.0}; | |
| float A[3][3] = {0}; | |
| float Bx[3] = {0}; | |
| float By[3] = {0}; | |
| // if (locked[i] || corner[i]) { continue; } | |
| for (int j = -window_radius_; j <= window_radius_; ++j) { | |
| int k = static_cast<int>(i) + j; | |
| if (k < 0) { | |
| k = data.size() + k; | |
| } else if (k >= data.size()) { | |
| k = k - data.size(); | |
| } |
🤖 Prompt for AI Agents
In `@src/wasm/modules/image/src/SavitskyGolay.cpp` around lines 86 - 98, The loop
uses size_t i and int j then computes int k = i + j, causing signed/unsigned
mismatch and UB; fix by converting i to a signed type before arithmetic (e.g.,
int idx = static_cast<int>(i); then use int k = idx + j) and apply the same
change in the other occurrence in filter_wrap; update uses of i in this loop
(and any wrap-around adjustments) to use idx to avoid implicit conversions.
| float SavitzkyGolay::solveQuadraticAtZero(float A[3][3], float B[3]) { | ||
| // Determinant of A (Symmetric) | ||
| float det = A[0][0] * (A[1][1] * A[2][2] - A[1][2] * A[2][1]) - | ||
| A[0][1] * (A[1][0] * A[2][2] - A[1][2] * A[2][0]) + | ||
| A[0][2] * (A[1][0] * A[2][1] - A[1][1] * A[2][0]); | ||
|
|
||
| if (std::abs(det) < 1e-9) return 0.0; // Singularity check | ||
|
|
||
| // We only need the first element of the solution vector (c0), | ||
| // which corresponds to the polynomial value at x=0. | ||
| // Cramer's Rule for x[0]: Replace first column of A with B, divide det by main det. | ||
|
|
||
| // Matrix A_0: | ||
| // [ B0 A01 A02 ] | ||
| // [ B1 A11 A12 ] | ||
| // [ B2 A21 A22 ] | ||
|
|
||
| float det0 = B[0] * (A[1][1] * A[2][2] - A[1][2] * A[2][1]) - | ||
| A[0][1] * (B[1] * A[2][2] - A[1][2] * B[2]) + | ||
| A[0][2] * (B[1] * A[2][1] - A[1][1] * B[2]); | ||
|
|
||
| return det0 / det; | ||
| } No newline at end of file |
There was a problem hiding this comment.
Returning 0.0 on singular matrix silently maps the point to the origin.
When solveQuadraticAtZero encounters a near-singular matrix (line 220), it returns 0.0 for both x and y coordinates, effectively teleporting the point to (0, 0). This would produce a visible spike/artifact in the contour.
A safer fallback is to return the original data value (the center sample), preserving the point when the system is ill-conditioned.
🐛 Proposed fix — pass the original value as fallback
Change the function signature to accept a fallback value:
-float SavitzkyGolay::solveQuadraticAtZero(float A[3][3], float B[3]) {
+float SavitzkyGolay::solveQuadraticAtZero(float A[3][3], float B[3], float fallback) {
// ...
- if (std::abs(det) < 1e-9) return 0.0; // Singularity check
+ if (std::abs(det) < 1e-9) return fallback; // Singularity: keep original
// ...And at the call sites (lines 131–132):
- val.x = solveQuadraticAtZero(A, Bx);
- val.y = solveQuadraticAtZero(A, By);
+ val.x = solveQuadraticAtZero(A, Bx, data[i].x);
+ val.y = solveQuadraticAtZero(A, By, data[i].y);🤖 Prompt for AI Agents
In `@src/wasm/modules/image/src/SavitskyGolay.cpp` around lines 214 - 236, The
function solveQuadraticAtZero currently returns 0.0 on near-singular matrices
which maps the point to the origin; change its signature to accept a fallback
value (e.g., float fallback) and when std::abs(det) < 1e-9 return that fallback
instead of 0.0, and update all call sites that invoke solveQuadraticAtZero to
pass the center sample (original data value) as the fallback so ill-conditioned
solves preserve the original point rather than teleporting it to (0,0).
|
rebase is so bad 😠 This is also optional... if you think it's not worth the merge then i'm ok with that. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/wasm/modules/image/src/contours.cpp (1)
835-852:⚠️ Potential issue | 🟡 MinorRemove endpoint guard to align with closed-loop treatment.
The SG filter
filter_wrap_with_constraints(line 774) processes all indices including endpoints using wrap-around neighbors, producing valid smoothed targets for all contour points. The main loop (line 794) now iterates all points including endpoints. However, the partner target calculation (line 835) still excludes endpoints (op > 0 && op < (int)otherContour.size() - 1), forcing partners at endpoints to snap to their current position (line 850) instead of using their smoothed target. This guard is inconsistent with the closed-loop design and prevents endpoint partners from contributing their computed smoothed targets.
🤖 Fix all issues with AI agents
In `@src/wasm/modules/image/src/contours.cpp`:
- Around line 730-759: The smoothing loop mistakenly reads from the in-place
vector pts (causing Gauss-Seidel order dependence) even though an unmodified
snapshot original is created; update the inner convolution to read from original
(use original[k] instead of pts[k] when computing val) so the filter is
Jacobi-style and independent of index order while keeping the existing checks
(isLocked, Corner logic) and using radius/coeff/Point as before.
- Around line 744-756: The loop mixes signed and unsigned types causing UB when
adding negative j to size_t i; change the loop index to a signed type (e.g., int
or ptrdiff_t) and compute a signed container length once (e.g., int n =
static_cast<int>(pts.size()) or ptrdiff_t n = pts.size()), then replace uses of
pts.size() in the wrap logic with that signed n and compute k as a signed index
(int k = i + j) so expressions like pts.size() + k and k >= pts.size() become
well-defined; update the for loop header (i), the n variable, and the wrap
branches accordingly for variables pts, isLocked, radius, and k.
🧹 Nitpick comments (3)
src/wasm/modules/image/src/contours.cpp (3)
10-10: Unused<iostream>include.The debug
std::coutstatements were removed per a previous review, but the<iostream>header is still included. Nostd::cout/std::cerrusage remains in this file.🧹 Remove unused include
-#include <iostream>
725-738: Recursivestd::functionfactorial is slow and fragile.
std::functioninvolves heap allocation and virtual dispatch on every call. More importantly,intoverflow occurs atfactorial(13), so anyradius ≥ 7silently produces wrong coefficients. A simple iterative binomial coefficient computation avoids both problems.♻️ Proposed replacement — compute binomial coefficients iteratively
- std::function<int(int)> factorial = [&factorial](int x) -> int { - if (x <= 1) return 1; - return x * factorial(x-1); - }; - std::vector<Point> original = pts; int window = 2 * radius + 1; std::vector<float> coeff(window); - // binomial coefficients - for (int k = 0; k < window; ++k){ - //n!/((n-k)! * k!) - coeff[k] = static_cast<float>(factorial(window-1)/ (factorial(k) * factorial(window-1-k))); - } + // Compute binomial coefficients iteratively (overflow-safe for reasonable radii) + coeff[0] = 1.0f; + for (int k = 1; k < window; ++k) { + coeff[k] = coeff[k - 1] * static_cast<float>(window - 1 - (k - 1)) / static_cast<float>(k); + }
796-840: Remove commented-out code.Several commented-out fragments remain (lines 701–702, 796, 836, 839–840). These are dead vestiges from development iterations. If the logic may be needed again, it can be recovered from version control.
| std::vector<Point> original = pts; | ||
| for (size_t i = 1; i < pts.size() - 1; ++i) { | ||
| int window = 2 * radius + 1; | ||
|
|
||
| std::vector<float> coeff(window); | ||
| // binomial coefficients | ||
| for (int k = 0; k < window; ++k){ | ||
| //n!/((n-k)! * k!) | ||
| coeff[k] = static_cast<float>(factorial(window-1)/ (factorial(k) * factorial(window-1-k))); | ||
| } | ||
| float sum = std::accumulate(coeff.begin(), coeff.end(), 0.0f); | ||
| for (int k = 0; k < window; ++k){ | ||
| coeff[k] /= sum; | ||
| } | ||
|
|
||
| for (size_t i = 0; i < pts.size(); ++i) { | ||
| // DO NOT move if it's a Corner OR if it's Locked on the boundary | ||
| if (isLocked[i]) | ||
| continue; | ||
|
|
||
| Point val{0,0}; | ||
| for (int j = -radius; j <= radius; ++j) { | ||
| int k = i + j; | ||
| if (k < 0) { | ||
| k = pts.size() + k; | ||
| } else if (k >= pts.size()) { | ||
| k = k - pts.size(); | ||
| } | ||
| val += pts[k] * coeff[j+radius]; | ||
| } | ||
| pts[i] = val; |
There was a problem hiding this comment.
Bug: smoothing reads from in-place modified pts instead of the original snapshot.
original is created on line 730 as a snapshot but is never referenced. The inner loop on line 757 reads pts[k], which has already been modified for indices < i, producing order-dependent (Gauss-Seidel) smoothing instead of the intended Jacobi-style filter. This introduces directional bias — especially problematic on closed contours where the seam at the iteration start point will be visible.
🐛 Fix: read from `original` instead of `pts`
Point val{0,0};
for (int j = -radius; j <= radius; ++j) {
int k = i + j;
if (k < 0) {
k = pts.size() + k;
} else if (k >= pts.size()) {
k = k - pts.size();
}
- val += pts[k] * coeff[j+radius];
+ val += original[k] * coeff[j+radius];
}
pts[i] = val;🤖 Prompt for AI Agents
In `@src/wasm/modules/image/src/contours.cpp` around lines 730 - 759, The
smoothing loop mistakenly reads from the in-place vector pts (causing
Gauss-Seidel order dependence) even though an unmodified snapshot original is
created; update the inner convolution to read from original (use original[k]
instead of pts[k] when computing val) so the filter is Jacobi-style and
independent of index order while keeping the existing checks (isLocked, Corner
logic) and using radius/coeff/Point as before.
| for (size_t i = 0; i < pts.size(); ++i) { | ||
| // DO NOT move if it's a Corner OR if it's Locked on the boundary | ||
| if (isLocked[i]) | ||
| continue; | ||
|
|
||
| Point val{0,0}; | ||
| for (int j = -radius; j <= radius; ++j) { | ||
| int k = i + j; | ||
| if (k < 0) { | ||
| k = pts.size() + k; | ||
| } else if (k >= pts.size()) { | ||
| k = k - pts.size(); | ||
| } |
There was a problem hiding this comment.
Signed/unsigned mixing: size_t i + int j → int k is implementation-defined.
On line 751, j (negative int) is promoted to size_t before adding to i, producing a wrapped unsigned value. Assigning that to int k is implementation-defined. Same pattern on line 753 (pts.size() + k where k is negative). Use a signed loop index to avoid this:
🛡️ Proposed fix
- for (size_t i = 0; i < pts.size(); ++i) {
+ int n = static_cast<int>(pts.size());
+ for (int i = 0; i < n; ++i) {
if (isLocked[i])
continue;
Point val{0,0};
for (int j = -radius; j <= radius; ++j) {
int k = i + j;
if (k < 0) {
- k = pts.size() + k;
- } else if (k >= pts.size()) {
- k = k - pts.size();
+ k = n + k;
+ } else if (k >= n) {
+ k = k - n;
}🤖 Prompt for AI Agents
In `@src/wasm/modules/image/src/contours.cpp` around lines 744 - 756, The loop
mixes signed and unsigned types causing UB when adding negative j to size_t i;
change the loop index to a signed type (e.g., int or ptrdiff_t) and compute a
signed container length once (e.g., int n = static_cast<int>(pts.size()) or
ptrdiff_t n = pts.size()), then replace uses of pts.size() in the wrap logic
with that signed n and compute k as a signed index (int k = i + j) so
expressions like pts.size() + k and k >= pts.size() become well-defined; update
the for loop header (i), the n variable, and the wrap branches accordingly for
variables pts, isLocked, radius, and k.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/wasm/modules/image/src/contours.cpp`:
- Around line 700-710: The loop uses pts.size() (size_t) mixed with signed ints
causing warnings; cache the container size into an int (e.g., int n =
static_cast<int>(pts.size())) at the start and use n for all wrap arithmetic and
comparisons for previ/nexti (variables in this loop), then replace expressions
like if (previ < 0) previ = pts.size() + previ and if (nexti >= pts.size()) ...
with logic using n (e.g., add n when previ is negative, subtract n when nexti >=
n) so all math and comparisons are performed with signed ints.
🧹 Nitpick comments (3)
src/wasm/modules/image/src/contours.cpp (3)
10-10: Unused<iostream>include.The debug
std::coutstatements were removed (per prior review), but the<iostream>header was left behind. It's no longer referenced anywhere in this file.🧹 Proposed fix
-#include <iostream>
725-738: Factorial viastd::functionis fragile and inefficient.
std::functionincurs type-erasure overhead for every call. More importantly,intoverflow occurs forradius ≥ 7(factorial(13) > INT_MAX), and the intermediate productfactorial(k) * factorial(window-1-k)can overflow even sooner.Since the binomial coefficients are small here, consider an iterative multiplicative formula that avoids large intermediate factorials entirely, or at minimum use a plain function:
♻️ Suggested replacement — iterative binomial coefficient
- std::function<int(int)> factorial = [&factorial](int x) -> int { - if (x <= 1) return 1; - return x * factorial(x-1); - }; - std::vector<Point> original = pts; int window = 2 * radius + 1; std::vector<float> coeff(window); - // binomial coefficients - for (int k = 0; k < window; ++k){ - //n!/((n-k)! * k!) - coeff[k] = static_cast<float>(factorial(window-1)/ (factorial(k) * factorial(window-1-k))); - } + // Binomial coefficients via multiplicative recurrence (no overflow for small window) + coeff[0] = 1.0f; + for (int k = 1; k < window; ++k) { + coeff[k] = coeff[k - 1] * static_cast<float>(window - 1 - (k - 1)) / static_cast<float>(k); + }This also lets you remove the
#include <functional>on Line 9 if it's not used elsewhere.
796-842: Clean up commented-out code.Several commented-out lines remain across this function (Lines 701–702, 796, 836–837, 841–842). Version control preserves the history — shipping dead comments adds noise and makes review harder.
| for (int i = 0; i < pts.size(); ++i) { | ||
| // int previ = i > 0 ? i - 1 : pts.size() - 1; | ||
| // int nexti = i < pts.size() - 1 ? i + 1 : 0; | ||
| int previ = i - 2; | ||
| int nexti = i + 2; | ||
| if (previ < 0) { | ||
| previ = pts.size() + previ; | ||
| } | ||
| if (nexti >= pts.size()) { | ||
| nexti = nexti - pts.size(); | ||
| } |
There was a problem hiding this comment.
Signed/unsigned mixing in wrap-around arithmetic.
pts.size() returns size_t. On Line 706, adding a negative previ to it triggers implicit unsigned conversion; on Line 708, nexti >= pts.size() is a signed-vs-unsigned comparison. Both produce compiler warnings and rely on implementation-defined behavior.
Cache the size as int once and use it throughout:
🛡️ Proposed fix
+ int n = static_cast<int>(pts.size());
- for (int i = 0; i < pts.size(); ++i) {
+ for (int i = 0; i < n; ++i) {
int previ = i - 2;
int nexti = i + 2;
if (previ < 0) {
- previ = pts.size() + previ;
+ previ = n + previ;
}
- if (nexti >= pts.size()) {
- nexti = nexti - pts.size();
+ if (nexti >= n) {
+ nexti = nexti - n;
}🤖 Prompt for AI Agents
In `@src/wasm/modules/image/src/contours.cpp` around lines 700 - 710, The loop
uses pts.size() (size_t) mixed with signed ints causing warnings; cache the
container size into an int (e.g., int n = static_cast<int>(pts.size())) at the
start and use n for all wrap arithmetic and comparisons for previ/nexti
(variables in this loop), then replace expressions like if (previ < 0) previ =
pts.size() + previ and if (nexti >= pts.size()) ... with logic using n (e.g.,
add n when previ is negative, subtract n when nexti >= n) so all math and
comparisons are performed with signed ints.
Yeah.😂😂 When I first used it, I messed up horribly because I didn't realize that the commit hashes changed. |
|
@Krasner, what exactly is the impact of this PR? The visual changes aren't overtly obvious to me, but the paths look slightly rounder now. Is that correct? |
|
Yep just that. This is optional |
|
This doesn't look like a good addition to our code because it makes straight edges rounder, which pushes use closer to lossier output. We need to try to match the image as precisely as possible without creating bad SVGs. I also want to take a moment to thank you for your sustained interest in this project. It's really nice to have someone else working on it and your contributions are always wonderful. |
2 participants
Note: rebasing is a mess. but this should be up-to-date with main.
A few updates to get smoother contours possibly:
filter_wrap_with_constraintstries to prevent points on the borders from moving. https://github.com/Krasner/Img2Num/blob/0c610818b03a25a51e7321f40b29ebc1bc3b2742/src/wasm/modules/image/src/contours.cpp#L781C32-L781C60SG filter has some ringing to it, so further apply binomial smoothing
https://github.com/Krasner/Img2Num/blob/0c610818b03a25a51e7321f40b29ebc1bc3b2742/src/wasm/modules/image/src/contours.cpp#L782
for (int i = 1; i < pts.size() - 1; i++)where looking back would bei-1and looking ahead isi+1. now all those loops arefor (int i = 0; i < pts.size() ; i++)with the correct logic to wrap around. This prevents some distortion on the end points.Summary by CodeRabbit
Documentation
New Features