From e001178405a6bd59f3a196cf533b6eedf079d720 Mon Sep 17 00:00:00 2001 From: Walter Perdan Date: Wed, 15 Jul 2026 11:23:12 +0200 Subject: [PATCH] refactor(ar,ar2): group params in private helpers + document allows (#83) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit First increment of the too_many_arguments cleanup. Refactor the three private, non-SIMD helpers to take a param struct (destructured at the top so bodies are unchanged), removing their #[allow]: - ar/matrix.rs: sample_grid -> GridSample - ar2/feature_map.rs: make_template -> TemplatePatch, select_features -> FeatureSelection For the remaining ar/ar2 allows, add a `// rationale:` comment instead of refactoring: - public C-faithful API (ar_labeling, ar_detect_marker2, ar_get_marker_info, ar_patt_save/get_image/get_image2, ar2_tracking_2d_sub, ar2_get_best_matching[_sub_fine]) — struct-grouping would be a breaking change, deferred to a pre-1.0 API pass. - SIMD runtime-dispatch (get_similarity + _scalar) — signatures locked to the SIMD variants for is_x86_feature_detected! dispatch. kpm/freak allows are left for a follow-up increment. Full audit + incremental plan in docs/design/issue-83-too-many-args-audit.md. Verified: cargo test (matrix + feature_map green), strict clippy clean, and the simple (pattern) + simple_nft (kpm) examples run correctly (marker CF 0.89 / KPM match error 5.09). Refs #83 Co-Authored-By: Claude Opus 4.8 --- crates/core/src/ar/labeling.rs | 3 + crates/core/src/ar/marker.rs | 6 + crates/core/src/ar/matrix.rs | 66 +++++---- crates/core/src/ar/pattern.rs | 9 ++ crates/core/src/ar2/feature_map.rs | 143 +++++++++++++++----- crates/core/src/ar2/tracking.rs | 9 ++ docs/design/issue-83-too-many-args-audit.md | 86 ++++++++++++ 7 files changed, 261 insertions(+), 61 deletions(-) create mode 100644 docs/design/issue-83-too-many-args-audit.md diff --git a/crates/core/src/ar/labeling.rs b/crates/core/src/ar/labeling.rs index 44e8b86..2a365de 100644 --- a/crates/core/src/ar/labeling.rs +++ b/crates/core/src/ar/labeling.rs @@ -95,6 +95,9 @@ pub enum ImageProcMode { /// ImageProcMode::FrameImage, &mut label_info, false).unwrap(); /// arlog_i!("{} regions found", label_info.label_num); /// ``` +// rationale: public C-faithful API — mirrors the flat ARToolKit signature +// (arLabeling); struct-grouping is a breaking change deferred to a pre-1.0 +// API pass (#83, docs/design/issue-83-too-many-args-audit.md). #[allow(clippy::too_many_arguments)] pub fn ar_labeling( image: &[u8], diff --git a/crates/core/src/ar/marker.rs b/crates/core/src/ar/marker.rs index 9beb731..3a6a48d 100644 --- a/crates/core/src/ar/marker.rs +++ b/crates/core/src/ar/marker.rs @@ -611,6 +611,9 @@ pub(crate) fn history_resurrect(ar_handle: &mut crate::types::ARHandle) { /// - `label_info` — labeling output produced by [`crate::labeling::ar_labeling`]. /// - `marker_info2` — output slice (length ≥ `AR_SQUARE_MAX`) to write candidates into. /// - `marker2_num` — number of candidates written on return. +// rationale: public C-faithful API — mirrors the flat ARToolKit signature +// (arDetectMarker2); struct-grouping is a breaking change deferred to a +// pre-1.0 API pass (#83, docs/design/issue-83-too-many-args-audit.md). #[allow(clippy::too_many_arguments)] pub fn ar_detect_marker2( xsize: i32, @@ -1175,6 +1178,9 @@ pub fn ar_get_line( /// - `patt_handle_opt` — loaded pattern database (required for template modes; /// pass `None` for pure matrix-code mode). /// - `matrix_code_type` — dimension/ECC variant used by [`crate::matrix::ar_matrix_code_get_id`]. +// rationale: public C-faithful API — mirrors the flat ARToolKit signature +// (arGetMarkerInfo); struct-grouping is a breaking change deferred to a +// pre-1.0 API pass (#83, docs/design/issue-83-too-many-args-audit.md). #[allow(clippy::too_many_arguments)] pub fn ar_get_marker_info( image: &[u8], diff --git a/crates/core/src/ar/matrix.rs b/crates/core/src/ar/matrix.rs index de4e95b..202b769 100644 --- a/crates/core/src/ar/matrix.rs +++ b/crates/core/src/ar/matrix.rs @@ -122,13 +122,15 @@ pub fn ar_matrix_code_get_id( let mut bits = vec![0u8; (grid_size * grid_size) as usize]; sample_grid( - image, - xsize, - ysize, - vertex, - grid_size, - pixel_format, - patt_ratio, + &GridSample { + image, + xsize, + ysize, + vertex, + grid_size, + pixel_format, + patt_ratio, + }, &mut bits, ) .map_err(|_| MatchError::PatternExtraction)?; @@ -379,6 +381,19 @@ pub fn ar_get_barcode_marker( Ok(marker_info) } +/// Inputs to [`sample_grid`]: the source image plus the geometry of the +/// square to project onto a regular grid. Grouped into a struct to keep the +/// sampler within clippy's argument limit (#83). +struct GridSample<'a> { + image: &'a [u8], + xsize: i32, + ysize: i32, + vertex: &'a [[ARdouble; 2]; 4], + grid_size: i32, + pixel_format: crate::types::ARPixelFormat, + patt_ratio: f64, +} + /// Project image pixels onto a regular grid using a homography. /// /// Samples `grid_size × grid_size` evenly-spaced points inside the square @@ -395,17 +410,16 @@ pub fn ar_get_barcode_marker( /// equal to `dim + 2` where `dim = code_type & 0xFF`. /// - `patt_ratio` — fraction of the square covered by data cells (0.5–0.9). /// - `bits` — output: `grid_size * grid_size` raw intensity values. -#[allow(clippy::too_many_arguments)] -fn sample_grid( - image: &[u8], - xsize: i32, - ysize: i32, - vertex: &[[ARdouble; 2]; 4], - grid_size: i32, - pixel_format: crate::types::ARPixelFormat, - patt_ratio: f64, - bits: &mut [u8], -) -> Result<(), &'static str> { +fn sample_grid(g: &GridSample<'_>, bits: &mut [u8]) -> Result<(), &'static str> { + let GridSample { + image, + xsize, + ysize, + vertex, + grid_size, + pixel_format, + patt_ratio, + } = *g; let nc = match pixel_format { crate::types::ARPixelFormat::MONO => 1, crate::types::ARPixelFormat::RGB | crate::types::ARPixelFormat::BGR => 3, @@ -568,13 +582,15 @@ fn ar_matrix_code_get_id_global( let mut grid = vec![0u8; AR_GLOBAL_ID_OUTER_SIZE * AR_GLOBAL_ID_OUTER_SIZE]; let patt_ratio = AR_GLOBAL_ID_OUTER_SIZE as f64 / (AR_GLOBAL_ID_OUTER_SIZE as f64 + 2.0); sample_grid( - image, - xsize, - ysize, - vertex, - AR_GLOBAL_ID_OUTER_SIZE as i32, - pixel_format, - patt_ratio, + &GridSample { + image, + xsize, + ysize, + vertex, + grid_size: AR_GLOBAL_ID_OUTER_SIZE as i32, + pixel_format, + patt_ratio, + }, &mut grid, ) .map_err(|_| { diff --git a/crates/core/src/ar/pattern.rs b/crates/core/src/ar/pattern.rs index 700eca1..48d7928 100644 --- a/crates/core/src/ar/pattern.rs +++ b/crates/core/src/ar/pattern.rs @@ -199,6 +199,9 @@ pub fn ar_patt_load(patt_handle: &mut ARPattHandle, filename: &str) -> Result ExtractionParams { // Template helpers (ported from featureMap.c static functions) // --------------------------------------------------------------------------- -/// Create a zero-mean template patch centred at `(cx, cy)`. -/// -/// Returns `Some(vlen)` on success, `None` if the patch is out of bounds or -/// has insufficient variance. -#[allow(clippy::too_many_arguments)] -fn make_template( - image: &[u8], +/// The source image plus the centre and radii describing a template patch. +/// Grouped into a struct to keep [`make_template`] within clippy's argument +/// limit (#83). +struct TemplatePatch<'a> { + image: &'a [u8], xsize: i32, ysize: i32, cx: i32, @@ -119,8 +117,23 @@ fn make_template( ts1: i32, ts2: i32, sd_thresh: f32, - template: &mut [f32], -) -> Option { +} + +/// Create a zero-mean template patch centred at `(cx, cy)`. +/// +/// Returns `Some(vlen)` on success, `None` if the patch is out of bounds or +/// has insufficient variance. +fn make_template(p: &TemplatePatch<'_>, template: &mut [f32]) -> Option { + let TemplatePatch { + image, + xsize, + ysize, + cx, + cy, + ts1, + ts2, + sd_thresh, + } = *p; if cy - ts1 < 0 || cy + ts2 >= ysize || cx - ts1 < 0 || cx + ts2 >= xsize { return None; } @@ -175,6 +188,9 @@ fn make_template( /// `sx` (sum of u8 values) and `sxx` (sum of squared u8 values) are /// accumulated as `u64` integers in all paths — no FP drift on those. #[inline] +// rationale: runtime dispatcher whose signature is locked to match +// get_similarity_scalar and the SIMD variants (get_similarity_sse41/avx2) +// for is_x86_feature_detected! dispatch (#83). #[allow(clippy::too_many_arguments)] fn get_similarity( image: &[u8], @@ -211,6 +227,8 @@ fn get_similarity( /// `sx`/`sxx` use `u64` integer accumulation to avoid f32 rounding past /// 2^24. `sxy` is accumulated left-to-right in row-major order. #[inline] +// rationale: scalar fallback whose signature is locked to match the SIMD +// variants (get_similarity_sse41/avx2) for runtime dispatch (#83). #[allow(clippy::too_many_arguments)] fn get_similarity_scalar( image: &[u8], @@ -615,11 +633,22 @@ fn gen_feature_map_for_level( let ci = i as i32; let cj = j as i32; - let vlen = - match make_template(image, xsize, ysize, ci, cj, ts1, ts2, sd_thresh, &mut tmpl) { - Some(v) => v, - None => continue, - }; + let vlen = match make_template( + &TemplatePatch { + image, + xsize, + ysize, + cx: ci, + cy: cj, + ts1, + ts2, + sd_thresh, + }, + &mut tmpl, + ) { + Some(v) => v, + None => continue, + }; let mut max = -1.0f32; let mut early_exit = false; @@ -655,22 +684,39 @@ fn gen_feature_map_for_level( // Feature selection // --------------------------------------------------------------------------- -/// Greedily select features from a feature map. -/// -/// Ported from `ar2SelectFeature` in the C source. -#[allow(clippy::too_many_arguments, clippy::needless_range_loop)] -fn select_features( - image: &[u8], +/// Inputs to [`select_features`]: the source image, its feature map, and the +/// selection thresholds. Grouped into a struct to keep the selector within +/// clippy's argument limit (#83). +struct FeatureSelection<'a> { + image: &'a [u8], xsize: i32, ysize: i32, dpi: f32, - fmap: &[f32], + fmap: &'a [f32], max_feature_num: i32, max_sim_thresh: f32, min_sim_thresh: f32, sd_thresh: f32, occ_size: i32, -) -> Vec { +} + +/// Greedily select features from a feature map. +/// +/// Ported from `ar2SelectFeature` in the C source. +#[allow(clippy::needless_range_loop)] +fn select_features(s: &FeatureSelection<'_>) -> Vec { + let FeatureSelection { + image, + xsize, + ysize, + dpi, + fmap, + max_feature_num, + max_sim_thresh, + min_sim_thresh, + sd_thresh, + occ_size, + } = *s; let w = xsize as usize; let h = ysize as usize; let ts1 = AR2_DEFAULT_TS1; @@ -704,7 +750,19 @@ fn select_features( } // Validate: re-create template and check variance - let vlen = match make_template(image, xsize, ysize, cx, cy, ts1, ts2, 0.0, &mut tmpl) { + let vlen = match make_template( + &TemplatePatch { + image, + xsize, + ysize, + cx, + cy, + ts1, + ts2, + sd_thresh: 0.0, + }, + &mut tmpl, + ) { Some(v) => v, None => { work[(cy as usize) * w + (cx as usize)] = 1.0; @@ -838,18 +896,18 @@ pub fn ar2_gen_feature_map( ); // Greedily select features using level-dependent thresholds. - let coords = select_features( - &img.img_bw, - img.xsize, - img.ysize, - img.dpi, - &fmap, - search_feature_num, - params.max_sim_thresh, - params.min_sim_thresh, - params.sd_thresh, - params.occ_size, - ); + let coords = select_features(&FeatureSelection { + image: &img.img_bw, + xsize: img.xsize, + ysize: img.ysize, + dpi: img.dpi, + fmap: &fmap, + max_feature_num: search_feature_num, + max_sim_thresh: params.max_sim_thresh, + min_sim_thresh: params.min_sim_thresh, + sd_thresh: params.sd_thresh, + occ_size: params.occ_size, + }); // Compute mindpi: next lower DPI in the set, or current × 0.5. // Ports the scale1 loop in markerCreator.cpp. @@ -995,7 +1053,20 @@ mod tests { let data = vec![100u8; 10 * 10]; let mut tmpl = vec![0.0f32; 529]; // (11+11+1)^2 = 529 // Centre at (0,0) with ts1=11 → out of bounds - assert!(make_template(&data, 10, 10, 0, 0, 11, 11, 0.0, &mut tmpl).is_none()); + assert!(make_template( + &TemplatePatch { + image: &data, + xsize: 10, + ysize: 10, + cx: 0, + cy: 0, + ts1: 11, + ts2: 11, + sd_thresh: 0.0, + }, + &mut tmpl + ) + .is_none()); } /// Helper: build a deterministic 64×64 pseudo-random image and template diff --git a/crates/core/src/ar2/tracking.rs b/crates/core/src/ar2/tracking.rs index 8b3f7bb..4dd14e1 100644 --- a/crates/core/src/ar2/tracking.rs +++ b/crates/core/src/ar2/tracking.rs @@ -1008,6 +1008,9 @@ pub struct AR2Tracking2DResult { pub pos3d: [f32; 3], } +// rationale: public C-faithful API — mirrors the flat ARToolKit signature +// (ar2Tracking internals); struct-grouping is a breaking change deferred to +// a pre-1.0 API pass (#83, docs/design/issue-83-too-many-args-audit.md). #[allow(clippy::too_many_arguments)] pub fn ar2_tracking_2d_sub( cparam_lt: Option<&ARParamLT>, @@ -1426,6 +1429,9 @@ pub fn ar2_get_trans_mat( pub const KEEP_NUM: usize = 3; pub const SKIP_INTERVAL: i32 = 3; +// rationale: public C-faithful API — mirrors the flat ARToolKit signature +// (ar2GetBestMatching); struct-grouping is a breaking change deferred to a +// pre-1.0 API pass (#83, docs/design/issue-83-too-many-args-audit.md). #[allow(clippy::too_many_arguments)] pub fn ar2_get_best_matching( img: &[u8], @@ -1554,6 +1560,9 @@ pub fn ar2_get_best_matching( final_ret } +// rationale: public C-faithful API — mirrors the flat ARToolKit signature +// (ar2GetBestMatchingSubFine); struct-grouping is a breaking change deferred +// to a pre-1.0 API pass (#83, docs/design/issue-83-too-many-args-audit.md). #[allow(clippy::too_many_arguments)] pub fn ar2_get_best_matching_sub_fine( img: &[u8], diff --git a/docs/design/issue-83-too-many-args-audit.md b/docs/design/issue-83-too-many-args-audit.md new file mode 100644 index 0000000..63ad50e --- /dev/null +++ b/docs/design/issue-83-too-many-args-audit.md @@ -0,0 +1,86 @@ +# `clippy::too_many_arguments` audit (#83) + +Classification of every `#[allow(clippy::too_many_arguments)]` suppression +site in the codebase, and the planned action for each. Tracks the +incremental cleanup requested in +[#83](https://github.com/webarkit/WebARKitLib-rs/issues/83). + +**Context:** all sites are currently `#[allow]`-silenced (CI's strict +`--all-targets --all-features -D warnings` gate is green because of the +allows, not because the functions are refactored). #83 asks to *reduce* +them by grouping tightly-coupled arguments into param/context structs where +that is idiomatic and safe. + +**Decision (per maintainer, 2026-07-15):** refactor the private/internal +helpers into param structs now (zero API risk); on the public-API and +SIMD-locked sites, keep the allow and add a `// rationale:` comment. A full +public-API restructure is deferred to a deliberate pre-1.0 decision. + +--- + +## #83 original scope — `ar/` + `ar2/` + +### Refactor now — private helpers, zero API risk +| Function | Location | Args | Call sites | +|----------|----------|------|-----------| +| `sample_grid` | `ar/matrix.rs` | 8 | 2 (in-module) | +| `make_template` | `ar2/feature_map.rs` | 9 | 3 (in-module + test) | +| `select_features` | `ar2/feature_map.rs` | 10 | 1 (test) | + +### Keep allow + rationale — public C-faithful API (breaking to refactor) +| Function | Location | +|----------|----------| +| `ar_labeling` | `ar/labeling.rs` | +| `ar_detect_marker2` | `ar/marker.rs` | +| `ar_get_marker_info` | `ar/marker.rs` | +| `ar_patt_save` | `ar/pattern.rs` | +| `ar_patt_get_image` | `ar/pattern.rs` | +| `ar_patt_get_image2` | `ar/pattern.rs` | +| `ar2_tracking_2d_sub` | `ar2/tracking.rs` | +| `ar2_get_best_matching` | `ar2/tracking.rs` | +| `ar2_get_best_matching_sub_fine` | `ar2/tracking.rs` | + +### Keep allow + rationale — SIMD runtime-dispatch, signatures locked (CLAUDE.md §7.4) +| Function | Location | +|----------|----------| +| `get_similarity` | `ar2/feature_map.rs` | +| `get_similarity_scalar` | `ar2/feature_map.rs` | +| `get_similarity_sse41` | `ar2/feature_map.rs` | +| `get_similarity_avx2` | `ar2/feature_map.rs` | + +--- + +## Added since #83 — `kpm/freak/` (later increment) + +| Function | Location | Recommended | +|----------|----------|-------------| +| `homography_4_points_geometrically_consistent` | `kpm/freak/homography.rs` | private math — refactor candidate (defer*) | +| `condition_4_points_2d` | `kpm/freak/homography.rs` | private math — defer* | +| `denormalize_homography` | `kpm/freak/homography.rs` | private math — defer* | +| `solve_homography_4_points_inhomogeneous` | `kpm/freak/homography.rs` | private math — defer* | +| `solve_homography_4_points` | `kpm/freak/homography.rs` | private math — defer* | +| `compute_homography_normal_equations_post_multiply` | `kpm/freak/homography.rs` | private math — defer* | +| `preemptive_robust_homography` | `kpm/freak/homography.rs` | `pub` — rationale | +| `polish_homography` | `kpm/freak/homography.rs` | `pub` — rationale | +| `HoughSimilarityVoting::new` | `kpm/freak/hough.rs` | `pub` — rationale | +| `HoughSimilarityVoting::new_auto_xy` | `kpm/freak/hough.rs` | `pub` — rationale | +| `find_hough_similarity` | `kpm/freak/hough.rs` | `pub` — rationale | +| `webarkit_cpp_auto_adjust_xy_num_bins` | `kpm/freak/hough.rs` | FFI extern — leave (matches shim cfg) | +| `generate` | `kpm/ref_data_set.rs` | `pub` — rationale | +| module-wide `#![allow]` | `kpm/freak/detector.rs` | narrow to per-fn later | + +\* The homography math helpers are private (so technically refactorable), +but they're bit-parity-sensitive numerical C ports where flat +`(points, matrix, scalars)` signatures often read clearer than a struct. +Defer to a deliberate pass rather than obscure the math. + +--- + +## Incremental plan + +1. **This PR:** refactor `sample_grid`, `make_template`, `select_features`; + add rationale comments to the 13 public/SIMD `ar`/`ar2` allows. +2. **Follow-up:** `kpm/freak` public rationale comments + narrow the + `detector.rs` module-level allow. +3. **Pre-1.0 (maybe):** deliberate public-API restructure of the + C-faithful `ar`/`ar2` entry points into param structs (breaking).