Normalize cvTry lambda body indentation across OpenCvSharpExtern - #2003
Merged
Conversation
…tern
Indent the body of every multi-line cvTry([&] { ... }); block one
level deeper than the enclosing return statement, across all of
OpenCvSharpExtern. About half of the ~2900 multi-line blocks already
followed this convention; the rest had accumulated at the same
indentation as the return statement over time. Preprocessor directives
(#ifdef/#else/#endif) inside a block are left at column 0, matching
this codebase's existing convention for them. Pure whitespace change,
no behavior difference.
Follow-up to #2000's ad hoc fix of this in geometry.h; see #2002.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe pull request reformats wrapper implementations across many ChangesCore and calibration wrappers
DNN, features, flann, img_hash, geometry
HighGUI and imgproc family
ML, objdetect, optflow, and photo wrappers
Ptcloud, saliency, shape, stereo, and stitching wrappers
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/OpenCvSharpExtern/geometry.h (1)
493-507: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCall
cv::convertPointsToHomogeneousin these branches.
The 2→3 and 3→4 array overloads still callcv::convertPointsFromHomogeneous, so they produce the wrong transform and corrupt the output.🤖 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 `@src/OpenCvSharpExtern/geometry.h` around lines 493 - 507, The array overloads in geometry_convertPointsToHomogeneous_array2 are calling the wrong OpenCV conversion routine, so update the 2→3 and 3→4 branches to use cv::convertPointsToHomogeneous instead of cv::convertPointsFromHomogeneous. Keep the existing srcMat/dstMat setup in geometry.h, and make sure the function name geometry_convertPointsToHomogeneous_array2 matches the conversion being invoked in both branches.
🤖 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 `@src/OpenCvSharpExtern/calib.h`:
- Around line 22-30: The copy loop for imagePointsVec is using the wrong bound,
so it may iterate past imagePoints when the lengths differ. Update the loop in
initCameraMatrix2D handling to use imagePointsLength for populating
imagePointsVec, keeping the objectPointsVec loop unchanged and matching each
vector to its own input count.
In `@src/OpenCvSharpExtern/face_FaceRecognizer.h`:
- Around line 351-361: The histogram population in
face_LBPHFaceRecognizer_getHistograms clears dst and only calls reserve(), then
indexes dst->at(i) on an empty vector. Fix this by resizing or assigning dst to
match result.size() before the loop, then copy the Mats into the pre-sized
container; use the existing face_LBPHFaceRecognizer_getHistograms symbol to
locate the change.
In `@src/OpenCvSharpExtern/features_Feature2D.h`:
- Around line 32-45: The vector construction in detect_Mat2 and compute2 is
wrong because imageVec is initialized with a fixed size and then filled with
push_back, which leaves default Mat entries at the front. Change the code to
build imageVec with reserve(imageLength) and append only the actual input mats,
matching the existing maskVec pattern, so detector->detect and the corresponding
compute call receive correctly populated vectors from features_Feature2D.h.
In `@src/OpenCvSharpExtern/geometry.h`:
- Around line 706-715: The correctMatches wrapper is reshaping the wrong
matrices for the output buffers, so the destination values are written into
points1M and points2M instead of newPoints1M and newPoints2M. Update the
reshaping in geometry.h so the newPoints1MM and newPoints2MM variables are
created from newPoints1M and newPoints2M, then keep cv::correctMatches using
those corrected destination matrices.
In `@src/OpenCvSharpExtern/photo_HDR.h`:
- Around line 49-59: The CalibrateDebevec samples bindings are wired to the
wrong property and type, since photo_CalibrateDebevec_getSamples and
photo_CalibrateDebevec_setSamples still delegate to getLambda/setLambda with a
float. Update these externs in photo_HDR.h to call the CalibrateDebevec samples
accessors instead, and change the setter/getter parameter and return types to
int so the native binding matches getSamples/setSamples(int).
In `@src/OpenCvSharpExtern/stitching_detail_Matchers.h`:
- Around line 127-137: The Matchers conversion is creating extra empty entries
because `featuresVec` is pre-sized and then appended to in the loop. Update the
logic in the `(*obj)(...)` path that builds `featuresVec` so it starts empty,
reserves capacity if needed, and only adds each `cv::detail::ImageFeatures` once
via `push_back` in the loop.
---
Outside diff comments:
In `@src/OpenCvSharpExtern/geometry.h`:
- Around line 493-507: The array overloads in
geometry_convertPointsToHomogeneous_array2 are calling the wrong OpenCV
conversion routine, so update the 2→3 and 3→4 branches to use
cv::convertPointsToHomogeneous instead of cv::convertPointsFromHomogeneous. Keep
the existing srcMat/dstMat setup in geometry.h, and make sure the function name
geometry_convertPointsToHomogeneous_array2 matches the conversion being invoked
in both branches.
🪄 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: CHILL
Plan: Pro
Run ID: 00f64de4-3440-4fa2-bdf9-d0ac70088e5e
📒 Files selected for processing (108)
src/OpenCvSharpExtern/barcode.hsrc/OpenCvSharpExtern/bgsegm.hsrc/OpenCvSharpExtern/calib.hsrc/OpenCvSharpExtern/calib_fisheye.hsrc/OpenCvSharpExtern/calib_multiview.hsrc/OpenCvSharpExtern/core.hsrc/OpenCvSharpExtern/core_Algorithm.hsrc/OpenCvSharpExtern/core_FileNode.hsrc/OpenCvSharpExtern/core_FileStorage.hsrc/OpenCvSharpExtern/core_InputArray.hsrc/OpenCvSharpExtern/core_LDA.hsrc/OpenCvSharpExtern/core_Mat.hsrc/OpenCvSharpExtern/core_MatExpr.hsrc/OpenCvSharpExtern/core_OutputArray.hsrc/OpenCvSharpExtern/core_PCA.hsrc/OpenCvSharpExtern/core_SVD.hsrc/OpenCvSharpExtern/core_SparseMat.hsrc/OpenCvSharpExtern/core_UMat.hsrc/OpenCvSharpExtern/dnn.hsrc/OpenCvSharpExtern/dnn_Model.hsrc/OpenCvSharpExtern/dnn_Net.hsrc/OpenCvSharpExtern/dnn_TextModel.hsrc/OpenCvSharpExtern/dnn_Tokenizer.hsrc/OpenCvSharpExtern/dnn_superres.hsrc/OpenCvSharpExtern/face_FaceRecognizer.hsrc/OpenCvSharpExtern/face_Facemark.hsrc/OpenCvSharpExtern/features.hsrc/OpenCvSharpExtern/features_ANNIndex.hsrc/OpenCvSharpExtern/features_DescriptorMatcher.hsrc/OpenCvSharpExtern/features_Feature2D.hsrc/OpenCvSharpExtern/flann.hsrc/OpenCvSharpExtern/flann_IndexParams.hsrc/OpenCvSharpExtern/geometry.hsrc/OpenCvSharpExtern/highgui.hsrc/OpenCvSharpExtern/img_hash.hsrc/OpenCvSharpExtern/imgcodecs.hsrc/OpenCvSharpExtern/imgproc.hsrc/OpenCvSharpExtern/imgproc_CLAHE.hsrc/OpenCvSharpExtern/imgproc_FontFace.hsrc/OpenCvSharpExtern/imgproc_GeneralizedHough.hsrc/OpenCvSharpExtern/imgproc_LineIterator.hsrc/OpenCvSharpExtern/imgproc_LineSegmentDetector.hsrc/OpenCvSharpExtern/imgproc_Segmentation.hsrc/OpenCvSharpExtern/imgproc_Subdiv2D.hsrc/OpenCvSharpExtern/imgproc_undistort.hsrc/OpenCvSharpExtern/line_descriptor.hsrc/OpenCvSharpExtern/ml_ANN_MLP.hsrc/OpenCvSharpExtern/ml_Boost.hsrc/OpenCvSharpExtern/ml_DTrees.hsrc/OpenCvSharpExtern/ml_EM.hsrc/OpenCvSharpExtern/ml_KNearest.hsrc/OpenCvSharpExtern/ml_LogisticRegression.hsrc/OpenCvSharpExtern/ml_NormalBayesClassifier.hsrc/OpenCvSharpExtern/ml_RTrees.hsrc/OpenCvSharpExtern/ml_SVM.hsrc/OpenCvSharpExtern/ml_StatModel.hsrc/OpenCvSharpExtern/objdetect.hsrc/OpenCvSharpExtern/objdetect_FaceDetectorYN.hsrc/OpenCvSharpExtern/objdetect_HOGDescriptor.hsrc/OpenCvSharpExtern/objdetect_QRCodeDetector.hsrc/OpenCvSharpExtern/objdetect_chessboard.hsrc/OpenCvSharpExtern/optflow.hsrc/OpenCvSharpExtern/optflow_motempl.hsrc/OpenCvSharpExtern/photo.hsrc/OpenCvSharpExtern/photo_HDR.hsrc/OpenCvSharpExtern/photo_Tonemap.hsrc/OpenCvSharpExtern/ptcloud_Odometry.hsrc/OpenCvSharpExtern/ptcloud_OdometryFrame.hsrc/OpenCvSharpExtern/ptcloud_OdometrySettings.hsrc/OpenCvSharpExtern/ptcloud_RgbdNormals.hsrc/OpenCvSharpExtern/ptcloud_Volume.hsrc/OpenCvSharpExtern/ptcloud_VolumeSettings.hsrc/OpenCvSharpExtern/ptcloud_depth.hsrc/OpenCvSharpExtern/ptcloud_io.hsrc/OpenCvSharpExtern/quality.hsrc/OpenCvSharpExtern/saliency_MotionSaliencyBinWangApr2014.hsrc/OpenCvSharpExtern/saliency_ObjectnessBING.hsrc/OpenCvSharpExtern/saliency_StaticSaliencyFineGrained.hsrc/OpenCvSharpExtern/saliency_StaticSaliencySpectralResidual.hsrc/OpenCvSharpExtern/shape_HistogramCostExtractor.hsrc/OpenCvSharpExtern/shape_ShapeDistanceExtractor.hsrc/OpenCvSharpExtern/shape_ShapeTransformer.hsrc/OpenCvSharpExtern/stereo.hsrc/OpenCvSharpExtern/stereo_StereoMatcher.hsrc/OpenCvSharpExtern/stitching.hsrc/OpenCvSharpExtern/stitching_detail_Matchers.hsrc/OpenCvSharpExtern/superres.hsrc/OpenCvSharpExtern/text.hsrc/OpenCvSharpExtern/text_TextDetector.hsrc/OpenCvSharpExtern/tracking.hsrc/OpenCvSharpExtern/tracking_UnscentedKalmanFilter.hsrc/OpenCvSharpExtern/video_background_segm.hsrc/OpenCvSharpExtern/video_tracking.hsrc/OpenCvSharpExtern/videoio.hsrc/OpenCvSharpExtern/wechat_qrcode.hsrc/OpenCvSharpExtern/xfeatures2d.hsrc/OpenCvSharpExtern/xfeatures2d_BOW.hsrc/OpenCvSharpExtern/xfeatures2d_FeatureDetectors.hsrc/OpenCvSharpExtern/ximgproc.hsrc/OpenCvSharpExtern/ximgproc_EdgeBoxes.hsrc/OpenCvSharpExtern/ximgproc_EdgeDrawing.hsrc/OpenCvSharpExtern/ximgproc_EdgeFilter.hsrc/OpenCvSharpExtern/ximgproc_FastLineDetector.hsrc/OpenCvSharpExtern/ximgproc_RidgeDetectionFilter.hsrc/OpenCvSharpExtern/ximgproc_Segmentation.hsrc/OpenCvSharpExtern/ximgproc_StructuredEdgeDetection.hsrc/OpenCvSharpExtern/ximgproc_SuperPixel.hsrc/OpenCvSharpExtern/xphoto.h
CodeRabbit review comment on PR #2003: the loop populating imagePointsVec reused objectPointsLength instead of imagePointsLength, causing an out-of-bounds read of imagePoints (or leaving trailing default-constructed Mats) whenever the two lengths differ. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CodeRabbit review comment on PR #2003: reserve() only changes vector capacity, not size, so dst stayed empty and dst->at(i) threw std::out_of_range on the first iteration whenever getHistograms() returned any element. Assign the result directly instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…compute CodeRabbit review comment on PR #2003: imageVec (and, in compute2, descriptorsVec) were constructed with a fixed size and then filled via push_back, leaving imageLength/descriptorsLength default-constructed Mat entries in front of the real ones. detector->detect/compute then ran over twice as many (half garbage) images. Build both vectors with reserve() instead, matching the existing maskVec pattern. Also fixes a second bug in compute2 uncovered while adding a regression test: detector->compute() writes into the local descriptorsVec copy, so its results never reached the caller-owned Mat objects. Copy the computed descriptors back into *descriptors[i] after the call. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CodeRabbit review comment on PR #2003: newPoints1MM/newPoints2MM were reshaped from the input matrices (points1M/points2M) instead of the output matrices (newPoints1M/newPoints2M), so cv::correctMatches wrote its results back into the caller's input points instead of the dedicated output arrays. While fixing this, found that the underlying Mat_<Point2d>::reshape(2) calls were also broken independent of that mixup: Mat_<_Tp>::reshape only takes a row count, not a channel count, so "2" was being interpreted as the new row count rather than requesting a 2-channel view. Replaced the whole Mat_/reshape dance with direct cv::Mat(rows, 1, CV_64FC2, ptr) construction, matching the pattern already used by sibling wrappers like geometry_findHomography_vector. Added a regression test that exercises this array-based overload end to end (the existing CorrectMatches test only covered the Mat-based overload, which uses a separate, unaffected code path). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CodeRabbit review comment on PR #2003: photo_CalibrateDebevec_getSamples and _setSamples called getLambda/setLambda instead of getSamples/setSamples, so the Samples property actually read/wrote Lambda. Wire the native accessors to the correct CalibrateDebevec methods, and change the float signature to int (native getSamples()/setSamples(int) return/take an int) on the extern declaration and the public C# property. This is a breaking change to CalibrateDebevec.Samples's type, but the previous float type never worked correctly anyway. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CodeRabbit review comment on PR #2003: featuresVec was constructed with a fixed size and then filled via push_back, leaving featuresSize default-constructed ImageFeatures entries in front of the real ones. (*obj)(featuresVec, ...) then matched over twice as many (half empty) images, producing a pairwise-matches matrix with bogus image indices. Build the vector with reserve() instead, matching the already-correct sibling wrappers in stitching_detail_Matchers.h. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
cvTry([&] { ... });block one level deeper than the enclosingreturnstatement, across all ofsrc/OpenCvSharpExtern. Roughly half of the ~2900 multi-line blocks already followed this convention (see the fix applied to the newly-addedgeometry.hcode in Add missing OpenCV 5 geometry APIs; fix stale readNet doc #2000); the rest had accumulated at the same indentation asreturn cvTry(over time, presumably added by different contributors/scripts without a consistent formatter for this specific pattern.#ifdef/#else/#endiflines inside a block are left at column 0, matching this codebase's existing convention of always putting preprocessor directives at column 0 regardless of the surrounding C++ indentation.Test plan
cmake --build src/build --config Release --target OpenCvSharpExternsucceeds with no errors (only pre-existing, unrelated code-page warnings).dotnet build test/OpenCvSharp.Tests/OpenCvSharp.Tests.csproj -c Releasesucceeds, 0 warnings/errors.dotnet test test/OpenCvSharp.Tests/OpenCvSharp.Tests.csproj -c Release --no-build- 1259 passed, 0 failed, 27 skipped (pre-existing, environment-dependent).🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Bug Fixes