fix(core)!: prevent holes during SVG generation - #429
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds shared-edge contour construction, junction-aware smoothing and curve reduction (Bézier + Douglas–Peucker), refactors Graph::compute_contours to use shared loops by default, and exposes a new min_thickness option through core APIs and language bindings. ChangesShared-edge contours with junction-aware smoothing
Sequence Diagram(s)sequenceDiagram
participant GraphCC as Graph::compute_contours
participant BuildShared as build_shared_loops
participant SharedEdge as canonical edge extraction
participant Fit as fit_curve_reduction / dp_curve_reduction
participant Assemble as per-region loop assembly
GraphCC->>BuildShared: labels grid, w, h, eps
BuildShared->>SharedEdge: build crack graph, find junctions
SharedEdge->>Fit: canonical edge corner chains
Fit-->>SharedEdge: fitted QuadBezier per canonical edge
BuildShared->>Assemble: directed region traversals
Assemble->>Fit: orient/reuse fitted curves
Assemble-->>GraphCC: label→loop map
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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: 12
🤖 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.
Inline comments:
In `@core/include/internal/contours.h`:
- Line 52: Add a Doxygen docstring for the function coupled_smooth_junctions
describing that it is a variant of coupled_smooth which additionally locks
junction pixels during smoothing, document the parameter semantics: contours
(vector of contour point vectors), bounds (Rect image bounds), junctions (a
junction mask where nonzero entries mark junction pixels to be
preserved/locked), and width (image width used for raster indexing), and explain
any return/side-effect behavior (modifies contours in-place). Also mention
relationship to coupled_smooth and intended usage (junction locking during
smoothing).
In `@core/include/internal/douglas_peucker.h`:
- Around line 9-26: Convert the existing block comment above dp_curve_reduction
into a Doxygen comment block: add a brief description with `@brief` summarizing
Douglas–Peucker point reduction, then add `@param` tags for each parameter
(chains, fixed, results, eps) and also document the implicit/relevant parameters
described in the text (retract_eps, and the environment overrides IMG2NUM_DP_EPS
and IMG2NUM_DP_RETRACT) and use `@note` or `@details` for behavior about fixed[i][k]
junctions, boundary retraction limits, and that kept points are emitted as
straight-line quads; keep the same content but reformat it to a /** ... */
Doxygen style above the dp_curve_reduction declaration.
In `@core/include/internal/graph.h`:
- Around line 42-47: Add Doxygen-style comments above the two new protected
methods in core/include/internal/graph.h: document getPixel(const
std::vector<uint8_t>& img, int w, int h, int x, int y) to describe parameters,
state that it performs bounds checking and returns 0 for out-of-bounds
coordinates, and explain the returned uint8_t pixel value; document
analyzeJunctions(const std::vector<uint8_t>& skel, int w, int h) to describe
parameters, state that it uses an 8-neighbor crossing-number algorithm to detect
junctions, and explain the return semantics (a vector<uint8_t> mask the same
size as the image where nonzero entries mark junction pixels).
In `@core/include/internal/shared_contours.h`:
- Around line 11-28: The public function build_shared_loops lacks a Doxygen
docstring; replace the plain comments above it with a Doxygen-style block
describing the function, its purpose, parameters, return value, and any
important notes (e.g. coordinate system, treatment of out-of-bounds as exterior,
that edges are canonical and reused), and annotate parameters `@param` labels,
`@param` w, `@param` h, `@param` eps, and `@return` std::unordered_map<int32_t,
std::vector<std::vector<QuadBezier>>> so the API shows up in generated docs;
keep the existing explanatory content but format it using /** ... */ Doxygen
tags and include types/names (QuadBezier, build_shared_loops) for clarity.
In `@core/src/internal/bezier.cpp`:
- Around line 158-186: fit_curve_reduction currently assumes fixed has the same
outer and per-chain sizes as chains; add an upfront validation that fixed.size()
== chains.size() and for each i verify fixed[i].size() == chains[i].size() (or
at least >= chains[i].size()) and handle violations (e.g., throw
std::invalid_argument or assert) before the existing loop so mismatched masks
are caught early and interior junctions aren't silently ignored; reference
function fit_curve_reduction and variables fixed, chains and the loop that uses
fixed[i].size() to locate where to add the checks.
In `@core/src/internal/contours.cpp`:
- Line 694: Add a brief inline comment next to the SavitzkyGolay sg(3, 2)
instantiation explaining why the radius was increased (e.g., to better smooth
junction-aware seam continuity and reduce spurious peaks across junctions),
noting trade-offs (higher cost and potential over-smoothing) and whether this
value is intentionally fixed or should be made configurable (reference
SavitzkyGolay sg(3, 2) and the junction-aware smoothing code path so reviewers
can decide if a runtime/config option is needed).
- Around line 652-662: updateLockedMasks currently computes idx = pt.y * width +
pt.x and indexes junctions without bounds checks; fix by validating pt.x and
pt.y are >= 0, width > 0, compute height = junctions.size() / width (guard
against division by zero and ensure junctions.size() % width == 0 or use
size()/width semantics), ensure pt.x < width and pt.y < height, and verify idx <
junctions.size() before accessing junctions[idx]; also validate locked[c].size()
> p before writing locked[c][p]; if any check fails simply skip that point so
out-of-bounds contour points do not access memory.
- Around line 794-797: Remove the two debug std::cout lines that print "boundary
masks" and "update junctions" surrounding the call to createBoundaryMask;
specifically delete the prints near the creation of lockedMasks (the call to
createBoundaryMask(contours, bounds)) so production code does not emit console
debug output—if runtime diagnostics are required instead, replace with the
project's logging facility rather than std::cout.
In `@core/src/internal/douglas_peucker.cpp`:
- Around line 57-110: The function dp_curve_reduction assumes fixed has the same
outer size as chains and uses fixed[i].size() later; add upfront validation:
check fixed.size() == chains.size() and for each i verify fixed[i].size() ==
chains[i].size() (or at least >= chains[i].size()) and fail fast (e.g.,
return/error/throw or assert/log) if not; perform this validation at the start
of dp_curve_reduction before any processing so mismatched dimensions don't
silently drop interior junctions used by dp_reduce and the bounds/fixed logic.
In `@core/src/internal/graph.cpp`:
- Around line 260-279: compute_contours() was changed to always use the new
shared-edge path (build_shared_loops) but the PR lacks regression tests; add
tests that cover the reported gap artifact and topology-sensitive cases
(donut/holed regions and diagonal-touch junctions) and gate the new behavior
behind a toggle so tests can exercise both implementations. Specifically, add
unit/regression tests that call compute_contours() (or invoke
build_shared_loops() directly) with crafted inputs for: (1) the previously
failing gap artifact, (2) a donut-shaped region, and (3) diagonal-touch
junctions; implement a temporary runtime flag (e.g., enable_shared_edge or
similar) or parameter to compute_contours()/build_shared_loops so tests can run
both the legacy and shared-edge paths and assert identical/topologically-correct
contours before making the shared-edge branch the unconditional default.
- Around line 281-297: The loop repopulates Node::m_contours but overwrites
containment metadata by hard-coding n->m_contours.hierarchy to {-1,-1,-1,-1} and
n->m_contours.is_hole to false; instead, after pushing all
contours/curves/colors for a given node (inside the get_nodes() loop when
handling loops[n->id()]), rebuild the contour hierarchy and hole flags for
n->m_contours before leaving the node: compute each contour's parent by testing
containment (point-in-polygon or bounding-box quick-reject) and set
n->m_contours.hierarchy entries accordingly, then set n->m_contours.is_hole
using nesting parity or contour orientation (or call the project’s existing
contour-hierarchy utility if one exists) so holes and nesting information are
preserved rather than hard-coded.
In `@core/src/internal/shared_contours.cpp`:
- Line 15: Remove the in-band OUTSIDE sentinel and instead represent the canvas
exterior out-of-band: delete the OUTSIDE constant in shared_contours.cpp and
change build_shared_loops() (and its callers) to accept an explicit exterior
marker (e.g. std::optional<int32_t> exterior_id or a separate bool/flag) rather
than treating a numeric label as "outside"; update all checks that compared
labels to OUTSIDE to use the new out-of-band indicator so a real region id equal
to -2147483647 is no longer co-opted and the exterior boundary handling is kept
separate.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0b7874a5-e114-4078-b09b-63c040b60fbf
📒 Files selected for processing (10)
core/include/internal/bezier.hcore/include/internal/contours.hcore/include/internal/douglas_peucker.hcore/include/internal/graph.hcore/include/internal/shared_contours.hcore/src/internal/bezier.cppcore/src/internal/contours.cppcore/src/internal/douglas_peucker.cppcore/src/internal/graph.cppcore/src/internal/shared_contours.cpp
📜 Review details
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Documentation Site / Build Docusaurus Site
- GitHub Check: Build C/C++ / Build WASM (bindings/js)
- GitHub Check: Build C/C++ / Build Python
- GitHub Check: Build C/C++ / Build C & C++
- GitHub Check: Lint & Validate Code
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{c,cc,cpp,cxx,h,hpp,hxx}
📄 CodeRabbit inference engine (.clang-format)
**/*.{c,cc,cpp,cxx,h,hpp,hxx}: Follow Google style guide for C/C++ code formatting
Use 4 spaces for indentation
Maintain a column limit of 100 characters per line
Do not allow short functions on a single line
Files:
core/include/internal/contours.hcore/include/internal/bezier.hcore/src/internal/bezier.cppcore/include/internal/graph.hcore/include/internal/shared_contours.hcore/src/internal/shared_contours.cppcore/src/internal/contours.cppcore/include/internal/douglas_peucker.hcore/src/internal/douglas_peucker.cppcore/src/internal/graph.cpp
**/*.{hpp,cpp,c,h}
📄 CodeRabbit inference engine (.editorconfig)
**/*.{hpp,cpp,c,h}: Use 4-space indentation for C/C++ files
Maintain 120 character maximum line length for C/C++ files
Files:
core/include/internal/contours.hcore/include/internal/bezier.hcore/src/internal/bezier.cppcore/include/internal/graph.hcore/include/internal/shared_contours.hcore/src/internal/shared_contours.cppcore/src/internal/contours.cppcore/include/internal/douglas_peucker.hcore/src/internal/douglas_peucker.cppcore/src/internal/graph.cpp
core/**/*.{cpp,c,h,hpp}
⚙️ CodeRabbit configuration file
core/**/*.{cpp,c,h,hpp}: This is the Img2Num core C/C++ library. Review for:
- Memory safety: null pointer dereferences, use-after-free, buffer overflows.
- Correct RAII usage and smart pointer idioms.
- Adherence to the .clang-format style; formatting must be applied via
./img2num format-cpp(or./img2num format-wasmfor WASM modules),
NOT by calling clang-format directly.- When suggesting build/test steps, always use the Docker-first wrapper scripts
(./img2num,img2num.ps1, orimg2num.bat) rather than direct tool invocations,
since contributors may not have dependencies installed locally.- Must have Doxygen docstrings in the API (
img2num.h) files. This is important!
Files:
core/include/internal/contours.hcore/include/internal/bezier.hcore/src/internal/bezier.cppcore/include/internal/graph.hcore/include/internal/shared_contours.hcore/src/internal/shared_contours.cppcore/src/internal/contours.cppcore/include/internal/douglas_peucker.hcore/src/internal/douglas_peucker.cppcore/src/internal/graph.cpp
**/*.{cpp,c,h,hpp}
⚙️ CodeRabbit configuration file
**/*.{cpp,c,h,hpp}: - For any C/C++ code outside core/ (e.g. bindings, example-apps), apply the same
memory-safety and style standards as the core. Formatting via./img2num format-cpp.
- Doxygen docstrings are required.
Files:
core/include/internal/contours.hcore/include/internal/bezier.hcore/src/internal/bezier.cppcore/include/internal/graph.hcore/include/internal/shared_contours.hcore/src/internal/shared_contours.cppcore/src/internal/contours.cppcore/include/internal/douglas_peucker.hcore/src/internal/douglas_peucker.cppcore/src/internal/graph.cpp
**
⚙️ CodeRabbit configuration file
**: # Contributing to Img2NumWant to contribute to Img2Num? There are a few things you need to know.
We wrote a contribution guide to help you get started.
A few important points:
- Add tests with your PR — new features and bug fixes must include tests where appropriate. PRs without tests are unlikely to be approved.
- Follow the repository's coding style rules.
- Use the issue and PR templates when filing issues or submitting code. Your PR will be rejected if you don't.
If you're unsure what to change, open a discussion and someone will assist you.
Questions?
If you have questions or need help:
- Open a discussion
- Create an issue
- Check existing PRs for ideas
Thank you for improving Img2Num! 🎨🚀
**: BasedOnStyle: Google
IndentWidth: 4
ColumnLimit: 100
AllowShortFunctionsOnASingleLine: false
**: root = true-------------------------
Global defaults
-------------------------
[*]
charset = utf-8
end_of_line = lf
indent_style = space
indent_size = 2
trim_trailing_whitespace = true
insert_final_newline = true
max_line_length = 120-------------------------
C++ / C headers
-------------------------
[*.{hpp,cpp,c,h}]
indent_style = space
indent_size = 4
charset = utf-8
end_of_line = lf
trim_trailing_whitespace = true
insert_final_newline = true
max_line_length = 120-------------------------
CMake
-------------------------
[CMakeLists.txt]
indent_style = space
indent_size = 2
charset = utf-8
end_of_line = lf
trim_trailing_whitespace = true
insert_final_newline = true
max_line_length = 120[*.cmake]
indent_style = space
indent_size = 2
charset = utf-8
end_of_line = lf
trim_trailing_whitespace = true
insert_final_newline = true
max_line_length = 120-------------------------
Ja...
Files:
core/include/internal/contours.hcore/include/internal/bezier.hcore/src/internal/bezier.cppcore/include/internal/graph.hcore/include/internal/shared_contours.hcore/src/internal/shared_contours.cppcore/src/internal/contours.cppcore/include/internal/douglas_peucker.hcore/src/internal/douglas_peucker.cppcore/src/internal/graph.cpp
🧠 Learnings (9)
📓 Common learnings
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-25T16:20:34.359Z
Learning: In the Ryan-Millard/Img2Num repository, the following follow-up issues were created after merging PR `#307` (Python bindings): `#328` (Python docstrings/docs), `#329` (numpy img.shape inference for width/height), `#330` (.coderabbit.yaml Python tooling config), `#331` (CI workflow for Python bindings build test).
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-01T20:45:20.732Z
Learning: In the Ryan-Millard/Img2Num repository, a PR body containing generic boilerplate such as "This contribution was made as part of an open-source documentation improvement initiative. All changes are meant to be helpful additions to the project." is a strong signal of an AI-generated slop PR and should be explicitly flagged during review.
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-05-01T00:15:20.172Z
Learning: In Ryan-Millard/Img2Num, the correct release propagation strategy uses TWO GitHub Actions jobs added to release.yml:
1. `propagate-core`: fires when `core_created == 'true'` → creates separate PRs (not direct commits) for bindings/c, packages/js, and packages/py using `git commit --allow-empty` with conventional commit messages and `gh pr create`. Requires RELEASE_PROPAGATION_TOKEN.
2. `propagate-c`: fires when `c_created == 'true' AND core_created != 'true'` → creates a PR for packages/js only (the guard prevents double-bumping JS when core already propagated to it in the same run).
Dependency graph: core → {bindings/c, packages/js, packages/py}; bindings/c → {packages/js}. packages/py depends directly on core (not via bindings/c). PRs must be created (not direct commits) so Ryan-Millard can review each propagation before merging.
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 339
File: release-please-config.json:3-3
Timestamp: 2026-05-23T23:39:14.889Z
Learning: In Ryan-Millard/Img2Num (release-please-config.json), `separate-pull-requests: true` is intentional and correct. Each package (`core`, `bindings/c`, `packages/js`, `packages/py`) is versioned independently because they have separate logic. A change to `packages/js` or `packages/py` does not imply a change to `core`, so a single combined release PR is not desired. Do not flag `separate-pull-requests: true` as an issue in this repository.
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 339
File: release-please-config.json:3-3
Timestamp: 2026-05-23T23:39:14.889Z
Learning: In Ryan-Millard/Img2Num (release-please-config.json), `separate-pull-requests: true` is intentional and correct. Each package (`core`, `bindings/c`, `packages/js`, `packages/py`) is versioned independently because they each have their own separate package-level logic. A change to `packages/js` or `packages/py` does not imply a change to `core`, so a single combined release PR is explicitly not desired. Do not flag `separate-pull-requests: true` as an issue in this repository.
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-23T18:48:02.692Z
Learning: In the Ryan-Millard/Img2Num repository, PR `#311` adds `.coderabbit.yaml` to main. PR `#307` (adding Python .py files) must rebase onto main after `#311` merges so that `.coderabbit.yaml` exists and can be updated with: `ruff: enabled: true`, `pylint: enabled: true`, `flake8: enabled: false` under `tools:`, and a `path_instructions` entry for `**/*.py` covering PEP 8, type annotations, exception handling, resource management, mutable default arguments, and Docker-first command suggestions via the `./img2num` wrapper. The "CodeRabbit Config Needs Update" custom check will automatically flag `#307` if the Python config is missing.
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 335
File: core/src/internal/image_to_svg.cpp:16-19
Timestamp: 2026-05-04T16:50:52.142Z
Learning: In the Img2Num project (Ryan-Millard/Img2Num), the existing Gaussian blur implementation uses zero-padding, which causes a vignette-like darkening artifact at image edges. Because of this, Gaussian blur is not currently a suitable drop-in alternative to bilateral_filter in the image_to_svg pipeline. Any future PR adding a GaussianBlurConfig / SmoothingMethod::GAUSSIAN option must first fix the zero-padding boundary handling (e.g., replicate or reflect padding) before it can be safely exposed as a user-selectable smoothing stage.
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-23T18:48:02.712Z
Learning: In the Ryan-Millard/Img2Num repository, PR `#311` adds `.coderabbit.yaml` to main. PR `#307` (adding Python .py files) must rebase onto main after `#311` merges so that `.coderabbit.yaml` exists and can be updated with: `ruff: enabled: true`, `pylint: enabled: true`, `flake8: enabled: false` under `tools:`, and a `path_instructions` entry for `**/*.py` covering PEP 8, type annotations, exception handling, resource management, mutable default arguments, and Docker-first command suggestions via the `./img2num` wrapper. The "CodeRabbit Config Needs Update" custom check will automatically flag `#307` if the Python config is missing.
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-25T16:31:13.204Z
Learning: In the Ryan-Millard/Img2Num repository, issue `#331` (CI workflow for Python bindings build test) was addressed in PR `#307` via the `build-py` job in `.github/workflows/cmake-build.yml`. That job runs `uv venv`, `uv sync --no-build-isolation`, a smoke test (`uv run python3 -c "import img2num;"`), and uploads artifacts.
📚 Learning: 2026-02-25T21:24:34.055Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 272
File: core/include/internal/bilateral_filter_gpu.h:108-109
Timestamp: 2026-02-25T21:24:34.055Z
Learning: In WGSL shaders, .rgb swizzle on a vec4 returns the first three components regardless of color space. Do not assume data is RGB color space when the texture contains non-RGB data (e.g., CIELAB L, A, B). If LAB data is stored, treat and compute distances in LAB space, not RGB. Clearly document shader code paths that rely on specific color spaces and prefer explicit conversions or comments when using swizzled components with non-RGB textures. Apply this guidance to WGSL shader files across the codebase (not just this header) when handling color data or color-like channels.
Applied to files:
core/include/internal/contours.hcore/include/internal/bezier.hcore/include/internal/graph.hcore/include/internal/shared_contours.hcore/include/internal/douglas_peucker.h
📚 Learning: 2026-04-09T19:05:40.514Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 302
File: core/include/internal/gpu.h:203-216
Timestamp: 2026-04-09T19:05:40.514Z
Learning: In Img2Num’s internal GPU initialization code, if querying adapter limits fails (e.g., `adapter.GetLimits(...)` returns `false`), treat this as a GPU initialization failure and route execution to the CPU fallback path. Do not continue with default limits in this case; the failure should cause the same fallback behavior as other GPU init errors.
Applied to files:
core/include/internal/contours.hcore/include/internal/bezier.hcore/include/internal/graph.hcore/include/internal/shared_contours.hcore/include/internal/douglas_peucker.h
📚 Learning: 2025-12-31T17:46:54.476Z
Learnt from: fransafu
Repo: Ryan-Millard/Img2Num PR: 176
File: src/wasm/modules/image/src/bilateral_filter.cpp:98-101
Timestamp: 2025-12-31T17:46:54.476Z
Learning: In bilateral_filter.cpp (src/wasm/modules/image/src/bilateral_filter.cpp), rely on the fact that the center pixel contributes a weight of exactly 1.0 to both spatial and range components (exp(0) = 1) so the normalization sum cannot be zero. This implies you do not need explicit guards against division by zero for normalization in this implementation.
Applied to files:
core/src/internal/bezier.cppcore/src/internal/shared_contours.cppcore/src/internal/contours.cppcore/src/internal/douglas_peucker.cppcore/src/internal/graph.cpp
📚 Learning: 2026-01-06T04:56:57.269Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 191
File: src/wasm/modules/image/src/kmeans.cpp:128-134
Timestamp: 2026-01-06T04:56:57.269Z
Learning: In C++ sources compiled for WASM with Emscripten/Clang, designated initializers (e.g., RGBXY{.r = ..., .g = ...}) are allowed as a C++17 extension. When reviewing code that relies on designated initializers, verify that the target toolchain enables CXX_STANDARD 17 or higher and that the build system (CMake/emsdk) uses Emscripten with a compatible clang. If not, avoid such initializers or provide portable alternatives.
Applied to files:
core/src/internal/bezier.cppcore/src/internal/shared_contours.cppcore/src/internal/contours.cppcore/src/internal/douglas_peucker.cppcore/src/internal/graph.cpp
📚 Learning: 2026-01-06T21:06:24.476Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 191
File: src/wasm/modules/image/src/cielab.cpp:43-57
Timestamp: 2026-01-06T21:06:24.476Z
Learning: In the Img2Num project, prefer recommending and using the provided docker/script wrappers (e.g., ./img2num format-wasm, ./img2num clean-wasm) instead of invoking local tools directly (e.g., clang-format -i). This reduces dependency requirements for users and ensures consistent tooling across environments. Apply this guidance to C++ source files under the project when reviewing changes.
Applied to files:
core/src/internal/bezier.cppcore/src/internal/shared_contours.cppcore/src/internal/contours.cppcore/src/internal/douglas_peucker.cppcore/src/internal/graph.cpp
📚 Learning: 2026-01-19T00:02:34.957Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 234
File: src/wasm/modules/image/src/kmeans.cpp:79-81
Timestamp: 2026-01-19T00:02:34.957Z
Learning: In C++ multithreading contexts, prefer unsigned types for thread-count-like parameters (e.g., n_threads) to prevent negative values. Validate that the value is at least 1 before any division to avoid divide-by-zero at runtime. At the start of functions handling such values, create a safe count like: const unsigned int thread_count{std::max(1u, n_threads)}; This ensures non-negative, non-zero usage for divisions and related arithmetic. Apply this pattern to similar parameters across C++ modules, not just the specific file.
Applied to files:
core/src/internal/bezier.cppcore/src/internal/shared_contours.cppcore/src/internal/contours.cppcore/src/internal/douglas_peucker.cppcore/src/internal/graph.cpp
📚 Learning: 2026-02-25T21:24:19.036Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 272
File: core/src/internal/bilateral_filter_gpu.cpp:171-185
Timestamp: 2026-02-25T21:24:19.036Z
Learning: Adopt brace initialization (e.g., Type var{}) over copy initialization (e.g., Type var = {}) for zero-initialization in C++ across the repository. This should apply to variables, arrays, and structs in most C++ files (e.g., core/src/internal/bilateral_filter_gpu.cpp). It improves safety by avoiding narrowing conversions and makes initialization intent explicit.
Applied to files:
core/src/internal/bezier.cppcore/src/internal/shared_contours.cppcore/src/internal/contours.cppcore/src/internal/douglas_peucker.cppcore/src/internal/graph.cpp
📚 Learning: 2026-04-02T18:39:24.330Z
Learnt from: Krasner
Repo: Ryan-Millard/Img2Num PR: 290
File: core/src/internal/graph.cpp:138-208
Timestamp: 2026-04-02T18:39:24.330Z
Learning: In the Ryan-Millard/Img2Num repository, `add_edge_pixel()` on a Node is intentionally designed to produce dual ownership of boundary pixels: a pixel may exist in one node's `m_pixels` AND in a neighboring node's `m_edge_pixels` simultaneously. This dual ownership is required for SVG generation to prevent a 1-pixel boundary gap artifact (the "1 pixel boundary problem"). Edge pixels do not influence node properties like color; they only expand the contour/binary representation for boundary tracing. Do not flag this dual ownership as a bug or misuse of the API.
Applied to files:
core/include/internal/graph.h
🪛 Cppcheck (2.20.0)
core/src/internal/shared_contours.cpp
[style] 59-59: The function 'build_shared_loops' is never used.
(unusedFunction)
core/src/internal/contours.cpp
[style] 793-793: The function 'coupled_smooth_junctions' is never used.
(unusedFunction)
core/src/internal/douglas_peucker.cpp
[style] 57-57: The function 'dp_curve_reduction' is never used.
(unusedFunction)
🔇 Additional comments (1)
core/include/internal/bezier.h (1)
6-15:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd Doxygen docstrings for both function overloads.
As per coding guidelines, Doxygen docstrings are required for C/C++ files in
core/. Bothfit_curve_reductionoverloads need proper Doxygen documentation describing their parameters, behavior, and return semantics. The existing comment at lines 9-12 provides good content but should be formatted as a Doxygen block (/**...*/with@param,@brieftags).📝 Proposed Doxygen format
+/** + * `@brief` Fits quadratic Bezier curves to a set of polyline chains with Douglas-Peucker reduction. + * + * `@param` chains Input polyline chains (each chain is a sequence of points) + * `@param` results Output Bezier curve segments for each chain + * `@param` tolerance Maximum deviation tolerance in pixels + */ void fit_curve_reduction(const std::vector<std::vector<Point>> &chains, std::vector<std::vector<QuadBezier>> &results, float tolerance); +/** + * `@brief` Fits quadratic Bezier curves with junction-aware segmentation. + * + * Chains are split at fixed junction points marked by the fixed mask, so each + * junction becomes an exact pinned endpoint. Chains with no fixed points behave + * identically to the overload above. + * + * `@param` chains Input polyline chains + * `@param` fixed Junction mask: fixed[i][k] != 0 marks point k of chain i as a junction + * `@param` results Output Bezier curve segments for each chain + * `@param` tolerance Maximum deviation tolerance in pixels + */ -// Same, but `fixed[i][k]!=0` marks point k of chain i as a junction that must NOT -// move: the chain is split at those points so each becomes an exact (pinned) -// curve endpoint. Chains with no fixed points fit identically to the overload -// above. void fit_curve_reduction(const std::vector<std::vector<Point>> &chains, const std::vector<std::vector<uint8_t>> &fixed, std::vector<std::vector<QuadBezier>> &results, float tolerance);> Likely an incorrect or invalid review comment.Source: Coding guidelines
| void fit_curve_reduction(const std::vector<std::vector<Point>> &chains, | ||
| const std::vector<std::vector<uint8_t>> &fixed, | ||
| std::vector<std::vector<QuadBezier>> &results, float tolerance) { | ||
| for (size_t i = 0; i < chains.size(); ++i) { | ||
| const std::vector<Point> &chain = chains[i]; | ||
| const int n = static_cast<int>(chain.size()); | ||
| std::vector<QuadBezier> result; | ||
| if (n < 2) { | ||
| results.push_back(result); | ||
| continue; | ||
| } | ||
|
|
||
| // Segment boundaries: chain ends plus every interior junction point. | ||
| std::vector<int> bounds; | ||
| bounds.push_back(0); | ||
| for (int k = 1; k < n - 1; ++k) | ||
| if (k < static_cast<int>(fixed[i].size()) && fixed[i][k]) bounds.push_back(k); | ||
| bounds.push_back(n - 1); | ||
|
|
||
| // Fit each [bounds[s], bounds[s+1]] piece; consecutive pieces share the | ||
| // junction point, so the curve stays continuous and pinned there. | ||
| for (size_t s = 0; s + 1 < bounds.size(); ++s) { | ||
| const int a = bounds[s], b = bounds[s + 1]; | ||
| std::vector<Point> seg(chain.begin() + a, chain.begin() + b + 1); | ||
| fitRecursive(seg, tolerance, result); | ||
| } | ||
| results.push_back(result); | ||
| } | ||
| } |
There was a problem hiding this comment.
Validate that fixed dimensions match chains dimensions.
The function assumes fixed[i].size() is compatible with chains[i].size(), but only checks the size at line 174 within the loop. If a caller provides a fixed mask with mismatched dimensions (e.g., fixed[i].size() < chains[i].size()), interior junctions beyond the fixed mask length will be silently ignored. Consider adding an upfront validation that fixed.size() == chains.size() and optionally that each fixed[i].size() == chains[i].size() to catch contract violations early.
🛡️ Proposed validation
void fit_curve_reduction(const std::vector<std::vector<Point>> &chains,
const std::vector<std::vector<uint8_t>> &fixed,
std::vector<std::vector<QuadBezier>> &results, float tolerance) {
+ if (fixed.size() != chains.size()) {
+ throw std::invalid_argument("fixed and chains must have the same outer dimension");
+ }
for (size_t i = 0; i < chains.size(); ++i) {
const std::vector<Point> &chain = chains[i];
const int n = static_cast<int>(chain.size());
+ if (i < fixed.size() && fixed[i].size() != static_cast<size_t>(n)) {
+ throw std::invalid_argument("fixed[i] and chains[i] must have the same size");
+ }
std::vector<QuadBezier> result;🤖 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 `@core/src/internal/bezier.cpp` around lines 158 - 186, fit_curve_reduction
currently assumes fixed has the same outer and per-chain sizes as chains; add an
upfront validation that fixed.size() == chains.size() and for each i verify
fixed[i].size() == chains[i].size() (or at least >= chains[i].size()) and handle
violations (e.g., throw std::invalid_argument or assert) before the existing
loop so mismatched masks are caught early and interior junctions aren't silently
ignored; reference function fit_curve_reduction and variables fixed, chains and
the loop that uses fixed[i].size() to locate where to add the checks.
There was a problem hiding this comment.
@Krasner please check this out. I'm not sure if this is even likely.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| for (const Node_ptr &n : get_nodes()) { | ||
| if (n->area() == 0) continue; | ||
| n->clear_contour(); | ||
| auto it = loops.find(n->id()); | ||
| if (it == loops.end()) continue; | ||
| ImageLib::RGBPixel<uint8_t> c = n->color(); | ||
| ImageLib::RGBAPixel<uint8_t> col{c.red, c.green, c.blue, 255}; | ||
| for (std::vector<QuadBezier> &curve : it->second) { | ||
| std::vector<Point> anchors; // keep contours[] parallel to curves[] | ||
| anchors.reserve(curve.size() + 1); | ||
| for (const QuadBezier &q : curve) anchors.push_back(q.p0); | ||
| if (!curve.empty()) anchors.push_back(curve.back().p2); | ||
| n->m_contours.contours.push_back(std::move(anchors)); | ||
| n->m_contours.curves.push_back(std::move(curve)); | ||
| n->m_contours.colors.push_back(col); | ||
| n->m_contours.hierarchy.push_back({-1, -1, -1, -1}); | ||
| n->m_contours.is_hole.push_back(false); |
There was a problem hiding this comment.
Shared-loop mode drops hole semantics from m_contours.
This repopulates Node::m_contours with every returned ring, but it hard-codes hierarchy to {-1, -1, -1, -1} and is_hole to false for all of them. That breaks the existing contour contract for regions with interior voids or nested islands: downstream consumers still receive multiple loops, but no longer know which ones are holes or how they nest. Please rebuild hole/containment metadata before storing the loops.
🤖 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 `@core/src/internal/graph.cpp` around lines 281 - 297, The loop repopulates
Node::m_contours but overwrites containment metadata by hard-coding
n->m_contours.hierarchy to {-1,-1,-1,-1} and n->m_contours.is_hole to false;
instead, after pushing all contours/curves/colors for a given node (inside the
get_nodes() loop when handling loops[n->id()]), rebuild the contour hierarchy
and hole flags for n->m_contours before leaving the node: compute each contour's
parent by testing containment (point-in-polygon or bounding-box quick-reject)
and set n->m_contours.hierarchy entries accordingly, then set
n->m_contours.is_hole using nesting parity or contour orientation (or call the
project’s existing contour-hierarchy utility if one exists) so holes and nesting
information are preserved rather than hard-coded.
There was a problem hiding this comment.
@Krasner please check this out. I understand that we don't really use the hierarchical information because we're more focused on the actual edges, but we may want to preserve this in the future.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Krasner
changed the title
|
Hi. This sounds good, but I still haven't gotten my PC properly fixed, so I can't review it easily.🥲 I'll try my best when I get the chance. Thank you! |
Ryan-Millard
changed the title
Oh right, I forgot... Hope you get your new PC soon! |
… prevent many very thin neighbors next to each other
github-actions
Bot
added
js/ts
example-app
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
core/src/internal/shared_contours.cpp (2)
1-57:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd Doxygen documentation for all functions in this core file.
As per coding guidelines, Doxygen docstrings are required for C/C++ files in
core/. Document each function:
smooth_edge: Explain the border-preserving smoothing algorithm, parameters, and the iterative averaging approachfit_edge: Document that it smooths then fits to quadratic Béziers, explain the eps parameter's rolereverse_curve: Describe the in-place reversal of Bézier curve direction🤖 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 `@core/src/internal/shared_contours.cpp` around lines 1 - 57, Add Doxygen-style comments for each internal function in this file: document smooth_edge to describe it performs endpoint- and border-preserving smoothing by iterative weighted averaging, list parameters (std::vector<Point> &p, int w, int h, int iters), explain that points on the image frame are locked and how many iterations control smoothing; document fit_edge to state it copies and smooths the corner polyline, then fits quadratic Bézier segments via fit_curve_reduction, describe parameters (const std::vector<Point> &corners, int w, int h, float eps) and that eps controls fitting tolerance and that the function returns a vector<QuadBezier> (possibly empty); document reverse_curve to state it reverses the order of QuadBezier segments in-place and swaps p0/p2 on each segment to flip direction (parameter: std::vector<QuadBezier> &c). Include brief `@param` and `@return` tags and mark these comments above the corresponding function definitions (smooth_edge, fit_edge, reverse_curve).Source: Coding guidelines
59-65:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate that eps is positive.
The
epsparameter controls curve-fitting tolerance but is never validated. Negative or zero values could cause unexpected behavior infit_curve_reduction. Add a guard at the function entry:🛡️ Proposed validation
std::unordered_map<int32_t, std::vector<std::vector<QuadBezier>>> build_shared_loops( const std::vector<int32_t> &labels, int w, int h, float eps) { + if (eps <= 0.0f) { + eps = 1.0f; // or throw, depending on error-handling policy + } const int W1 = w + 1; // corner grid width🤖 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 `@core/src/internal/shared_contours.cpp` around lines 59 - 65, In build_shared_loops, validate the eps parameter at function entry (e.g., if eps <= 0.0f) and guard against non-positive values by returning an error or throwing a std::invalid_argument (or using an assert) before any calls to fit_curve_reduction; reference the build_shared_loops function and the eps parameter so the check occurs immediately after the function signature and prevents downstream misuse in fit_curve_reduction.core/src/internal/graph.cpp (3)
212-258: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd Doxygen docstrings for
analyzeJunctions.As per coding guidelines, internal/API functions in the core library must have Doxygen docstrings explaining purpose, parameters, and return values.
📝 Suggested docstring
+/** + * `@brief` Classify skeleton pixels as junctions based on 8-neighbor transitions. + * `@param` skel Binary skeleton mask (1 = skeleton pixel, 0 = background). + * `@param` w Width of the mask. + * `@param` h Height of the mask. + * `@return` A mask where 1 indicates a junction pixel (3+ branches). + */ std::vector<uint8_t> Graph::analyzeJunctions(const std::vector<uint8_t>& skel, int w, int h) {🤖 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 `@core/src/internal/graph.cpp` around lines 212 - 258, Add a Doxygen-style comment block above the function Graph::analyzeJunctions describing its purpose, parameters, and return value: explain that it analyzes a binary skeleton image (parameter skel) of width w and height h, that it computes junction (and optionally endpoint) pixels using 8-neighbor crossing-number logic, list the parameters (const std::vector<uint8_t>& skel, int w, int h) and state the return type (std::vector<uint8_t> junction_map where 1 marks junction pixels and 0 marks non-junctions), and note any assumptions (row-major layout, pixel values nonzero = foreground). Ensure the comment uses Doxygen tags (`@brief`, `@param`, `@return`) and is placed immediately above the analyzeJunctions definition.Source: Coding guidelines
262-262:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
if (true)makes the legacy path dead code.The
if (true)on line 262 unconditionally takes the shared-edge branch, making the entireelseblock (lines 302-384) unreachable dead code. Either remove the dead code or use a proper compile-time or runtime flag to allow selection between implementations during testing/rollout.🔧 Suggested approach
If you intend to keep the legacy path for testing/comparison, consider a runtime or compile-time toggle:
- if (true) { + constexpr bool USE_SHARED_EDGE_CONTOURS = true; // TODO: remove legacy path after validation + if (USE_SHARED_EDGE_CONTOURS) {Or remove the dead code entirely if the legacy path is no longer needed.
🤖 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 `@core/src/internal/graph.cpp` at line 262, The unconditional "if (true)" branch in the shared-edge selection logic makes the legacy else block unreachable; either remove the legacy branch entirely (delete the else block and tests referencing it) or replace "if (true)" with a proper toggle (e.g., a compile-time macro or a runtime/config flag) so callers can choose between the shared-edge implementation and the legacy implementation during testing; update the surrounding function (the conditional guarding the shared-edge vs legacy path) to read that flag and add a short comment explaining the toggle's purpose.
343-343:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove debug
std::coutstatement from production code.Line 343 contains a debug print statement that should not remain in production code.
🗑️ Suggested fix
- std::cout << "Apply smoothing" << std::endl;🤖 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 `@core/src/internal/graph.cpp` at line 343, Remove the debug print statement `std::cout << "Apply smoothing" << std::endl;` from production code in core/src/internal/graph.cpp; locate the occurrence (inside the graph smoothing routine) and delete that line, or replace it with the project's logging facility if a persistent informational message is required (use the existing logger call pattern used elsewhere in the file/class).
🤖 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.
Inline comments:
In `@bindings/c/include/cimg2num.h`:
- Line 45: Add a documentation comment describing the purpose and units/range of
the min_thickness field in the struct where min_thickness is declared (in
cimg2num.h) so it matches the style used for other fields in the header; update
the comment format to mirror neighboring fields (e.g., brief one-line // or /**
*/ comment) and include that it represents the minimum stroke/line thickness
used by the algorithm (units/pixel integer, expected range or special values if
any).
In `@bindings/py/src/img2num_pybind.cpp`:
- Around line 116-126: The pybind wrapper for labels_to_svg currently requires
min_thickness as a positional argument, breaking existing callers; update the
pybind binding for the lambda that calls img2num::labels_to_svg so the
pybind11::arg("min_thickness") has a default value (e.g., = 10 to match the JS
binding or = 0 to disable), leaving the lambda signature and call unchanged but
changing the argument declaration to include that default and updating the final
docstring if needed.
In `@core/include/img2num.h`:
- Line 44: Add a Doxygen comment for the ImageToSvgConfig field min_thickness
describing what it controls, its units and typical valid range (e.g., "minimum
stroke thickness in pixels" and whether zero disables the check), matching the
style of nearby fields' docstrings; update the declaration of int min_thickness
= 0; in ImageToSvgConfig with a brief one-line /** ... */ above it that explains
purpose, units, default value, and any edge-case behavior.
In `@core/src/internal/graph.cpp`:
- Around line 387-470: Add brief Doxygen-style docstrings above the
anonymous-namespace helper functions dt_1d and max_inscribed_radius describing
their purpose, inputs, outputs, and any important invariants: for dt_1d explain
it implements the Felzenszwalb & Huttenlocher 1D squared-distance transform,
describe parameters (const std::vector<float> &f, std::vector<float> &d, int n)
and that d is filled with min_p((q-p)^2+f[p]); for max_inscribed_radius describe
it computes the largest inscribed-disk radius (in pixels) for a Node_ptr region,
note that it uses a padded binary mask, performs separable 2-pass transforms
calling dt_1d, and returns the radius (sqrt of max squared distance). Ensure the
comments are Doxygen formatted (brief summary, `@param`, `@return`) and placed
immediately above each function definition.
- Line 270: The local variable eps uses copy initialization; update its
declaration in graph.cpp (variable eps) to use brace initialization per repo
convention by declaring eps with braces around its initializer (retain the 0.25f
value) instead of using the equals-style initialization.
In `@core/src/internal/shared_contours.cpp`:
- Line 44: Replace the magic literal 5 passed to smooth_edge(pts, w, h, 5) with
a named constant or parameter: introduce a constexpr int (e.g.,
SMOOTHING_ITERATIONS = 5) near the top of the translation unit or expose an
argument on the public API and use that symbol in the call site; update any
function signature or callers as needed so smooth_edge(...) uses the named
constant or forwarded parameter instead of the hardcoded 5, keeping the
parameter name descriptive (e.g., smoothingIterations) and preserving current
behavior by defaulting to 5 if you add a new parameter.
In `@example-apps/console-c/main.c`:
- Line 74: Add an inline comment next to the call to img2num_labels_to_svg
explaining the sixth parameter (min_thickness): state that min_thickness is the
minimum stroke/segment thickness in pixels to include in the SVG (use 0 to
disable thickness filtering), suggest typical values (e.g. 1–3 for thin fonts)
and note its effect (filters out tiny noisy components). Update the comment near
the img2num_labels_to_svg(...) invocation to briefly describe this behavior and
recommended use.
In `@example-apps/console-cpp/main.cpp`:
- Around line 69-72: Add an inline comment above or next to the new
config.min_thickness assignment explaining what ImageToSvgConfig::min_thickness
controls, its units/range, default behavior when unset, and how it affects
image_to_svg output (e.g., it prevents stroke/path widths below this pixel
threshold from being emitted or merged). Update the example in main.cpp so the
comment sits beside config.min_thickness (reference symbols: ImageToSvgConfig,
config, min_thickness, image_to_svg) and keep the comment short and user-facing.
- Around line 67-72: res_svg is computed by calling img2num::labels_to_svg but
never used; either remove the unused call (delete the std::string res_svg{...}
line) or explicitly demonstrate both APIs by keeping the labels_to_svg call,
renaming its result (e.g., res_labels_svg), and writing it to a separate file
alongside res_svg2 created by img2num::image_to_svg; update identifiers res_svg
/ labels_to_svg / res_svg2 / image_to_svg and ensure img_data, out_labels,
width, height are passed correctly when writing both outputs.
In `@example-apps/console-py/main.py`:
- Line 26: The k parameter was increased to 64 for img2num.kmeans (and in
ImageToSvgConfig(kmeans={"k": 64})) to address the new shared-edge contour/holes
fix; add a concise inline comment next to the img2num.kmeans call and the
ImageToSvgConfig instantiation explaining that the higher k produces finer
clusters which helps avoid merged/shared-edge contours and prevents spurious
holes after the contour/hole-fix, and mention any trade-offs (slower runtime,
more SVG complexity); alternatively, add the same short note to the
example-apps/console-py README if you prefer centralized documentation.
- Line 29: Call to labels_to_svg is missing required width, height, min_area and
min_thickness arguments; update the call to pass img.shape[1] as width,
img.shape[0] as height, min_area as 100 and min_thickness as 0 so replace the
single-arg call to img2num.labels_to_svg(img, labels, 100) with a six-argument
call using labels_to_svg(img, labels, img.shape[1], img.shape[0], 100, 0).
---
Outside diff comments:
In `@core/src/internal/graph.cpp`:
- Around line 212-258: Add a Doxygen-style comment block above the function
Graph::analyzeJunctions describing its purpose, parameters, and return value:
explain that it analyzes a binary skeleton image (parameter skel) of width w and
height h, that it computes junction (and optionally endpoint) pixels using
8-neighbor crossing-number logic, list the parameters (const
std::vector<uint8_t>& skel, int w, int h) and state the return type
(std::vector<uint8_t> junction_map where 1 marks junction pixels and 0 marks
non-junctions), and note any assumptions (row-major layout, pixel values nonzero
= foreground). Ensure the comment uses Doxygen tags (`@brief`, `@param`, `@return`)
and is placed immediately above the analyzeJunctions definition.
- Line 262: The unconditional "if (true)" branch in the shared-edge selection
logic makes the legacy else block unreachable; either remove the legacy branch
entirely (delete the else block and tests referencing it) or replace "if (true)"
with a proper toggle (e.g., a compile-time macro or a runtime/config flag) so
callers can choose between the shared-edge implementation and the legacy
implementation during testing; update the surrounding function (the conditional
guarding the shared-edge vs legacy path) to read that flag and add a short
comment explaining the toggle's purpose.
- Line 343: Remove the debug print statement `std::cout << "Apply smoothing" <<
std::endl;` from production code in core/src/internal/graph.cpp; locate the
occurrence (inside the graph smoothing routine) and delete that line, or replace
it with the project's logging facility if a persistent informational message is
required (use the existing logger call pattern used elsewhere in the
file/class).
In `@core/src/internal/shared_contours.cpp`:
- Around line 1-57: Add Doxygen-style comments for each internal function in
this file: document smooth_edge to describe it performs endpoint- and
border-preserving smoothing by iterative weighted averaging, list parameters
(std::vector<Point> &p, int w, int h, int iters), explain that points on the
image frame are locked and how many iterations control smoothing; document
fit_edge to state it copies and smooths the corner polyline, then fits quadratic
Bézier segments via fit_curve_reduction, describe parameters (const
std::vector<Point> &corners, int w, int h, float eps) and that eps controls
fitting tolerance and that the function returns a vector<QuadBezier> (possibly
empty); document reverse_curve to state it reverses the order of QuadBezier
segments in-place and swaps p0/p2 on each segment to flip direction (parameter:
std::vector<QuadBezier> &c). Include brief `@param` and `@return` tags and mark
these comments above the corresponding function definitions (smooth_edge,
fit_edge, reverse_curve).
- Around line 59-65: In build_shared_loops, validate the eps parameter at
function entry (e.g., if eps <= 0.0f) and guard against non-positive values by
returning an error or throwing a std::invalid_argument (or using an assert)
before any calls to fit_curve_reduction; reference the build_shared_loops
function and the eps parameter so the check occurs immediately after the
function signature and prevents downstream misuse in fit_curve_reduction.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a90b7b18-6405-4b59-bd0c-3ec97350c238
📒 Files selected for processing (14)
bindings/c/include/cimg2num.hbindings/c/src/cimg2num.cppbindings/js/src/wasm_wrapper.cbindings/py/src/img2num_pybind.cppcore/include/img2num.hcore/include/internal/graph.hcore/src/internal/graph.cppcore/src/internal/image_to_svg.cppcore/src/internal/labels_to_svg.cppcore/src/internal/shared_contours.cppexample-apps/console-c/main.cexample-apps/console-cpp/main.cppexample-apps/console-py/main.pypackages/js/safeWasmWrappers.js
📜 Review details
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Documentation Site / Build Docusaurus Site
- GitHub Check: Build C/C++ / Build WASM (bindings/js)
- GitHub Check: Build C/C++ / Build C & C++
- GitHub Check: Build C/C++ / Build Python
- GitHub Check: Lint & Validate Code
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{c,cc,cpp,cxx,h,hpp,hxx}
📄 CodeRabbit inference engine (.clang-format)
**/*.{c,cc,cpp,cxx,h,hpp,hxx}: Follow Google style guide for C/C++ code formatting
Use 4 spaces for indentation
Maintain a column limit of 100 characters per line
Do not allow short functions on a single line
Files:
core/src/internal/image_to_svg.cppexample-apps/console-c/main.cbindings/c/include/cimg2num.hexample-apps/console-cpp/main.cppcore/include/img2num.hbindings/c/src/cimg2num.cppbindings/js/src/wasm_wrapper.ccore/include/internal/graph.hcore/src/internal/labels_to_svg.cppbindings/py/src/img2num_pybind.cppcore/src/internal/graph.cppcore/src/internal/shared_contours.cpp
**/*.{hpp,cpp,c,h}
📄 CodeRabbit inference engine (.editorconfig)
**/*.{hpp,cpp,c,h}: Use 4-space indentation for C/C++ files
Maintain 120 character maximum line length for C/C++ files
Files:
core/src/internal/image_to_svg.cppexample-apps/console-c/main.cbindings/c/include/cimg2num.hexample-apps/console-cpp/main.cppcore/include/img2num.hbindings/c/src/cimg2num.cppbindings/js/src/wasm_wrapper.ccore/include/internal/graph.hcore/src/internal/labels_to_svg.cppbindings/py/src/img2num_pybind.cppcore/src/internal/graph.cppcore/src/internal/shared_contours.cpp
core/**/*.{cpp,c,h,hpp}
⚙️ CodeRabbit configuration file
core/**/*.{cpp,c,h,hpp}: This is the Img2Num core C/C++ library. Review for:
- Memory safety: null pointer dereferences, use-after-free, buffer overflows.
- Correct RAII usage and smart pointer idioms.
- Adherence to the .clang-format style; formatting must be applied via
./img2num format-cpp(or./img2num format-wasmfor WASM modules),
NOT by calling clang-format directly.- When suggesting build/test steps, always use the Docker-first wrapper scripts
(./img2num,img2num.ps1, orimg2num.bat) rather than direct tool invocations,
since contributors may not have dependencies installed locally.- Must have Doxygen docstrings in the API (
img2num.h) files. This is important!
Files:
core/src/internal/image_to_svg.cppcore/include/img2num.hcore/include/internal/graph.hcore/src/internal/labels_to_svg.cppcore/src/internal/graph.cppcore/src/internal/shared_contours.cpp
**/*.{cpp,c,h,hpp}
⚙️ CodeRabbit configuration file
**/*.{cpp,c,h,hpp}: - For any C/C++ code outside core/ (e.g. bindings, example-apps), apply the same
memory-safety and style standards as the core. Formatting via./img2num format-cpp.
- Doxygen docstrings are required.
Files:
core/src/internal/image_to_svg.cppexample-apps/console-c/main.cbindings/c/include/cimg2num.hexample-apps/console-cpp/main.cppcore/include/img2num.hbindings/c/src/cimg2num.cppbindings/js/src/wasm_wrapper.ccore/include/internal/graph.hcore/src/internal/labels_to_svg.cppbindings/py/src/img2num_pybind.cppcore/src/internal/graph.cppcore/src/internal/shared_contours.cpp
**
⚙️ CodeRabbit configuration file
**: # Contributing to Img2NumWant to contribute to Img2Num? There are a few things you need to know.
We wrote a contribution guide to help you get started.
A few important points:
- Add tests with your PR — new features and bug fixes must include tests where appropriate. PRs without tests are unlikely to be approved.
- Follow the repository's coding style rules.
- Use the issue and PR templates when filing issues or submitting code. Your PR will be rejected if you don't.
If you're unsure what to change, open a discussion and someone will assist you.
Questions?
If you have questions or need help:
- Open a discussion
- Create an issue
- Check existing PRs for ideas
Thank you for improving Img2Num! 🎨🚀
**: BasedOnStyle: Google
IndentWidth: 4
ColumnLimit: 100
AllowShortFunctionsOnASingleLine: false
**: root = true-------------------------
Global defaults
-------------------------
[*]
charset = utf-8
end_of_line = lf
indent_style = space
indent_size = 2
trim_trailing_whitespace = true
insert_final_newline = true
max_line_length = 120-------------------------
C++ / C headers
-------------------------
[*.{hpp,cpp,c,h}]
indent_style = space
indent_size = 4
charset = utf-8
end_of_line = lf
trim_trailing_whitespace = true
insert_final_newline = true
max_line_length = 120-------------------------
CMake
-------------------------
[CMakeLists.txt]
indent_style = space
indent_size = 2
charset = utf-8
end_of_line = lf
trim_trailing_whitespace = true
insert_final_newline = true
max_line_length = 120[*.cmake]
indent_style = space
indent_size = 2
charset = utf-8
end_of_line = lf
trim_trailing_whitespace = true
insert_final_newline = true
max_line_length = 120-------------------------
Ja...
Files:
core/src/internal/image_to_svg.cppexample-apps/console-py/main.pyexample-apps/console-c/main.cbindings/c/include/cimg2num.hexample-apps/console-cpp/main.cppcore/include/img2num.hbindings/c/src/cimg2num.cppbindings/js/src/wasm_wrapper.ccore/include/internal/graph.hcore/src/internal/labels_to_svg.cpppackages/js/safeWasmWrappers.jsbindings/py/src/img2num_pybind.cppcore/src/internal/graph.cppcore/src/internal/shared_contours.cpp
example-apps/**
⚙️ CodeRabbit configuration file
example-apps/**: - Example applications. These are for demonstration; keep them minimal and ensure
they correctly reflect the public API. Flag any use of internal/private APIs.
- IMPORTANT: These example apps must be a good reflection of how to use Img2Num's
library, which means that they need good comments and must have clean code. This
is meant to be external and is designed for others to use to get started.
Files:
example-apps/console-py/main.pyexample-apps/console-c/main.cexample-apps/console-cpp/main.cpp
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Python source files. Review for:
- PEP 8 compliance (enforced by ruff; focus on logic, not style).
- Type annotation coverage — prefer typed signatures for public functions.
- Exception handling: avoid bare
except:clauses; always catch specific exceptions.- Resource management: use
withstatements for file/socket/connection handling.- No mutable default arguments (e.g.
def foo(x=[]):).- When suggesting how to run Python tooling, prefer the
./img2numDocker wrapper
rather than directpython/ruff/pylintinvocations.
Files:
example-apps/console-py/main.py
bindings/**
⚙️ CodeRabbit configuration file
bindings/**: Language bindings for the Img2Num library. Ensure the public API surface matches
the core C/C++ headers and that error propagation is handled correctly for each
binding language. Also ensure the relevant docstrings are present to enable
auto-generation of documentation.
Files:
bindings/c/include/cimg2num.hbindings/c/src/cimg2num.cppbindings/js/src/wasm_wrapper.cbindings/py/src/img2num_pybind.cpp
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.editorconfig)
**/*.{js,ts,jsx,tsx}: Use 2-space indentation for JavaScript and TypeScript files
Maintain 200 character maximum line length for JavaScript/TypeScript files
Files:
packages/js/safeWasmWrappers.js
🧠 Learnings (8)
📚 Learning: 2025-12-31T17:46:54.476Z
Learnt from: fransafu
Repo: Ryan-Millard/Img2Num PR: 176
File: src/wasm/modules/image/src/bilateral_filter.cpp:98-101
Timestamp: 2025-12-31T17:46:54.476Z
Learning: In bilateral_filter.cpp (src/wasm/modules/image/src/bilateral_filter.cpp), rely on the fact that the center pixel contributes a weight of exactly 1.0 to both spatial and range components (exp(0) = 1) so the normalization sum cannot be zero. This implies you do not need explicit guards against division by zero for normalization in this implementation.
Applied to files:
core/src/internal/image_to_svg.cppexample-apps/console-cpp/main.cppbindings/c/src/cimg2num.cppcore/src/internal/labels_to_svg.cppbindings/py/src/img2num_pybind.cppcore/src/internal/graph.cppcore/src/internal/shared_contours.cpp
📚 Learning: 2026-01-06T04:56:57.269Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 191
File: src/wasm/modules/image/src/kmeans.cpp:128-134
Timestamp: 2026-01-06T04:56:57.269Z
Learning: In C++ sources compiled for WASM with Emscripten/Clang, designated initializers (e.g., RGBXY{.r = ..., .g = ...}) are allowed as a C++17 extension. When reviewing code that relies on designated initializers, verify that the target toolchain enables CXX_STANDARD 17 or higher and that the build system (CMake/emsdk) uses Emscripten with a compatible clang. If not, avoid such initializers or provide portable alternatives.
Applied to files:
core/src/internal/image_to_svg.cppexample-apps/console-cpp/main.cppbindings/c/src/cimg2num.cppcore/src/internal/labels_to_svg.cppbindings/py/src/img2num_pybind.cppcore/src/internal/graph.cppcore/src/internal/shared_contours.cpp
📚 Learning: 2026-01-06T21:06:24.476Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 191
File: src/wasm/modules/image/src/cielab.cpp:43-57
Timestamp: 2026-01-06T21:06:24.476Z
Learning: In the Img2Num project, prefer recommending and using the provided docker/script wrappers (e.g., ./img2num format-wasm, ./img2num clean-wasm) instead of invoking local tools directly (e.g., clang-format -i). This reduces dependency requirements for users and ensures consistent tooling across environments. Apply this guidance to C++ source files under the project when reviewing changes.
Applied to files:
core/src/internal/image_to_svg.cppexample-apps/console-cpp/main.cppbindings/c/src/cimg2num.cppcore/src/internal/labels_to_svg.cppbindings/py/src/img2num_pybind.cppcore/src/internal/graph.cppcore/src/internal/shared_contours.cpp
📚 Learning: 2026-01-19T00:02:34.957Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 234
File: src/wasm/modules/image/src/kmeans.cpp:79-81
Timestamp: 2026-01-19T00:02:34.957Z
Learning: In C++ multithreading contexts, prefer unsigned types for thread-count-like parameters (e.g., n_threads) to prevent negative values. Validate that the value is at least 1 before any division to avoid divide-by-zero at runtime. At the start of functions handling such values, create a safe count like: const unsigned int thread_count{std::max(1u, n_threads)}; This ensures non-negative, non-zero usage for divisions and related arithmetic. Apply this pattern to similar parameters across C++ modules, not just the specific file.
Applied to files:
core/src/internal/image_to_svg.cppexample-apps/console-cpp/main.cppbindings/c/src/cimg2num.cppcore/src/internal/labels_to_svg.cppbindings/py/src/img2num_pybind.cppcore/src/internal/graph.cppcore/src/internal/shared_contours.cpp
📚 Learning: 2026-02-25T21:24:19.036Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 272
File: core/src/internal/bilateral_filter_gpu.cpp:171-185
Timestamp: 2026-02-25T21:24:19.036Z
Learning: Adopt brace initialization (e.g., Type var{}) over copy initialization (e.g., Type var = {}) for zero-initialization in C++ across the repository. This should apply to variables, arrays, and structs in most C++ files (e.g., core/src/internal/bilateral_filter_gpu.cpp). It improves safety by avoiding narrowing conversions and makes initialization intent explicit.
Applied to files:
core/src/internal/image_to_svg.cppexample-apps/console-cpp/main.cppbindings/c/src/cimg2num.cppcore/src/internal/labels_to_svg.cppbindings/py/src/img2num_pybind.cppcore/src/internal/graph.cppcore/src/internal/shared_contours.cpp
📚 Learning: 2026-02-25T21:24:34.055Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 272
File: core/include/internal/bilateral_filter_gpu.h:108-109
Timestamp: 2026-02-25T21:24:34.055Z
Learning: In WGSL shaders, .rgb swizzle on a vec4 returns the first three components regardless of color space. Do not assume data is RGB color space when the texture contains non-RGB data (e.g., CIELAB L, A, B). If LAB data is stored, treat and compute distances in LAB space, not RGB. Clearly document shader code paths that rely on specific color spaces and prefer explicit conversions or comments when using swizzled components with non-RGB textures. Apply this guidance to WGSL shader files across the codebase (not just this header) when handling color data or color-like channels.
Applied to files:
bindings/c/include/cimg2num.hcore/include/img2num.hcore/include/internal/graph.h
📚 Learning: 2026-04-27T15:40:33.329Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 333
File: bindings/c/src/cimg2num.cpp:0-0
Timestamp: 2026-04-27T15:40:33.329Z
Learning: In the Ryan-Millard/Img2Num C bindings (e.g., `bindings/c/src/cimg2num.cpp`) that expose `extern "C"` functions, do not allow C++ exceptions (including `std::bad_alloc` and exceptions thrown from lambdas used internally) to propagate out of these C binding functions. Use the C-API failure idiom instead: catch exceptions internally as needed, clear/record error state via the existing error mechanism, and return `nullptr` (or the documented sentinel return value) on failure so the C caller can detect errors by checking the return value.
Applied to files:
bindings/c/src/cimg2num.cpp
📚 Learning: 2026-04-09T19:05:40.514Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 302
File: core/include/internal/gpu.h:203-216
Timestamp: 2026-04-09T19:05:40.514Z
Learning: In Img2Num’s internal GPU initialization code, if querying adapter limits fails (e.g., `adapter.GetLimits(...)` returns `false`), treat this as a GPU initialization failure and route execution to the CPU fallback path. Do not continue with default limits in this case; the failure should cause the same fallback behavior as other GPU init errors.
Applied to files:
core/include/internal/graph.h
🪛 Cppcheck (2.20.0)
bindings/js/src/wasm_wrapper.c
[style] 40-40: The function 'image_to_svg' is never used.
(unusedFunction)
🔇 Additional comments (15)
core/src/internal/shared_contours.cpp (1)
15-15: The OUTSIDE sentinel issue has already been flagged in previous review comments.core/include/internal/graph.h (2)
42-47: The missing Doxygen documentation forgetPixelandanalyzeJunctionshas already been flagged in previous review comments.
78-78: LGTM!bindings/py/src/img2num_pybind.cpp (1)
169-170: LGTM!Also applies to: 181-181, 191-191
core/src/internal/image_to_svg.cpp (1)
20-20: LGTM!packages/js/safeWasmWrappers.js (2)
187-187: LGTM!Also applies to: 195-198
225-225: LGTM!Also applies to: 234-237
core/src/internal/graph.cpp (3)
281-298: Shared-loop mode drops hole semantics fromm_contours.This repopulates
Node::m_contourswith every returned ring, but hard-codeshierarchyto{-1, -1, -1, -1}andis_holetofalsefor all of them. That breaks the existing contour contract for regions with interior voids or nested islands.
260-279: Add regression coverage before making the shared-edge path the default.
compute_contours()now always takes the new shared-loop path, but this PR does not include tests for the reported gap artifact or for topology-sensitive cases.
472-522: LGTM!core/include/img2num.h (1)
75-76: LGTM!core/src/internal/labels_to_svg.cpp (1)
189-190: LGTM!Also applies to: 208-208
bindings/c/include/cimg2num.h (1)
79-80: LGTM!bindings/c/src/cimg2num.cpp (1)
10-24: LGTM!Also applies to: 26-40, 80-94
bindings/js/src/wasm_wrapper.c (1)
35-55: LGTM!
| int min_area, int min_thickness) { | ||
| const uint8_t *data_ptr{static_cast<const uint8_t *>(data.request().ptr)}; | ||
| const int32_t *labels_ptr{static_cast<const int32_t *>(labels.request().ptr)}; | ||
|
|
||
| std::string svg{img2num::labels_to_svg(data_ptr, labels_ptr, width, height, min_area)}; | ||
| std::string svg{img2num::labels_to_svg(data_ptr, labels_ptr, width, height, min_area, min_thickness)}; | ||
| pybind11::str svg_py_str(std::move(svg)); | ||
|
|
||
| return svg_py_str; | ||
| }, | ||
| pybind11::arg("data"), pybind11::arg("labels"), pybind11::arg("width"), | ||
| pybind11::arg("height"), pybind11::arg("min_area"), "Convert labels to SVG string"); | ||
| pybind11::arg("height"), pybind11::arg("min_area"), pybind11::arg("min_thickness"), "Convert labels to SVG string"); |
There was a problem hiding this comment.
Add a default value for min_thickness to prevent breaking existing code.
The min_thickness parameter is added as a required positional argument without a default value. This is a breaking change that will cause existing Python code calling labels_to_svg to fail. Provide a sensible default (e.g., 0 to disable or 10 to match the JS binding):
🔧 Proposed fix
pybind11::arg("data"), pybind11::arg("labels"), pybind11::arg("width"),
- pybind11::arg("height"), pybind11::arg("min_area"), pybind11::arg("min_thickness"), "Convert labels to SVG string");
+ pybind11::arg("height"), pybind11::arg("min_area"), pybind11::arg("min_thickness") = 0, "Convert labels to SVG string");🤖 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 `@bindings/py/src/img2num_pybind.cpp` around lines 116 - 126, The pybind
wrapper for labels_to_svg currently requires min_thickness as a positional
argument, breaking existing callers; update the pybind binding for the lambda
that calls img2num::labels_to_svg so the pybind11::arg("min_thickness") has a
default value (e.g., = 10 to match the JS binding or = 0 to disable), leaving
the lambda signature and call unchanged but changing the argument declaration to
include that default and updating the final docstring if needed.
There was a problem hiding this comment.
@Krasner, I thought we already had this done. I'm sorry for missing - this is very important.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Why would it fail? Internally, the C++ applies a default value. Look at core/include/img2num.h - it applies the zero there.
Are you saying that Python will throw an error?
| int j = 0; | ||
| for (const Node_ptr &n : get_nodes()) { | ||
| if (n->area() == 0) continue; | ||
| float eps = 0.25f; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Use brace initialization for consistency.
Per repository conventions, prefer brace initialization over copy initialization for local variables.
♻️ Suggested fix
- float eps = 0.25f;
+ float eps{0.25f};🤖 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 `@core/src/internal/graph.cpp` at line 270, The local variable eps uses copy
initialization; update its declaration in graph.cpp (variable eps) to use brace
initialization per repo convention by declaring eps with braces around its
initializer (retain the 0.25f value) instead of using the equals-style
initialization.
Source: Learnings
There was a problem hiding this comment.
@Krasner is this meant to represent
Additionally, why is this passed through multiple functions all the way down to fit_curve_reduction (as tolerance), then over to fitRecursive (as errorLimit)?
flowchart TD
A["Graph::compute_contours<br /><sub>float eps = 0.25f;</sub>"] --> B[build_shared_loops]
B --> C["fit_edge"]
C --> D["fit_curve_reduction"]
D --> E["fitRecursive"]
It's a hard-coded value that just gets delegated down the call stack. In React, this would be called prop-drilling and it's generally a sign of vulnerable code - I'm not saying that there's anything wrong with it, I'm just making a suggestion and want to see if you had a particular motivation for this.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
epsilon is meant to control the error of the bezier fit. it does get pushed thru the build_shared_loops funciton to fit_edge to fit_curve_reduction. do you propose a better method?
There was a problem hiding this comment.
I'm not sure. If it is meant to work that way, it's fine. That entire Graph function is a bit confusing, though because there's a fair bit of dead code:
It contains an if (true) as well as an else block - the compiler will just strip the if-check and the else block entirely.
| img2num_kmeans(img_data, out_data, out_labels, width, height, 16, 100, 1); | ||
| // Generate SVG | ||
| char* res_svg = img2num_labels_to_svg(img_data, out_labels, width, height, 100); | ||
| char* res_svg = img2num_labels_to_svg(img_data, out_labels, width, height, 100, 0); |
There was a problem hiding this comment.
Add an inline comment explaining the min_thickness parameter.
As per coding guidelines, example applications must be a good reflection of how to use Img2Num's library with good comments for external users. The hardcoded 0 for min_thickness lacks explanation. Add a brief comment to help users understand this parameter:
📝 Suggested comment
- char* res_svg = img2num_labels_to_svg(img_data, out_labels, width, height, 100, 0);
+ // min_area=100, min_thickness=0 (disabled)
+ char* res_svg = img2num_labels_to_svg(img_data, out_labels, width, height, 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.
| char* res_svg = img2num_labels_to_svg(img_data, out_labels, width, height, 100, 0); | |
| // min_area=100, min_thickness=0 (disabled) | |
| char* res_svg = img2num_labels_to_svg(img_data, out_labels, width, height, 100, 0); |
🤖 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 `@example-apps/console-c/main.c` at line 74, Add an inline comment next to the
call to img2num_labels_to_svg explaining the sixth parameter (min_thickness):
state that min_thickness is the minimum stroke/segment thickness in pixels to
include in the SVG (use 0 to disable thickness filtering), suggest typical
values (e.g. 1–3 for thin fonts) and note its effect (filters out tiny noisy
components). Update the comment near the img2num_labels_to_svg(...) invocation
to briefly describe this behavior and recommended use.
Source: Coding guidelines
|
@Ryan-Millard not sure what the wheel building failure in the python ci build is caused by. locally I'm able to do |
|
@Krasner, I'm not sure. I think it was because you set the PR to target |
correct. |
It'll still fail because I broke the image the other day.😂😂 I'm still trying to fix the permissions errors in #423 - it just takes so long to build the stuff. |
|
/docker-build b4ee348 |
🐳 Docker image built successfully!Image
Run it locally:
|
no worries. it's hard to find comments in the diff viewer, but to address one comment in the |
In case of what? There are also cleaner ways to go about keeping old code you don't have a use for - maybe |
Since it's the old version, we can use git to get it back (it would be what's called "restoring the hunk" where the hunk is the block of code that changed). It's very simple to do - you just need to be able to use |
I can think of a few cases:
Sure the one could scrub through various branches, but this is a core part of the algorithm that have not yet been fully stabilized, so having clearer visibility to different versions might be useful |
I think suggesting that they run git diffing on the function would be the best solution. Since it's been around for a while and hasn't changed, they wouldn't need to be very accurate between the commits. |
|
@Krasner please will you also update the |
Is there anything there to update? I'm going to have a big documentation PR ready soon like #388 (which can be closed for now) where I will update as much documentation as I can for all the latest changes and new stuff (python bindings) |
I was talking about the images. Do you think it would be a good idea to update them? |
Yes good point. I've updated them |
Ryan-Millard
left a comment
Ryan-Millard
left a comment
There was a problem hiding this comment.
@Krasner this all looks good and it can be merged as-is. Please just address the comments I left as well as run this:
pnpm format:cpp # there's a bug with the format script| char *img2num_labels_to_svg(const uint8_t *data, const int32_t *labels, const int width, | ||
| const int height, const int min_area, const int min_thickness) { | ||
| char *result{nullptr}; | ||
| img2num::clear_last_error_and_catch( | ||
| [&](const uint8_t* d, const int32_t* l, const int w, const int h, const int min_a) { | ||
| std::string svg {img2num::labels_to_svg(d, l, w, h, min_a)}; | ||
| result = static_cast<char*>(std::malloc(svg.size() + 1)); | ||
| [&](const uint8_t *d, const int32_t *l, const int w, const int h, const int min_a, const int min_t) { | ||
| std::string svg{img2num::labels_to_svg(d, l, w, h, min_a, min_t)}; | ||
| result = static_cast<char *>(std::malloc(svg.size() + 1)); | ||
| if (!result) { | ||
| return; // Allocation failed | ||
| } | ||
| std::memcpy(result, svg.c_str(), svg.size() + 1); | ||
| }, | ||
| data, labels, width, height, min_area | ||
| ); | ||
| data, labels, width, height, min_area, min_thickness); | ||
| return result; | ||
| } |
There was a problem hiding this comment.
What are your thoughts on us removing this function entirely in the future (not now) to make it easier to maintain the library: we only expose one interface for converting to SVGs.
There was a problem hiding this comment.
I'd still keep it... it's not difficult to maintain
| int min_area, int min_thickness) { | ||
| const uint8_t *data_ptr{static_cast<const uint8_t *>(data.request().ptr)}; | ||
| const int32_t *labels_ptr{static_cast<const int32_t *>(labels.request().ptr)}; | ||
|
|
||
| std::string svg{img2num::labels_to_svg(data_ptr, labels_ptr, width, height, min_area)}; | ||
| std::string svg{img2num::labels_to_svg(data_ptr, labels_ptr, width, height, min_area, min_thickness)}; | ||
| pybind11::str svg_py_str(std::move(svg)); | ||
|
|
||
| return svg_py_str; | ||
| }, | ||
| pybind11::arg("data"), pybind11::arg("labels"), pybind11::arg("width"), | ||
| pybind11::arg("height"), pybind11::arg("min_area"), "Convert labels to SVG string"); | ||
| pybind11::arg("height"), pybind11::arg("min_area"), pybind11::arg("min_thickness"), "Convert labels to SVG string"); |
There was a problem hiding this comment.
Why would it fail? Internally, the C++ applies a default value. Look at core/include/img2num.h - it applies the zero there.
Are you saying that Python will throw an error?
| /** | ||
| * `@brief` Safely retrieves a pixel value from a binary image with bounds checking. | ||
| * | ||
| * `@param` img Binary image buffer | ||
| * `@param` w Image width | ||
| * `@param` h Image height | ||
| * `@param` x Pixel x-coordinate | ||
| * `@param` y Pixel y-coordinate | ||
| * `@return` Pixel value at (x, y), or 0 if out of bounds | ||
| */ | ||
| inline uint8_t getPixel(const std::vector<uint8_t>& img, int w, int h, int x, int y) { | ||
| if (x < 0 || x >= w || y < 0 || y >= h) return 0; // Boundary check | ||
| return img[y * w + x]; | ||
| } |
There was a problem hiding this comment.
We should probably schedule a refactor to make the library use the image class wherever possible because we repeat this pattern a lot.
2 participants
What was changed & why
Current method produces holes when bezier simplification / smoothing is applied since shared contour edges (those between neighboring regions) are processed independently. Here we treat shared edges correctly so that contour simplification does not cause holes.
Work done using Claude Code - trying many ideas to improve upon current implementation.
Let me know if the results look cleaner and better - from my local tests they seem a bit more pleasing.
Fixes: #
Changes
Testing & Verification
Additional Resources
OLD OUTPUT SVG:
NEW OUPUT SVG:
note the jagged edges in the old svg and holes between edges that should be overlapping
Summary by CodeRabbit
New Features
min_thicknessparameter to SVG generation APIs (C, Python, JavaScript, C++) for filtering thin regionsChores