refactor(api)!: remove draw_contour_borders from labels_to_svg#283
Merged
Conversation
github-actions
Bot
added
documentation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoved the Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant JS as JS caller
end
rect rgba(200,255,200,0.5)
participant WASM as WASM wrapper
end
rect rgba(255,200,200,0.5)
participant C as C binding
end
participant Core as img2num core
JS->>WASM: findContours(pixels, labels, width, height, min_area)
WASM->>C: labels_to_svg(pixels, labels, width, height, min_area)
C->>Core: labels_to_svg(const data, const labels, width, height, min_area)
Core-->>C: malloc'd SVG char*
C-->>WASM: SVG char*
WASM-->>JS: { svg, visualization }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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
force-pushed
the
refactor!/api/labels_to_svg
branch
from
7c87bb0 to
76783ff
Compare
- BREAKING CHANGE: labels_to_svg signature changed; contour border visualization support has been removed across all bindings. - Affected scope: C, C++, JS
Ryan-Millard
force-pushed
the
refactor!/api/labels_to_svg
branch
from
76783ff to
25ca61c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/internal/labels_to_svg.cpp (1)
212-220:⚠️ Potential issue | 🟠 MajorDead code:
resultsimage is computed but never used.The
resultsimage is populated with recolored pixels, but sincedatais nowconst, the old code that copiedresultsback intodatawas removed. Thisresultsvariable is now computed but:
- Never returned
- Cannot be written to
data(const)- Goes out of scope at function end
This means the JS wrapper's
visualizationoutput will just return the original unmodified pixel data, not the recolored visualization.If visualization is intentionally being removed (consistent with removing
draw_contour_borders), this code block should be deleted to avoid confusion:🔧 Option A: Remove dead code if visualization is no longer needed
- // 5. recolor image on new regions - ImageLib::Image<ImageLib::RGBAPixel<uint8_t>> results{width, height}; - for (auto &n : G.get_nodes()) { - if (n->area() == 0) continue; - - auto [r, g, b] = n->color(); - for (auto &[_, p] : n->get_pixels()) { - results(p.x, p.y) = {r, g, b}; - } - }If visualization should still work, consider adding an output parameter or changing the return type.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/internal/labels_to_svg.cpp` around lines 212 - 220, The block building the ImageLib::Image<ImageLib::RGBAPixel<uint8_t>> results is dead because results is never returned or written back into the now-const data; either remove the entire loop (delete results and the loop over G.get_nodes(), references to n->color() and n->get_pixels()) if visualization is intentionally removed, or restore visualization by exposing a writable output (e.g., change the function return or add an output parameter) and copy results into that output instead of writing to const data; locate the code using symbols results, G.get_nodes(), n->color(), and n->get_pixels() and apply one of these two fixes consistently (also align with removal of draw_contour_borders).
🧹 Nitpick comments (2)
packages/js/safeWasmWrappers.js (1)
187-192: JSDoc describes "optional" visualization, but the option was removed.The
@returnsdescription says "optionally pixels with visualized contours" and@variationsays "optional visualization", but thedraw_contour_bordersparameter that controlled this behavior has been removed. The visualization output is now always populated with recolored pixels.📝 Suggested fix
-* `@returns` {Promise<{svg: string, visualization: Uint8ClampedArray}>} Generated SVG and optionally pixels with visualized contours. +* `@returns` {Promise<{svg: string, visualization: Uint8ClampedArray}>} Generated SVG and recolored pixels. * `@throws` {Error} If the WASM function fails or input labels are invalid. * `@example` * const { svg, visualization } = await findContours({ pixels, labels, width, height }); * `@todo` Support additional visualization options like color-coded regions. -* `@variation` Contour extraction with optional visualization +* `@variation` Contour extraction with visualization🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/js/safeWasmWrappers.js` around lines 187 - 192, Update the JSDoc for findContours in safeWasmWrappers.js to reflect that visualization is always returned: remove references to "optionally" and "@variation" mentioning optional visualization, delete any mention of the removed parameter draw_contour_borders, and change the `@returns` description to state that both svg and visualization (recolored pixels) are always provided; ensure the `@throws/`@example remain valid and adjust the `@todo` if it implied optional visualization.bindings/js/src/wasm_wrapper.cpp (1)
36-39: Consider addingconstto match the C API signature.The wrapper parameters
dataandlabelsare non-const, butimg2num_labels_to_svgnow expectsconst uint8_t*andconst int32_t*. While this works (implicit conversion), matching the const-qualification would improve consistency across the API surface.♻️ Optional: align const-qualification
-EMSCRIPTEN_KEEPALIVE char *labels_to_svg(uint8_t *data, int32_t *labels, const int width, +EMSCRIPTEN_KEEPALIVE char *labels_to_svg(const uint8_t *data, const int32_t *labels, const int width, const int height, const int min_area) { return img2num_labels_to_svg(data, labels, width, height, min_area); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindings/js/src/wasm_wrapper.cpp` around lines 36 - 39, Update the EMSCRIPTEN_KEEPALIVE wrapper function labels_to_svg so its parameter const-qualification matches the underlying API: change the types of data and labels to const uint8_t* and const int32_t* respectively in the labels_to_svg signature and then forward them unchanged to img2num_labels_to_svg; this keeps the wrapper consistent with img2num_labels_to_svg's signature and avoids implicit non-const-to-const conversions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/docs/internal/packages/js/index.md`:
- Around line 132-136: The documentation snippet uses an invalid
function-call-like expression inside the args block; update the example so args
is a proper object containing the properties pixels, labels, width, height, and
min_area (e.g., args: { pixels, labels, width, height, min_area }) rather than
(pixels, labels, width, height, min_area); — edit the code block under the args
example (look for the args: { ... } block) and replace the parenthesized
expression with the comma-separated property list.
---
Outside diff comments:
In `@core/src/internal/labels_to_svg.cpp`:
- Around line 212-220: The block building the
ImageLib::Image<ImageLib::RGBAPixel<uint8_t>> results is dead because results is
never returned or written back into the now-const data; either remove the entire
loop (delete results and the loop over G.get_nodes(), references to n->color()
and n->get_pixels()) if visualization is intentionally removed, or restore
visualization by exposing a writable output (e.g., change the function return or
add an output parameter) and copy results into that output instead of writing to
const data; locate the code using symbols results, G.get_nodes(), n->color(),
and n->get_pixels() and apply one of these two fixes consistently (also align
with removal of draw_contour_borders).
---
Nitpick comments:
In `@bindings/js/src/wasm_wrapper.cpp`:
- Around line 36-39: Update the EMSCRIPTEN_KEEPALIVE wrapper function
labels_to_svg so its parameter const-qualification matches the underlying API:
change the types of data and labels to const uint8_t* and const int32_t*
respectively in the labels_to_svg signature and then forward them unchanged to
img2num_labels_to_svg; this keeps the wrapper consistent with
img2num_labels_to_svg's signature and avoids implicit non-const-to-const
conversions.
In `@packages/js/safeWasmWrappers.js`:
- Around line 187-192: Update the JSDoc for findContours in safeWasmWrappers.js
to reflect that visualization is always returned: remove references to
"optionally" and "@variation" mentioning optional visualization, delete any
mention of the removed parameter draw_contour_borders, and change the `@returns`
description to state that both svg and visualization (recolored pixels) are
always provided; ensure the `@throws/`@example remain valid and adjust the `@todo`
if it implied optional visualization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2ee38b33-d510-4d61-bcb7-e077d0420560
📒 Files selected for processing (8)
bindings/c/include/cimg2num.hbindings/c/src/cimg2num.cppbindings/js/src/wasm_wrapper.cppcore/include/img2num.hcore/src/internal/labels_to_svg.cppdocs/docs/internal/packages/js/index.mddoxygen/img2num.h.doxpackages/js/safeWasmWrappers.js
💤 Files with no reviewable changes (1)
- doxygen/img2num.h.dox
…-Millard/Img2Num into refactor!/api/labels_to_svg
Owner
Author
|
@Krasner, please have a look at this. This PR just removes the |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
example-apps/console-c/main.c (1)
94-94:⚠️ Potential issue | 🔴 CriticalUse assignment, not comparison, in the fopen error path.
Line 94 uses
==instead of=, soexit_codeis never set on file-open failure.Proposed fix
- exit_code == 1; + exit_code = 1;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example-apps/console-c/main.c` at line 94, The fopen error path currently uses a comparison instead of an assignment (exit_code == 1), so exit_code is never set; change that to an assignment (exit_code = 1) in the error handling branch where fopen fails, ensuring the variable exit_code is updated before returning/exiting from main or the surrounding function that handles file open errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@example-apps/console-c/main.c`:
- Line 77: The call to img2num_labels_to_svg may return NULL (res_svg) but the
code writes it with fputs unguarded; add a NULL check after the call to
img2num_labels_to_svg (check res_svg != NULL) and handle the failure before
calling fputs/printf or fclose: log or print an error, skip writing the SVG, and
ensure any allocated resources (img_data, out_labels, file handles) are still
cleaned up; specifically guard the use of res_svg where it’s passed to fputs and
free the buffer (or avoid fputs) only if res_svg is non-NULL.
---
Outside diff comments:
In `@example-apps/console-c/main.c`:
- Line 94: The fopen error path currently uses a comparison instead of an
assignment (exit_code == 1), so exit_code is never set; change that to an
assignment (exit_code = 1) in the error handling branch where fopen fails,
ensuring the variable exit_code is updated before returning/exiting from main or
the surrounding function that handles file open errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d97df2ac-b9f5-4c5f-893a-51fdb3dab472
📒 Files selected for processing (2)
example-apps/console-c/main.cexample-apps/console-cpp/main.cpp
github-actions
Bot
added
example-app
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
example-apps/console-cpp/main.cpp (2)
55-57: Remove commented-out dead code.The commented-out allocation on line 56 is redundant since
img_datais already allocated at line 49. This appears to be leftover from the refactor.🧹 Proposed fix
// Allocate a copy of the original image - // uint8_t* img_data{new uint8_t[width * height * NUM_CHANNELS]}; std::memcpy(img_data, image_data_original, static_cast<size_t>(width) * static_cast<size_t>(height) * NUM_CHANNELS);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example-apps/console-cpp/main.cpp` around lines 55 - 57, Remove the dead commented-out allocation statement "uint8_t* img_data{new uint8_t[width * height * NUM_CHANNELS]};"—it is redundant because img_data is already allocated earlier (see img_data) and the subsequent std::memcpy uses image_data_original and NUM_CHANNELS; simply delete that commented line to clean up the code.
24-24: Remove unusedMAX_ITERconstant.This constant is no longer used after the iteration loop was removed.
🧹 Proposed fix
constexpr int NUM_CHANNELS{4}; constexpr double SIGMA_WIDTH_RATIO{0.005}; -constexpr int MAX_ITER{100};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example-apps/console-cpp/main.cpp` at line 24, Remove the now-unused constant MAX_ITER from the file by deleting the declaration "constexpr int MAX_ITER{100};" (referenced symbol: MAX_ITER) in main (main.cpp); ensure no remaining references to MAX_ITER exist and run a build to confirm nothing else depends on it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@example-apps/console-cpp/main.cpp`:
- Line 65: The call to img2num::labels_to_svg can return nullptr on allocation
failure but res_svg is used unguarded when writing to svgFile; add a null check
after char* res_svg{...} and handle the failure path (e.g., log an
error/throw/return non-zero) before using svgFile << res_svg, and ensure any
allocated res_svg is freed when successful; reference res_svg,
img2num::labels_to_svg, and svgFile to locate the code to change.
---
Nitpick comments:
In `@example-apps/console-cpp/main.cpp`:
- Around line 55-57: Remove the dead commented-out allocation statement
"uint8_t* img_data{new uint8_t[width * height * NUM_CHANNELS]};"—it is redundant
because img_data is already allocated earlier (see img_data) and the subsequent
std::memcpy uses image_data_original and NUM_CHANNELS; simply delete that
commented line to clean up the code.
- Line 24: Remove the now-unused constant MAX_ITER from the file by deleting the
declaration "constexpr int MAX_ITER{100};" (referenced symbol: MAX_ITER) in main
(main.cpp); ensure no remaining references to MAX_ITER exist and run a build to
confirm nothing else depends on it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 34d24b20-24df-4aa5-8bc3-6030cf741995
📒 Files selected for processing (2)
example-apps/console-c/main.cexample-apps/console-cpp/main.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- example-apps/console-c/main.c
…emory management - check for NULL return from labels_to_svg - fix assignment bug (== → =) when setting exit_code - free res_svg after use in C example to avoid memory leak - simplify C++ example by removing unnecessary loop and redundant reallocations - ensure proper ownership and cleanup of SVG buffer in C++ example
Ryan-Millard
force-pushed
the
refactor!/api/labels_to_svg
branch
from
64ced44 to
22fe897
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was changed & why
The
labels_to_svgfunction signature has changed. It now marks more arguments asconstand removesdraw_contour_borders.This is important as it ensures all the function has a single responsibility: vectorize labels.
Fixes: none
Changes
Entire API around
labels_to_svg.Testing & Verification
It runs and builds fine.
Additional Resources
Summary by CodeRabbit
Breaking Changes
Documentation