FIX: Extrinsics dont set Identity rotation matrix by default#1744
FIX: Extrinsics dont set Identity rotation matrix by default#1744aljazkonec1 merged 7 commits intodevelopfrom
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughCMake device/version strings were updated. The Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pipeline/datatype/Extrinsics.cpp`:
- Around line 31-33: getRotationMatrix() currently returns rotationMatrix
silently when unset, unlike getInverseRotationMatrix() and
getTransformationMatrix() which validate and throw; update getRotationMatrix()
to perform the same validation on the rotationMatrix member and throw the same
exception type and message used by the sibling methods (e.g., std::runtime_error
with a descriptive message) when rotationMatrix is empty or not 3x3, so callers
receive a consistent error instead of an empty vector.
- Around line 127-130: The new runtime guard causes default-constructed
ImgTransformation instances to throw when calling
remap3DPointTo/remap3DPointFrom/getRotationMatrixTo/getTranslationVectorTo/project3DPointTo
because Extrinsics::toCameraSocket defaults to AUTO; update the public API/docs
to note this breaking change and add a migration path: introduce an
ImgTransformation constructor or factory that accepts an Extrinsics (or add
setExtrinsics/setToCameraSocket) and update those methods (remap3DPointTo,
remap3DPointFrom, getRotationMatrixTo, getTranslationVectorTo, project3DPointTo)
to include an error message pointing users to the new constructor/setter when
toCameraSocket == AUTO; ensure the thrown std::runtime_error text clearly
directs users to construct ImgTransformation with valid Extrinsics or call
setExtrinsics.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 73a14970-c2f7-4860-96aa-92ac1bc6976a
📒 Files selected for processing (9)
cmake/Depthai/DepthaiDeviceRVC4Config.cmakecmake/Depthai/DepthaiDeviceSideConfig.cmakeinclude/depthai/common/Extrinsics.hppsrc/pipeline/Node.cppsrc/pipeline/datatype/Extrinsics.cppsrc/pipeline/node/Rectification.cpptests/src/ondevice_tests/pipeline/node/spatial_location_calculator_test.cpptests/src/onhost_tests/image_transformations_test.cpptests/src/onhost_tests/normalization_test.cpp
📜 Review details
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2026-03-24T22:39:04.364Z
Learnt from: MaticTonin
Repo: luxonis/depthai-core PR: 1732
File: src/pipeline/Pipeline.cpp:705-705
Timestamp: 2026-03-24T22:39:04.364Z
Learning: Do not flag the `!= ""` part of the auto-calibration condition as redundant when it appears in `PipelineImpl::build()` (or closely related pipeline build logic). If the code uses `utility::getEnvAs<std::string>(..., default)` with a default such as `"ON_START"`, the explicit empty-string guard may still be intentional to treat an explicitly empty env var as “OFF/disabled” (or to avoid special-casing elsewhere). Only consider removing `!= ""` if the codebase has an explicit, enforceable guarantee that `DEPTHAI_AUTOCALIBRATION` can never be set to an empty string (e.g., via validated parsing/CI checks); otherwise, keep the guard.
Applied to files:
src/pipeline/Node.cppsrc/pipeline/node/Rectification.cppsrc/pipeline/datatype/Extrinsics.cpp
📚 Learning: 2026-03-24T22:39:00.650Z
Learnt from: MaticTonin
Repo: luxonis/depthai-core PR: 1732
File: src/pipeline/Pipeline.cpp:662-662
Timestamp: 2026-03-24T22:39:00.650Z
Learning: In `src/pipeline/Pipeline.cpp`, the `DEPTHAI_AUTOCALIBRATION` environment variable intentionally defaults to `"ON_START"` (not `""`). This means auto-calibration is initialized by default for stereo pipelines at pipeline build time. The resulting implicit EEPROM/calibration writes on startup are by design and should not be flagged as a backward-incompatible change or unintended side-effect in future reviews.
Applied to files:
tests/src/ondevice_tests/pipeline/node/spatial_location_calculator_test.cppcmake/Depthai/DepthaiDeviceSideConfig.cmakecmake/Depthai/DepthaiDeviceRVC4Config.cmake
📚 Learning: 2026-03-24T22:39:05.647Z
Learnt from: MaticTonin
Repo: luxonis/depthai-core PR: 1732
File: src/pipeline/Pipeline.cpp:705-705
Timestamp: 2026-03-24T22:39:05.647Z
Learning: In `src/pipeline/Pipeline.cpp`, within `PipelineImpl::build()`, the condition `autoCalibrationString != "OFF" && autoCalibrationString != ""` intentionally retains the empty-string check. Even though `utility::getEnvAs<std::string>("DEPTHAI_AUTOCALIBRATION", "ON_START")` provides a default of `"ON_START"`, the explicit `!= ""` guard is kept on purpose and should not be flagged as redundant in future reviews.
Applied to files:
tests/src/ondevice_tests/pipeline/node/spatial_location_calculator_test.cpp
📚 Learning: 2026-03-13T19:40:56.303Z
Learnt from: MaticTonin
Repo: luxonis/depthai-core PR: 1718
File: src/pipeline/node/AutoCalibration.cpp:327-330
Timestamp: 2026-03-13T19:40:56.303Z
Learning: In `src/pipeline/node/AutoCalibration.cpp`, the hard-coded 1280x800 resolution check in `AutoCalibration::validateIncomingData()` is intentional. The AutoCalibration node in the depthai-core (C++) project is designed to support only 1280x800 sensor resolution and this constraint should not be flagged as a usability issue in future reviews.
Applied to files:
tests/src/ondevice_tests/pipeline/node/spatial_location_calculator_test.cpp
📚 Learning: 2026-03-16T11:17:12.819Z
Learnt from: pheec
Repo: luxonis/depthai-core PR: 1715
File: include/depthai/pipeline/node/PointCloud.hpp:34-98
Timestamp: 2026-03-16T11:17:12.819Z
Learning: In `include/depthai/pipeline/node/PointCloud.hpp`, the nested `Impl` class inside `PointCloud` is intentionally public (not private/forward-declared) because the on-host unit tests in `tests/src/onhost_tests/point_cloud_test.cpp` directly access `Impl` methods such as `setIntrinsics`, `setExtrinsics`, `useCPU`, `useCPUMT`, `computePointCloudDense`, `applyTransformation`, and `filterValidPoints`. Do not suggest making `Impl` private or forward-declaring it.
Applied to files:
tests/src/ondevice_tests/pipeline/node/spatial_location_calculator_test.cpptests/src/onhost_tests/normalization_test.cpptests/src/onhost_tests/image_transformations_test.cpp
🪛 Cppcheck (2.20.0)
src/pipeline/Node.cpp
[information] 1-1: Include file
(missingIncludeSystem)
[information] 2-2: Include file
(missingIncludeSystem)
[information] 2-2: Include file
(missingIncludeSystem)
[information] 2-2: Include file
(missingIncludeSystem)
[information] 2-2: Include file
(missingIncludeSystem)
[information] 2-2: Include file
(missingIncludeSystem)
[information] 2-2: Include file
(missingIncludeSystem)
[information] 2-2: Include file
(missingIncludeSystem)
[information] 2-2: Include file
(missingIncludeSystem)
[information] 2-2: Include file
(missingIncludeSystem)
[information] 1-1: Include file
(missingIncludeSystem)
[information] 2-2: Include file
(missingIncludeSystem)
[information] 2-2: Include file
(missingIncludeSystem)
[information] 1-1: Include file
(missingIncludeSystem)
[information] 2-2: Include file
(missingIncludeSystem)
tests/src/onhost_tests/normalization_test.cpp
[information] 5-5: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
tests/src/onhost_tests/image_transformations_test.cpp
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
🔇 Additional comments (17)
src/pipeline/Node.cpp (1)
1-2: LGTM! Include reorganization improves code organization.Moving the spdlog include to the top and removing the duplicate is a good practice. The include is properly ordered with other system headers.
cmake/Depthai/DepthaiDeviceRVC4Config.cmake (1)
6-6: RVC4 version bump is clean and scoped.The update is consistent with a pure config version advance and introduces no logic regression in this file.
cmake/Depthai/DepthaiDeviceSideConfig.cmake (1)
5-5: Device-side commit has published firmware and checksum artifacts.The commit hash
5f516c7d34102ce86c8c7537987f88195c998a4chas corresponding firmware package and checksum artifacts reachable in Artifactory, so device-side package fetch will work correctly in CMake.tests/src/ondevice_tests/pipeline/node/spatial_location_calculator_test.cpp (1)
48-49: LGTM!The explicit identity extrinsics correctly satisfy the new validation requirements. The
computeSpatialDatafunction (inSpatialUtils.cpp:201) validatesdepthFrame->transformation.isValid()and callsgetIntrinsicMatrix(), which now requires properly set extrinsics.src/pipeline/node/Rectification.cpp (1)
110-110: LGTM!The conversion via
dai::matrix::vectorMatrixToMatrix3x3()correctly adapts to the updatedgetRotationMatrix()return type (std::vector<std::vector<float>>). The utility function validates 3x3 dimensions and throws on mismatch.tests/src/onhost_tests/image_transformations_test.cpp (2)
3-4: LGTM!The new includes are necessary for the explicit
ExtrinsicsandCameraModelusage in the updated test.
363-365: LGTM!The test now explicitly provides identity extrinsics (4×4 transformation matrix) and
CameraModel::Perspectiveto theImgTransformationconstructors. This aligns with the new requirement that extrinsics must be explicitly set forremapPointBetweenFrames()to work correctly.tests/src/onhost_tests/normalization_test.cpp (3)
5-5: LGTM!Include added for explicit
Extrinsicsusage in the test.
52-54: LGTM!The test now explicitly sets identity extrinsics on both
transformFromandtransformTobefore callingremapPointTo(), satisfying the new validation requirements.
66-71: LGTM!Same pattern as the point remap test - explicit identity extrinsics set before
remapRectTo()call.include/depthai/common/Extrinsics.hpp (4)
19-20: LGTM!The private helper centralizes rotation matrix validation logic, used by getter methods to throw meaningful errors when the matrix is unset.
33-33: Breaking API change:rotationMatrixtype changed from fixed-size array to dynamic vector.This is the core fix for the PR objective - the rotation matrix is no longer initialized with an identity matrix by default. Consumers must now explicitly set extrinsics before calling methods like
getRotationMatrix(),getTransformationMatrix(), etc.This change resolves the backwards compatibility issue when loading rotation matrices from EEPROM, as the empty state is now distinguishable from an identity matrix.
57-63: Return type change fromstd::array<std::array<float, 3>, 3>tostd::vector<std::vector<float>>.This is consistent with the internal storage change. Callers using these methods (like
Rectification.cpp) need to convert viadai::matrix::vectorMatrixToMatrix3x3()if they require the fixed-size array type.
78-78: Good documentation update.The
@noteclarifies the preconditions for obtaining the full transformation matrix, helping users understand why astd::runtime_errormight be thrown.src/pipeline/datatype/Extrinsics.cpp (3)
35-42: LGTM!Proper validation with a clear error message before computing the inverse.
44-51: LGTM!Validation ensures the transformation matrix can only be obtained when the rotation matrix is properly set. The error message is clear and actionable.
159-162: LGTM!Simple dimension check that serves as the foundation for validation in getter methods.
| std::vector<std::vector<float>> Extrinsics::getRotationMatrix() const { | ||
| return rotationMatrix; | ||
| } |
There was a problem hiding this comment.
Inconsistent validation: getRotationMatrix() doesn't throw on invalid state unlike sibling methods.
getInverseRotationMatrix() and getTransformationMatrix() validate and throw if the rotation matrix is unset, but getRotationMatrix() silently returns an empty vector. This inconsistency could lead to subtle bugs when callers assume the returned vector is always 3×3.
Consider adding validation here as well, or document that this method may return an empty vector.
🛡️ Proposed fix to add validation
std::vector<std::vector<float>> Extrinsics::getRotationMatrix() const {
+ if(!validRotationMatrix()) {
+ throw std::runtime_error(
+ "Rotation matrix has not been set. Please use setRotationMatrix() or setExtrinsics() to set the rotation matrix before calling getRotationMatrix().");
+ }
return rotationMatrix;
}📝 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.
| std::vector<std::vector<float>> Extrinsics::getRotationMatrix() const { | |
| return rotationMatrix; | |
| } | |
| std::vector<std::vector<float>> Extrinsics::getRotationMatrix() const { | |
| if(!validRotationMatrix()) { | |
| throw std::runtime_error( | |
| "Rotation matrix has not been set. Please use setRotationMatrix() or setExtrinsics() to set the rotation matrix before calling getRotationMatrix()."); | |
| } | |
| return rotationMatrix; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pipeline/datatype/Extrinsics.cpp` around lines 31 - 33,
getRotationMatrix() currently returns rotationMatrix silently when unset, unlike
getInverseRotationMatrix() and getTransformationMatrix() which validate and
throw; update getRotationMatrix() to perform the same validation on the
rotationMatrix member and throw the same exception type and message used by the
sibling methods (e.g., std::runtime_error with a descriptive message) when
rotationMatrix is empty or not 3x3, so callers receive a consistent error
instead of an empty vector.
| if(this->toCameraSocket == dai::CameraBoardSocket::AUTO || to.toCameraSocket == dai::CameraBoardSocket::AUTO) { | ||
| throw std::runtime_error( | ||
| "Cannot get extrinsics transformation to or from an extrinsics with AUTO camera socket. Please specify the camera socket for both extrinsics."); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find all usages of getExtrinsicsTransformationTo and getExtrinsicsTransformationMatrixTo
# to assess the impact of the new AUTO socket guard.
echo "=== Direct calls to getExtrinsicsTransformationTo ==="
rg -n --type=cpp 'getExtrinsicsTransformationTo\s*\(' -C3
echo ""
echo "=== Direct calls to getExtrinsicsTransformationMatrixTo ==="
rg -n --type=cpp 'getExtrinsicsTransformationMatrixTo\s*\(' -C3
echo ""
echo "=== Places where ImgTransformation is created without explicit extrinsics ==="
rg -n --type=cpp 'ImgTransformation\s*\(' -C2 | head -100Repository: luxonis/depthai-core
Length of output: 15323
🏁 Script executed:
# Get ImgTransformation class definition and constructor
cat -n include/depthai/common/ImgTransformations.hpp | sed -n '1,100p'Repository: luxonis/depthai-core
Length of output: 4813
🏁 Script executed:
# Find ImgTransformation constructors in the implementation
rg -n "ImgTransformation::" src/pipeline/datatype/ImgTransformations.cpp -A5 | head -150Repository: luxonis/depthai-core
Length of output: 7514
🏁 Script executed:
# Check how extrinsics is initialized in ImgTransformation
rg -n "extrinsics" include/depthai/common/ImgTransformations.hpp -B2 -A2Repository: luxonis/depthai-core
Length of output: 3151
🏁 Script executed:
# Look at the actual test case that creates ImgTransformation with dimensions
sed -n '45,55p' tests/src/onhost_tests/image_transformations_test.cppRepository: luxonis/depthai-core
Length of output: 552
🏁 Script executed:
# Search for calls to the extrinsics-dependent methods on ImgTransformation objects
echo "=== Calls to imageToCamera3D ==="
rg -n "imageToCamera3D" --type=cpp
echo ""
echo "=== Calls to pointToCamera3D ==="
rg -n "pointToCamera3D" --type=cpp
echo ""
echo "=== Calls to remap3DPointTo ==="
rg -n "remap3DPointTo" --type=cpp
echo ""
echo "=== Calls to remap3DPointFrom ==="
rg -n "remap3DPointFrom" --type=cpp
echo ""
echo "=== Calls to getRotationMatrixTo ==="
rg -n "getRotationMatrixTo" --type=cpp
echo ""
echo "=== Calls to getTranslationVectorTo ==="
rg -n "getTranslationVectorTo" --type=cppRepository: luxonis/depthai-core
Length of output: 2071
🏁 Script executed:
# Check if the test case would fail - see what happens in validateTransformations
rg -n "validateTransformations" include/depthai/common/ImgFrame.hpp -A10Repository: luxonis/depthai-core
Length of output: 139
🏁 Script executed:
# Check Extrinsics default constructor to confirm AUTO socket assumption
sed -n '1,100p' include/depthai/common/Extrinsics.hppRepository: luxonis/depthai-core
Length of output: 3701
🏁 Script executed:
# Check what method contains the remap3DPointTo call at line 469
sed -n '460,475p' src/pipeline/datatype/ImgTransformations.cppRepository: luxonis/depthai-core
Length of output: 791
🏁 Script executed:
# Search for any test that actually invokes the extrinsics transformation methods
rg -n "(remap3DPointTo|remap3DPointFrom|getRotationMatrixTo|getTranslationVectorTo|imageToCamera3D|pointToCamera3D)" tests/ --type=cppRepository: luxonis/depthai-core
Length of output: 46
🏁 Script executed:
# Check if deserialization sets extrinsics properly
sed -n '125,200p' src/utility/ProtoSerialize.cppRepository: luxonis/depthai-core
Length of output: 3921
🏁 Script executed:
# Also check if ImgFrame deserialization sets extrinsics
sed -n '950,970p' src/utility/ProtoSerialize.cppRepository: luxonis/depthai-core
Length of output: 833
🏁 Script executed:
# Check if there are any proto files that define ImgTransformation serialization
find . -name "*.proto" -type f | xargs grep -l "ImgTransformation" 2>/dev/null | head -5Repository: luxonis/depthai-core
Length of output: 208
🏁 Script executed:
# Look for any documentation or comments about when extrinsics should be set
rg -n "has_extrinsics\|extrinsics" include/depthai/common/ImgTransformations.hpp -B3 -A3Repository: luxonis/depthai-core
Length of output: 46
🏁 Script executed:
# Check the actual implementation of getExtrinsicsTransformationTo to understand the guard better
sed -n '124,135p' src/pipeline/datatype/Extrinsics.cppRepository: luxonis/depthai-core
Length of output: 897
🏁 Script executed:
# Check if there are any additional usages in C++ examples or demos
find examples/ -name "*.cpp" -type f 2>/dev/null | xargs grep -l "remap3DPointTo\|remap3DPointFrom\|getRotationMatrixTo\|project3DPointTo" 2>/dev/nullRepository: luxonis/depthai-core
Length of output: 48
🏁 Script executed:
# Check Python bindings to understand public API exposure
rg -n "remap3DPointTo|remap3DPointFrom|getRotationMatrixTo|getTranslationVectorTo" bindings/python/ --type=cppRepository: luxonis/depthai-core
Length of output: 487
🏁 Script executed:
# Verify the API documentation comment about the requirement
sed -n '343,348p' include/depthai/common/ImgTransformations.hppRepository: luxonis/depthai-core
Length of output: 547
🏁 Script executed:
# Check the ImgFrame proto to understand how extrinsics is serialized
sed -n '1,100p' protos/ImgFrame.proto | head -60Repository: luxonis/depthai-core
Length of output: 1985
🏁 Script executed:
# Look for how ImgFrame.transformation gets populated on-device
rg -n "setTransformation\|transformation.*=" src/pipeline/ --type=cpp | grep -v "\.setTransformation\|addTransformation" | head -20Repository: luxonis/depthai-core
Length of output: 46
🏁 Script executed:
# Check if there are any example scripts that use the extrinsics methods
find examples/ -name "*.py" -o -name "*.cpp" | xargs grep -l "remap3DPoint\|getRotationMatrix\|getTranslationVector" 2>/dev/nullRepository: luxonis/depthai-core
Length of output: 48
🏁 Script executed:
# Check if there's a changelog or PR description
git log --oneline --all | grep -i "extrinsic\|AUTO" | head -20Repository: luxonis/depthai-core
Length of output: 1200
🏁 Script executed:
# Look at what happens when extrinsics is default-constructed
rg -n "Extrinsics()" include/depthai/common/Extrinsics.hpp -B2 -A10Repository: luxonis/depthai-core
Length of output: 3200
🏁 Script executed:
# Verify the actual behavior - does getTransformationMatrix work with AUTO socket?
sed -n '70,120p' src/pipeline/datatype/Extrinsics.cppRepository: luxonis/depthai-core
Length of output: 1889
This guard enforces the documented API contract at runtime; breaking change for code violating it.
The new guard correctly enforces the documented requirement that "Both transformations must have a common toCameraSocket." Since ImgTransformation default-constructs with extrinsics.toCameraSocket = AUTO (line 26 of ImgTransformations.hpp), and all constructors except one (lines 65–80) don't accept an explicit Extrinsics parameter, any code calling remap3DPointTo(), remap3DPointFrom(), getRotationMatrixTo(), getTranslationVectorTo(), or project3DPointTo() on a default-constructed ImgTransformation will now throw std::runtime_error.
This is a breaking change, though it enforces behavior that was already documented as required. The concern is valid: users relying on manual construction of ImgTransformation objects (rather than deserialization from device data) may encounter runtime exceptions. Deserialization paths (e.g., ProtoSerialize.cpp:125–200) properly populate extrinsics from proto messages, so the typical device→host data flow is unaffected. Consider documenting this as a breaking API change or providing a migration path for users constructing these objects manually.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pipeline/datatype/Extrinsics.cpp` around lines 127 - 130, The new runtime
guard causes default-constructed ImgTransformation instances to throw when
calling
remap3DPointTo/remap3DPointFrom/getRotationMatrixTo/getTranslationVectorTo/project3DPointTo
because Extrinsics::toCameraSocket defaults to AUTO; update the public API/docs
to note this breaking change and add a migration path: introduce an
ImgTransformation constructor or factory that accepts an Extrinsics (or add
setExtrinsics/setToCameraSocket) and update those methods (remap3DPointTo,
remap3DPointFrom, getRotationMatrixTo, getTranslationVectorTo, project3DPointTo)
to include an error message pointing users to the new constructor/setter when
toCameraSocket == AUTO; ensure the thrown std::runtime_error text clearly
directs users to construct ImgTransformation with valid Extrinsics or call
setExtrinsics.
2 participants
Purpose
As this was causing backwards compatibility issues when loading rotation matrices from eeprom. Additionally fix failing tests.
Specification
None / not applicable
Dependencies & Potential Impact
None / not applicable
Deployment Plan
None / not applicable
Testing & Validation
None / not applicable
Summary by CodeRabbit
Refactor
Bug Fixes
Tests