Fix SolvePnPMethod enum values to match native OpenCV5 numbering - #2081
Merged
Merged
Conversation
The geometry module removed the broken DLS/UPnP methods (both just fell back to EPnP) and renumbered the remaining SOLVEPNP_* constants, but the managed SolvePnPMethod enum still carried the old OpenCV4 values. As a result AP3P/IPPE silently ran the wrong native algorithm, and IPPE_SQUARE/SQPNP were rejected outright since they fell outside the native flag range. Also drops the obsolete SolvePnPFlags alias enum, which is no longer needed now that OpenCvSharp5 doesn't guarantee source compatibility with the 4.x API. Fixes #2080
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesSolvePnP method alignment
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🧹 Nitpick comments (2)
src/OpenCvSharp/Modules/geometry/Enum/SolvePnPMethod.cs (1)
53-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse PascalCase for the compound enum member.
IPPE_SQUAREis not a standalone acronym. Rename it to the repository’s PascalCase form, such asIppeSquare, and update callers intest/OpenCvSharp.Tests/calib3d/Calib3dTest.cs.Suggested rename
- IPPE_SQUARE = 5, + IppeSquare = 5,As per coding guidelines, C# enum members from C++
ALL_CAPS_WITH_UNDERSCORESshould use PascalCase for compounds, except standalone acronyms.🤖 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/OpenCvSharp/Modules/geometry/Enum/SolvePnPMethod.cs` at line 53, Rename the SolvePnPMethod enum member IPPE_SQUARE to IppeSquare, then update all references in Calib3dTest.cs and any other callers to use the new PascalCase identifier.Source: Coding guidelines
test/OpenCvSharp.Tests/calib3d/Calib3dTest.cs (1)
484-512: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the recovered pose, not only that the call does not throw.
Because
Cv2.SolvePnPforwards(int)flagsdirectly, these tests can pass while dispatching to another valid solver. Reproject with the returnedrvec/tvecand assert low error; also add explicit integer assertions for the renumbered members.Suggested validation
Cv2.SolvePnP(objPts, imgPts, cameraMatrix, dist, ref rvec, ref tvec, flags: method); + Cv2.ProjectPoints(objPts, rvec, tvec, cameraMatrix, dist, out var reproj, out _); + Assert.Equal(imgPts.Length, reproj.Length); + for (var i = 0; i < imgPts.Length; i++) + { + Assert.InRange(MathF.Abs(reproj[i].X - imgPts[i].X), 0f, 1e-3f); + Assert.InRange(MathF.Abs(reproj[i].Y - imgPts[i].Y), 0f, 1e-3f); + }Also applies to: 514-538
🤖 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 `@test/OpenCvSharp.Tests/calib3d/Calib3dTest.cs` around lines 484 - 512, Strengthen SolvePnPTestByArrayMethods and the corresponding test around the later overload to validate the returned pose: reproject objPts using the updated rvec and tvec, then assert the reprojection error is below a small tolerance against imgPts. Add explicit assertions for the integer values of each renumbered SolvePnPMethod member so flags dispatch to the intended solver.
🤖 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 `@test/OpenCvSharp.Tests/calib3d/Calib3dTest.cs`:
- Around line 491-492: Update the synthetic camera poses in the projection tests
around the rvec/tvec setup to use a positive Z translation, changing each
affected tvec from [0, 0, -10] to [0, 0, 10] while preserving the other pose
values.
---
Nitpick comments:
In `@src/OpenCvSharp/Modules/geometry/Enum/SolvePnPMethod.cs`:
- Line 53: Rename the SolvePnPMethod enum member IPPE_SQUARE to IppeSquare, then
update all references in Calib3dTest.cs and any other callers to use the new
PascalCase identifier.
In `@test/OpenCvSharp.Tests/calib3d/Calib3dTest.cs`:
- Around line 484-512: Strengthen SolvePnPTestByArrayMethods and the
corresponding test around the later overload to validate the returned pose:
reproject objPts using the updated rvec and tvec, then assert the reprojection
error is below a small tolerance against imgPts. Add explicit assertions for the
integer values of each renumbered SolvePnPMethod member so flags dispatch to the
intended solver.
🪄 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 Plus
Run ID: 97499327-c3c1-4b4a-b1a8-d12f6bd92eca
📒 Files selected for processing (2)
src/OpenCvSharp/Modules/geometry/Enum/SolvePnPMethod.cstest/OpenCvSharp.Tests/calib3d/Calib3dTest.cs
3 tasks done
- Rename SolvePnPMethod.IPPE_SQUARE to IppeSquare: once an acronym is glued to another word to form a compound member name, this repo's enum-naming convention PascalCases the whole compound rather than leaving part of it in caps (see .github/copilot-instructions.md). - Use a positive-Z tvec in the new synthetic poses so the projected points sit in front of the camera instead of behind it. - Strengthen SolvePnPTestByArrayMethods/IppeSquare to reproject with the recovered pose and assert it matches the input points, instead of only asserting the call doesn't throw. Confirmed this actually matters: with the old AP3P/IPPE values, SolvePnP silently dispatches to a different native solver that still completes without throwing, so a throws-only assertion doesn't catch it - the reprojection check does.
CodeRabbit's suggested PascalCase compound (IppeSquare) follows the letter of the naming convention doc, but AKAZEDescriptorType already has an established precedent for this exact shape (acronym + plain word) that keeps the acronym in caps: KAZEUpright/MLDBUpright, not KazeUpright/MldbUpright. Match that existing style instead so IPPE (the bare enum member) and the compound built from it stay visually related.
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.
Description
Cv2.SolvePnP/Cv2.SolvePnPRansacpassSolvePnPMethodstraight through as a nativeint flagsvalue, but the enum's numeric values were never updated when thegeometrymodule droppedSOLVEPNP_DLSandSOLVEPNP_UPNP(both were broken implementations that only fell back to EPnP) and renumbered the remainingSOLVEPNP_*constants:This meant
AP3P/IPPEsilently invoked the wrong native algorithm, andIPPE_SQUARE/SQPNPfell outside the native flag range and threwStsBadArg.Changes
SolvePnPMethod(AP3P=3, IPPE=4, IPPE_SQUARE=5, SQPNP=6) to match the native enum, and drop the now-nonexistentDLS/UPNPmembers.SolvePnPFlagsalias enum (kept around from the OpenCvSharp4 days) since OpenCvSharp5 doesn't guarantee source compatibility with the old API. Renamed the containing file toSolvePnPMethod.csto match its sole remaining type.Calib3dTestcoveringP3P/AP3P/IPPE/SQPNP/IPPE_SQUARE.Fixes #2080
Test plan
dotnet buildsucceedsdotnet test --filter FullyQualifiedName~calib3d— 90 passed, 0 failedSummary by CodeRabbit
Breaking Changes
SolvePnPMethodoptionsDLSandUPNP.SolvePnPMethodmembers.IPPE_SQUAREtoIPPESquare.Tests
SolvePnPtest cases that validate accuracy across multipleSolvePnPMethodvalues.SolvePnPMethod.IPPESquare.