Feat: Add abstract TransformableBuffer class#1778
Open
aljazkonec1 wants to merge 43 commits into
Open
Conversation
…ialImgDetections.
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:
📝 WalkthroughWalkthroughThis PR introduces a comprehensive transformation framework enabling detection, April tag, tracklet, and point cloud datatypes to remap coordinates between different image coordinate systems. A new ChangesTransformable abstraction and datatype infrastructure
Geometric transformation and projection
Detection datatype transformation support
Protobuf serialization and public API updates
Node producers and end-to-end validation
Python bindings for detection datatypes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/ImgDetections.cpp`:
- Around line 167-169: The current call to keypoints->transformTo(source,
target) inside ImgDetection::transform() can mis-normalize coordinates because
transformTo infers normalization; change this to only call transformTo when the
Keypoints object has explicit normalization metadata (e.g., a method/field like
hasNormalization(), isNormalized(), or metadata().normalization). If no explicit
normalization exists, skip the remap or convert using explicit info from
ImgDetection (e.g., its source/target normalization flags) or log/return an
error; alternatively add/use a transformTo overload that accepts explicit
normalization parameters and pass them from ImgDetection so the remap is
unambiguous.
In `@src/pipeline/datatype/SpatialImgDetections.cpp`:
- Around line 174-178: The current SpatialImgDetections::transformTo silently
returns untransformed data; change it to fail fast by replacing the placeholder
passthrough with an immediate exception (e.g., throw std::runtime_error) or a
CHECK/ASSERT that indicates transformTo is unimplemented, mentioning the target
transformation and the class name; also make transformToInternal(const
ImgTransformation&) either callable or similarly marked unimplemented so callers
can't accidentally use transformTo without explicit implementation. Ensure you
remove the "return *this" passthrough in SpatialImgDetections::transformTo and
provide a clear unimplemented error path referencing
SpatialImgDetections::transformTo and transformToInternal.
In `@tests/src/onhost_tests/pipeline/datatype/imgdetections_test.cpp`:
- Around line 255-281: Add assertions that the source ImgDetections object is
not mutated by ImgDetections::transformTo: after calling
detections.transformTo(target) assert detections.detections.size() is still the
original count, then check the original detection's bounding box (use
detections.detections.front().getBoundingBox()) for the expected center, size
and angle values and assert its keypoints (use
detections.detections.front().getKeypoints()) still have the original
imageCoordinates.x/y/z and confidence values and original edges (getEdges())
unchanged; place these REQUIREs immediately after the transformTo call and
before validating the transformed object.
🪄 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: eef6b5bd-3054-4291-8b79-1e5aeb0075d0
📒 Files selected for processing (13)
CMakeLists.txtbindings/python/src/pipeline/datatype/ImgDetectionsBindings.cppinclude/depthai/common/Keypoint.hppinclude/depthai/pipeline/datatype/ImgDetections.hppinclude/depthai/pipeline/datatype/ImgDetectionsT.hppinclude/depthai/pipeline/datatype/SpatialImgDetections.hppinclude/depthai/pipeline/datatype/TransformableBuffer.hppinclude/depthai/pipeline/datatypes.hppsrc/pipeline/datatype/ImgDetections.cppsrc/pipeline/datatype/SpatialImgDetections.cppsrc/pipeline/datatype/StreamMessageParser.cppsrc/pipeline/datatype/TransformableBuffer.cpptests/src/onhost_tests/pipeline/datatype/imgdetections_test.cpp
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 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/datatype/StreamMessageParser.cppsrc/pipeline/datatype/SpatialImgDetections.cppsrc/pipeline/datatype/TransformableBuffer.cppsrc/pipeline/datatype/ImgDetections.cpp
🔇 Additional comments (10)
include/depthai/common/Keypoint.hpp (1)
32-39: Good transformation flow for keypoints and keypoint lists.The remap implementation is consistent and the list-level API correctly returns a transformed copy without mutating the original object.
Also applies to: 94-100
include/depthai/pipeline/datatype/TransformableBuffer.hpp (1)
10-24: Solid base abstraction for transform-capable datatypes.The protected virtual hook plus shared transformation storage/serialization gives a clean foundation for derived message-specific remapping.
include/depthai/pipeline/datatypes.hpp (1)
40-40: Good umbrella-header exposure for the new base type.Including
TransformableBuffer.hpphere keeps the public datatype surface consistent.src/pipeline/datatype/TransformableBuffer.cpp (1)
7-15: Implementation matches the header contract cleanly.Destructor and transformation accessors are correctly wired to the new base state.
CMakeLists.txt (1)
346-346: Build integration is correct.Adding
TransformableBuffer.cpptoTARGET_CORE_SOURCESproperly links the new datatype base implementation.src/pipeline/datatype/StreamMessageParser.cpp (1)
21-21: Include update is fine for the new transformable datatype hierarchy.No concerns with this parser-side integration change.
include/depthai/pipeline/datatype/ImgDetectionsT.hpp (1)
21-29: Good consolidation of transformation state into the new base class.This migration cleanly aligns
ImgDetectionsTwithTransformableBufferand avoids duplicated transformation storage.src/pipeline/datatype/ImgDetections.cpp (1)
191-195: Copy-then-transform behavior is solid here.Returning a transformed copy preserves original
ImgDetectionsstate and keeps error handling localized.bindings/python/src/pipeline/datatype/ImgDetectionsBindings.cpp (2)
87-89: Python exposure forImgDetection.transformlooks correct.The new binding cleanly maps the C++ transform API with explicit
sourceandtargetargs.
156-156:ImgDetections.transformTobinding is a good addition.This matches the new C++ API and makes host-side remapping available from Python.
aljazkonec1
commented
May 4, 2026
| : Keypoint(Point3f{x, y, z}, confidence, label, std::move(labelName)) {} | ||
|
|
||
| void transform(const ImgTransformation& source, const ImgTransformation& target) { | ||
| // TODO (aljazkonec1) Possible issue: Keypoint does not store its own normalization flag, so normalization is guessed based on if coordinates are in |
…rmableBuffer datatypes.
aljazkonec1
added
testable
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@include/depthai/pipeline/datatype/AprilTags.hpp`:
- Line 99: The DEPTHAI_SERIALIZE invocation in AprilTags.hpp currently lists
TransformableBuffer::transformation but the file already has "using
TransformableBuffer::transformation;" so change the macro call to use the
unqualified name "transformation" for consistency with SegmentationMask.hpp and
ImgDetections.hpp; update the DEPTHAI_SERIALIZE(AprilTags, sequenceNum, ts,
tsDevice, TransformableBuffer::transformation, aprilTags) entry to
DEPTHAI_SERIALIZE(AprilTags, sequenceNum, ts, tsDevice, transformation,
aprilTags) so the unqualified symbol from the using-declaration is used.
In `@include/depthai/pipeline/datatype/PointCloudData.hpp`:
- Around line 25-27: The PointCloudData class currently throws in the required
TransformableBuffer override transformToInternal, so implement
transformToInternal in PointCloudData (the override declared in PointCloudData)
to perform the coordinate transformation instead of throwing: map the stored
point positions (and any per-point normals if present) through the provided
ImgTransformation (apply rotation/translation/scale as defined by
ImgTransformation) producing transformed coordinates, and update or produce a
new internal buffer as needed; ensure semantics align with cloneAndTransformTo()
by leaving the original object unchanged when clone is expected and by using
clone() to create a deep copy if the interface requires it. Locate the
transformToInternal override in the PointCloudData class and replace the logic
that throws std::logic_error("PointCloudData transform not implemented yet")
with an implementation that iterates points, applies ImgTransformation to
positions (and normals if present), and returns/assigns the transformed buffer
in accordance with TransformableBuffer expectations.
In `@include/depthai/pipeline/datatype/SpatialImgDetections.hpp`:
- Line 209: The member declaration for transformTo should be marked const
because it does not modify the object; update the declaration
SpatialImgDetections transformTo(const ImgTransformation& target); to
SpatialImgDetections transformTo(const ImgTransformation& target) const and
ensure the corresponding method definition/implementation (the
SpatialImgDetections::transformTo(...) method) is updated to include the
trailing const as well so signatures match.
In `@include/depthai/pipeline/datatype/Tracklets.hpp`:
- Line 124: The DEPTHAI_SERIALIZE line in Tracklets uses the qualified name
TransformableBuffer::transformation even though the class already declares using
TransformableBuffer::transformation; for consistency with SegmentationMask and
ImgDetections change the serialization invocation to use the unqualified
transformation identifier (i.e., list transformation instead of
TransformableBuffer::transformation) in the DEPTHAI_SERIALIZE(Tracklets, ...)
call so it matches the other serialization macros.
🪄 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: 02ab9f3d-d5c4-4fed-af95-9386e0712e95
📒 Files selected for processing (6)
include/depthai/pipeline/datatype/AprilTags.hppinclude/depthai/pipeline/datatype/ImgDetections.hppinclude/depthai/pipeline/datatype/PointCloudData.hppinclude/depthai/pipeline/datatype/SegmentationMask.hppinclude/depthai/pipeline/datatype/SpatialImgDetections.hppinclude/depthai/pipeline/datatype/Tracklets.hpp
📜 Review details
🔇 Additional comments (16)
include/depthai/pipeline/datatype/SegmentationMask.hpp (1)
10-10: LGTM!Also applies to: 27-30, 39-45, 210-210
include/depthai/pipeline/datatype/AprilTags.hpp (1)
8-8: LGTM!Also applies to: 65-68, 71-74, 81-90
include/depthai/pipeline/datatype/ImgDetections.hpp (1)
3-3: LGTM!Also applies to: 15-15, 155-160, 171-173, 182-186, 192-202, 219-219
include/depthai/pipeline/datatype/Tracklets.hpp (1)
10-10: LGTM!Also applies to: 67-73, 81-84, 87-92, 94-97, 111-120
include/depthai/pipeline/datatype/SpatialImgDetections.hpp (8)
7-7: LGTM!
27-28: LGTM!
155-162: LGTM!
172-174: LGTM!
183-186: LGTM!
188-191: LGTM!
199-208: LGTM!
227-227: LGTM!include/depthai/pipeline/datatype/PointCloudData.hpp (4)
10-10: LGTM!
24-24: LGTM!
44-46: ⚡ Quick winRemove
usingdeclarations that conflict with explicit method overrides.Lines 44–46 introduce
usingdeclarations forgetTransformation()andsetTransformation()from theTransformableBufferbase class. However, PointCloudData declares and implements these methods with different signatures and behavior:
- Base class
getTransformation()returnsstd::optional<ImgTransformation>; PointCloudData's version returnsconst ImgTransformation&and throws if not set- Base class
setTransformation()returnsvoid; PointCloudData's version returnsPointCloudData&for method chainingThe
usingdeclarations at lines 44–46 should be removed since PointCloudData intentionally overrides these methods with incompatible signatures. The explicit declarations at lines 220–227 and their implementations in PointCloudData.cpp are correct and necessary.> Likely an incorrect or invalid review comment.
281-282: The serialization is correct and already validated by existing test coverage. The test case "PointCloudData metadata serialization roundtrip" (line 154 of pointclouddata_test.cpp) confirms that the mixed qualified/unqualified member names in the DEPTHAI_SERIALIZE macro work properly, and all metadata—including sequenceNum from the Buffer base class and transformation from TransformableBuffer—serialize and deserialize correctly. The using declarations at lines 41–46 of PointCloudData.hpp properly expose these base-class members, enabling the serialization macro to access them.
aljazkonec1
commented
May 12, 2026
moratom
reviewed
May 15, 2026
Collaborator
moratom
left a comment
moratom
left a comment
There was a problem hiding this comment.
Looks good in general; two questions:
- If a user creats a transforamble type in Python, can he use the interface for its message?
- Do we need TransforambleBuffer or can it be a separate interface?
Comment on lines
53
to
55
| .def("getTransformation", [](AprilTags& msg) { return msg.transformation; }) | ||
| .def("setTransformation", [](AprilTags& msg, const std::optional<ImgTransformation>& transformation) { msg.transformation = transformation; }) | ||
| // .def("setTimestamp", &AprilTags::setTimestamp, DOC(dai, Buffer, setTimestamp)) |
Collaborator
There was a problem hiding this comment.
If we inherit we can avoid this + isinstance(dai.TransformableBuffer, dai.AprilTagsMessage)
| */ | ||
| class SpatialImgDetections : public ImgDetectionsT<SpatialImgDetection>, public ProtoSerializable { | ||
| protected: | ||
| void transformToInternal(const ImgTransformation& target) override; |
Collaborator
There was a problem hiding this comment.
comment on what this does
Comment on lines
+10
to
+33
| class TransformableBuffer : public Buffer { | ||
| protected: | ||
| virtual void transformToInternal(const ImgTransformation& target) = 0; | ||
| virtual std::shared_ptr<TransformableBuffer> clone() const = 0; | ||
|
|
||
| public: | ||
| virtual ~TransformableBuffer(); | ||
| using Buffer::Buffer; | ||
| std::optional<ImgTransformation> transformation; | ||
|
|
||
| std::optional<ImgTransformation> getTransformation() const; | ||
|
|
||
| void setTransformation(const ImgTransformation& transformation); | ||
|
|
||
| std::shared_ptr<TransformableBuffer> cloneAndTransformTo(const ImgTransformation& target) const; | ||
|
|
||
| DatatypeEnum getDatatype() const override { | ||
| return DatatypeEnum::TransformableBuffer; | ||
| } | ||
|
|
||
| DEPTHAI_SERIALIZE(TransformableBuffer, Buffer::ts, Buffer::tsDevice, Buffer::sequenceNum, transformation); | ||
| }; | ||
|
|
||
| } // namespace dai No newline at end of file |
Collaborator
There was a problem hiding this comment.
Could we go with a separate interface (since it doesn't really have to inherit from a message)?
Just Transformable for example.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@include/depthai/common/SpatialKeypoint.hpp`:
- Around line 124-145: In transformTo (SpatialKeypointsList::transformTo in
SpatialKeypoint.hpp) avoid calling source.getExtrinsicsTransformationMatrixTo up
front; instead defer that call until you encounter the first keypoint with
kp.spatialCoordinates.z > 0 and cache the returned matrix (e.g., optional
transMatrix + a bool or std::optional) so subsequent 3D keypoints reuse it;
leave the 2D remap path unchanged and continue using matrix::transformPoint3f
with the cached transMatrix for positive-depth points.
In `@src/pipeline/datatype/SpatialImgDetections.cpp`:
- Around line 226-235: The method SpatialImgDetections::transformToInternal
mutates the member vector detections before setTransformation, which can leave
the object in an inconsistent state if a detection.transform throws; fix this by
making a local copy of detections, iterate and call detection.transform(source,
target, unit) on the copies, and only after the loop completes without
exceptions assign the transformed copy back to the member detections and call
setTransformation(target); use the existing symbols detections,
detection.transform, getTransformation(), setTransformation(), source, target,
and unit to implement the copy-then-commit semantics.
- Around line 205-223: In SpatialImgDetection::transform, avoid calling
getExtrinsicsTransformationMatrixTo unconditionally; move the extrinsics/matrix
resolution inside the depth > 0 branch so that only the 3D path computes
transMatrix and calls matrix::transformPoint3f and projectRectTo (used by
setBoundingBox and setSpatialCoordinate), while the depth <= 0 path simply calls
source.remapRectTo without touching extrinsics; ensure
transformKeypointsFallback remains unchanged (it can still use
scaledFallbackCoordinates) and no extrinsics lookups occur on the 2D-only path.
In `@tests/src/ondevice_tests/pipeline/node/object_tracker_test.cpp`:
- Around line 142-147: The test dereferences frame returned from
trackerFrameQ->get<dai::ImgFrame>() without verifying it's non-null; add an
explicit null check by inserting REQUIRE(frame != nullptr); immediately after
the frame assignment (right after the trackerFrameQ->get<dai::ImgFrame>() call)
so subsequent calls to frame->transformation and comparisons in the test
(getSize, getSourceSize, getIntrinsicMatrix) are safe.
🪄 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: e860cce7-6d24-441e-a694-74eb8eedbab2
📒 Files selected for processing (10)
cmake/Depthai/DepthaiDeviceRVC4Config.cmakecmake/Depthai/DepthaiDeviceSideConfig.cmakeinclude/depthai/common/SpatialKeypoint.hppprotos/SpatialImgDetections.protosrc/pipeline/datatype/PointCloudData.cppsrc/pipeline/datatype/SegmentationMask.cppsrc/pipeline/datatype/SpatialImgDetections.cppsrc/pipeline/datatype/StreamMessageParser.cppsrc/pipeline/node/AprilTag.cpptests/src/ondevice_tests/pipeline/node/object_tracker_test.cpp
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 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/datatype/PointCloudData.cppsrc/pipeline/node/AprilTag.cppsrc/pipeline/datatype/StreamMessageParser.cppsrc/pipeline/datatype/SegmentationMask.cppsrc/pipeline/datatype/SpatialImgDetections.cpp
🔇 Additional comments (9)
cmake/Depthai/DepthaiDeviceRVC4Config.cmake (1)
6-6: LGTM!cmake/Depthai/DepthaiDeviceSideConfig.cmake (1)
5-5: LGTM!protos/SpatialImgDetections.proto (1)
16-16: LGTM!src/pipeline/datatype/PointCloudData.cpp (1)
4-4: LGTM!Also applies to: 124-128, 182-190
src/pipeline/node/AprilTag.cpp (1)
326-326: LGTM!src/pipeline/datatype/StreamMessageParser.cpp (1)
21-21: LGTM!Also applies to: 171-173
src/pipeline/datatype/SegmentationMask.cpp (1)
6-6: LGTM!Also applies to: 36-43
src/pipeline/datatype/SpatialImgDetections.cpp (1)
3-16: LGTM!Also applies to: 117-117, 184-203, 238-245
include/depthai/common/SpatialKeypoint.hpp (1)
7-15: LGTM!Also applies to: 38-99, 149-149
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
bindings/python/src/pipeline/datatype/AprilTagsBindings.cpp (1)
46-54:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBind
AprilTags::transformTofor Python.The C++ type now exposes
transformTo(const ImgTransformation&), but this binding never publishes it, so the new remapping API is unavailable from Python.Suggested binding
aprilTags.def(py::init<>()) .def("__repr__", &AprilTags::str) .def_property( "aprilTags", [](AprilTags& det) { return &det.aprilTags; }, [](AprilTags& det, std::vector<AprilTag> val) { det.aprilTags = val; }) .def("getTimestamp", &AprilTags::Buffer::getTimestamp, DOC(dai, Buffer, getTimestamp)) .def("getTimestampDevice", &AprilTags::Buffer::getTimestampDevice, DOC(dai, Buffer, getTimestampDevice)) .def("getSequenceNum", &AprilTags::Buffer::getSequenceNum, DOC(dai, Buffer, getSequenceNum)) .def("getTransformation", [](AprilTags& msg) { return msg.transformation; }) .def("setTransformation", [](AprilTags& msg, const std::optional<ImgTransformation>& transformation) { msg.transformation = transformation; }) + .def("transformTo", &AprilTags::transformTo, py::arg("target")) // .def("setTimestamp", &AprilTags::setTimestamp, DOC(dai, Buffer, setTimestamp))🤖 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/python/src/pipeline/datatype/AprilTagsBindings.cpp` around lines 46 - 54, Add a Python binding for AprilTags::transformTo so the new remapping API is exposed; locate the AprilTags pybind block (where AprilTags::str, aprilTags property, getTimestamp/getTimestampDevice/getSequenceNum, getTransformation/setTransformation are defined) and add a .def("transformTo", ...) that accepts a const ImgTransformation& (or std::optional if the C++ overload uses it) and calls AprilTags::transformTo(const ImgTransformation&), exposing the return value to Python; ensure the bound name is "transformTo" and use either a direct pointer-to-member or a small lambda wrapper to match pybind's expected signature.bindings/python/src/pipeline/datatype/TrackletsBindings.cpp (1)
61-78:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExpose the new
Trackletstransform surface in Python.
Trackletsgained bothunitandtransformTo(...)in C++, but neither is bound here. That makes the new remapping feature unusable from Python and hides the unit needed to interpretspatialCoordinatescorrectly.Suggested binding additions
tracklets.def(py::init<>()) .def("__repr__", &Tracklets::str) + .def_readwrite("unit", &Tracklets::unit) .def_property( "tracklets", [](Tracklets& track) { return &track.tracklets; }, [](Tracklets& track, std::vector<Tracklet> val) { track.tracklets = val; }, DOC(dai, Tracklets, tracklets)) .def("getTimestamp", &Tracklets::Buffer::getTimestamp, DOC(dai, Buffer, getTimestamp)) .def("getTimestampDevice", &Tracklets::Buffer::getTimestampDevice, DOC(dai, Buffer, getTimestampDevice)) .def("getSequenceNum", &Tracklets::Buffer::getSequenceNum, DOC(dai, Buffer, getSequenceNum)) .def("getTransformation", [](Tracklets& msg) { if(!msg.transformation.has_value()) { throw std::runtime_error("Transformation is not set"); } return *msg.transformation; }) .def("setTransformation", [](Tracklets& msg, const ImgTransformation& transformation) { msg.transformation = transformation; }) + .def("transformTo", &Tracklets::transformTo, py::arg("target"))
♻️ Duplicate comments (4)
src/pipeline/datatype/SegmentationMask.cpp (1)
36-38:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winThis transform path still returns an unmodified mask.
SegmentationMasknow inheritsTransformable, buttransformToInternal()is empty. The inheritedtransformToAPI can therefore report success while every pixel remains in the source frame, which breaks any caller expecting target-space labels. Please either implement label-preserving remapping here or fail fast / removeTransformablefrom this datatype until the remap exists.Suggested minimal safeguard
-void SegmentationMask::transformToInternal(const ImgTransformation&) { - // Transform the segmentation mask by using ImageAlign for faster processing. +void SegmentationMask::transformToInternal(const ImgTransformation&) { + throw std::logic_error( + "SegmentationMask::transformTo() is not implemented. Use ImageAlign before constructing the mask."); }🤖 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/pipeline/datatype/SegmentationMask.cpp` around lines 36 - 38, SegmentationMask::transformToInternal currently does nothing, so Transformable::transformTo can report success while pixels remain in source space; either implement a label-preserving remap using the existing ImageAlign-based pipeline (apply the ImgTransformation to each mask label/indices and resample with nearest-neighbor semantics so labels don’t blend) inside SegmentationMask::transformToInternal, or make the method fail fast by throwing/returning an error and/or remove the Transformable inheritance until the remapper is implemented; search for SegmentationMask::transformToInternal, the Transformable base API, and existing ImageAlign helpers to reuse nearest-neighbor remapping logic and preserve discrete label values.src/pipeline/datatype/PointCloudData.cpp (1)
182-184:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDon't silently no-op the point-cloud transform path.
PointCloudDatanow opts into the public transformable API, but this override leaves the payload unchanged. AtransformTo/cloneAndTransformTocall can therefore succeed without remapping any points, which is worse than the previous fail-fast behavior. If post-generation point-cloud remapping is intentionally unsupported, reject it here (or dropTransformableCRTP<PointCloudData>for now) instead of returning a "transformed" cloud with source-frame coordinates.Suggested minimal safeguard
-void PointCloudData::transformToInternal(const ImgTransformation&) { - // For efficiency reasons, point clouds should be created from aligned messages instead of tranforming them after the fact +void PointCloudData::transformToInternal(const ImgTransformation&) { + throw std::logic_error( + "PointCloudData::transformTo() is not supported. Create the point cloud from an already aligned input instead."); }🤖 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/pipeline/datatype/PointCloudData.cpp` around lines 182 - 184, The override PointCloudData::transformToInternal currently no-ops which falsely indicates a successful transform for callers using transformTo or cloneAndTransformTo; either implement proper point remapping (apply the ImgTransformation to point coordinates and update frame metadata) or explicitly reject unsupported transforms by making PointCloudData::transformToInternal throw/return an error (or remove TransformableCRTP<PointCloudData> so transform paths never call into it). Update the implementation of PointCloudData::transformToInternal (and any cloneAndTransformTo helper) to perform coordinate remapping and frame updates, or change it to fail-fast with a clear exception/error indicating post-generation transforms are unsupported.src/pipeline/datatype/SpatialImgDetections.cpp (2)
211-217:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDefer the extrinsics lookup to the positive-depth branch.
getExtrinsicsTransformationMatrixTo()runs before thedepth > 0check, so detections that should fall back toremapRectTo()can still fail on missing extrinsics.Suggested fix
- const auto transMatrix = source.getExtrinsicsTransformationMatrixTo(target, false, lengthUnit); - if(depth > 0) { + const auto transMatrix = source.getExtrinsicsTransformationMatrixTo(target, false, lengthUnit); setBoundingBox(source.projectRectTo(target, rect, depth)); setSpatialCoordinate(matrix::transformPoint3f(transMatrix, spatialCoordinates)); } else { setBoundingBox(source.remapRectTo(target, rect)); }🤖 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/pipeline/datatype/SpatialImgDetections.cpp` around lines 211 - 217, Move the extrinsics lookup so it only runs when depth > 0: instead of calling getExtrinsicsTransformationMatrixTo(...) before the depth check, call it inside the depth > 0 branch and then call setBoundingBox(source.projectRectTo(target, rect, depth)) and setSpatialCoordinate(matrix::transformPoint3f(transMatrix, spatialCoordinates)); otherwise continue to call setBoundingBox(source.remapRectTo(target, rect)) in the else branch; ensure you reference getExtrinsicsTransformationMatrixTo, projectRectTo, remapRectTo, setBoundingBox, setSpatialCoordinate and matrix::transformPoint3f when making the change and handle any error/optional return from getExtrinsicsTransformationMatrixTo appropriately.
227-236:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake
transformToInternal()copy-then-commit.If one
detection.transform(...)throws, earlier detections stay mutated whilegetTransformation()still reports the source frame. Transform a local copy and only assign it back after the full loop succeeds.Suggested fix
void SpatialImgDetections::transformToInternal(const ImgTransformation& target) { if(!this->getTransformation().has_value()) { throw std::runtime_error("Source transformation is not set, cannot transform spatial image detections."); } ImgTransformation source = *this->getTransformation(); - for(auto& detection : detections) { + auto transformedDetections = detections; + for(auto& detection : transformedDetections) { detection.transform(source, target, unit); } + detections = std::move(transformedDetections); setTransformation(target); }🤖 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/pipeline/datatype/SpatialImgDetections.cpp` around lines 227 - 236, transformToInternal currently mutates this->detections in-place and can leave the object partially updated if a detection.transform throws; make it copy-then-commit by copying detections into a local temp (e.g., auto temp = detections), perform the loop calling detection.transform(source, target, unit) on elements of temp using the same source and unit from this->getTransformation(), and only after the entire loop completes successfully move or swap temp back into this->detections and then call setTransformation(target) so the transformation is committed atomically.
🤖 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/python/src/DatatypeBindings.hpp`:
- Around line 38-40: The override for Transformable specializations uses
PYBIND11_OVERRIDE_PURE_NAME for the method transformToInternal which forces a
Python override; change it to PYBIND11_OVERRIDE_NAME so the C++ implementation
(e.g., ImgDetections) can be used as a fallback when Python doesn't override
transformTo, updating the specialization in DatatypeBindings.hpp where
transformToInternal is defined to match the non-pure pattern used in the primary
template.
In `@bindings/python/src/pipeline/datatype/TrackletsBindings.cpp`:
- Around line 71-78: The getter currently throws when msg.transformation is
unset; change Tracklets::getTransformation to preserve the optional semantics
(return an optional/None to Python instead of throwing) and update
setTransformation to accept an optional (or None) so callers can clear the
field. Specifically, modify the lambda for "getTransformation" to return
msg.transformation as an optional (or py::none when !has_value()) and change the
"setTransformation" binder to accept std::optional<ImgTransformation> (or
py::object/None) and set msg.transformation = value or reset/clear it when None
is received; keep the function names Tracklets::getTransformation,
Tracklets::setTransformation and the field msg.transformation to locate the
changes.
In `@src/pipeline/datatype/ImgDetections.cpp`:
- Around line 182-191: The method ImgDetections::transformToInternal updates
detections and the stored transformation but ignores any attached segmentation
mask, leaving message coordinates inconsistent; update transformToInternal so
that after computing source = *getTransformation() you also remap the
segmentation mask to the target frame (e.g., call the mask's transform/remap
routine or use ImgTransformation utilities) before calling
setTransformation(target), or if no mask remapper exists, explicitly clear/unset
the segmentation mask to avoid mixed-frame state; refer to
ImgDetections::transformToInternal, the detections collection, and the
segmentation mask member when making the change.
---
Outside diff comments:
In `@bindings/python/src/pipeline/datatype/AprilTagsBindings.cpp`:
- Around line 46-54: Add a Python binding for AprilTags::transformTo so the new
remapping API is exposed; locate the AprilTags pybind block (where
AprilTags::str, aprilTags property,
getTimestamp/getTimestampDevice/getSequenceNum,
getTransformation/setTransformation are defined) and add a .def("transformTo",
...) that accepts a const ImgTransformation& (or std::optional if the C++
overload uses it) and calls AprilTags::transformTo(const ImgTransformation&),
exposing the return value to Python; ensure the bound name is "transformTo" and
use either a direct pointer-to-member or a small lambda wrapper to match
pybind's expected signature.
---
Duplicate comments:
In `@src/pipeline/datatype/PointCloudData.cpp`:
- Around line 182-184: The override PointCloudData::transformToInternal
currently no-ops which falsely indicates a successful transform for callers
using transformTo or cloneAndTransformTo; either implement proper point
remapping (apply the ImgTransformation to point coordinates and update frame
metadata) or explicitly reject unsupported transforms by making
PointCloudData::transformToInternal throw/return an error (or remove
TransformableCRTP<PointCloudData> so transform paths never call into it). Update
the implementation of PointCloudData::transformToInternal (and any
cloneAndTransformTo helper) to perform coordinate remapping and frame updates,
or change it to fail-fast with a clear exception/error indicating
post-generation transforms are unsupported.
In `@src/pipeline/datatype/SegmentationMask.cpp`:
- Around line 36-38: SegmentationMask::transformToInternal currently does
nothing, so Transformable::transformTo can report success while pixels remain in
source space; either implement a label-preserving remap using the existing
ImageAlign-based pipeline (apply the ImgTransformation to each mask
label/indices and resample with nearest-neighbor semantics so labels don’t
blend) inside SegmentationMask::transformToInternal, or make the method fail
fast by throwing/returning an error and/or remove the Transformable inheritance
until the remapper is implemented; search for
SegmentationMask::transformToInternal, the Transformable base API, and existing
ImageAlign helpers to reuse nearest-neighbor remapping logic and preserve
discrete label values.
In `@src/pipeline/datatype/SpatialImgDetections.cpp`:
- Around line 211-217: Move the extrinsics lookup so it only runs when depth >
0: instead of calling getExtrinsicsTransformationMatrixTo(...) before the depth
check, call it inside the depth > 0 branch and then call
setBoundingBox(source.projectRectTo(target, rect, depth)) and
setSpatialCoordinate(matrix::transformPoint3f(transMatrix, spatialCoordinates));
otherwise continue to call setBoundingBox(source.remapRectTo(target, rect)) in
the else branch; ensure you reference getExtrinsicsTransformationMatrixTo,
projectRectTo, remapRectTo, setBoundingBox, setSpatialCoordinate and
matrix::transformPoint3f when making the change and handle any error/optional
return from getExtrinsicsTransformationMatrixTo appropriately.
- Around line 227-236: transformToInternal currently mutates this->detections
in-place and can leave the object partially updated if a detection.transform
throws; make it copy-then-commit by copying detections into a local temp (e.g.,
auto temp = detections), perform the loop calling detection.transform(source,
target, unit) on elements of temp using the same source and unit from
this->getTransformation(), and only after the entire loop completes successfully
move or swap temp back into this->detections and then call
setTransformation(target) so the transformation is committed atomically.
🪄 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: 25789282-a907-4d08-b365-2c0a3bda1bbe
📒 Files selected for processing (33)
CMakeLists.txtbindings/python/CMakeLists.txtbindings/python/src/DatatypeBindings.cppbindings/python/src/DatatypeBindings.hppbindings/python/src/pipeline/datatype/AprilTagsBindings.cppbindings/python/src/pipeline/datatype/ImgDetectionsBindings.cppbindings/python/src/pipeline/datatype/PointCloudDataBindings.cppbindings/python/src/pipeline/datatype/SegmentationMaskBindings.cppbindings/python/src/pipeline/datatype/SpatialImgDetectionsBindings.cppbindings/python/src/pipeline/datatype/TrackletsBindings.cppbindings/python/src/pipeline/datatype/TransformableBindings.cppinclude/depthai/pipeline/datatype/AprilTags.hppinclude/depthai/pipeline/datatype/DatatypeEnum.hppinclude/depthai/pipeline/datatype/ImgDetections.hppinclude/depthai/pipeline/datatype/ImgDetectionsT.hppinclude/depthai/pipeline/datatype/PointCloudData.hppinclude/depthai/pipeline/datatype/SegmentationMask.hppinclude/depthai/pipeline/datatype/SpatialImgDetections.hppinclude/depthai/pipeline/datatype/Tracklets.hppinclude/depthai/pipeline/datatype/Transformable.hppinclude/depthai/pipeline/datatypes.hppinclude/depthai/pipeline/node/ImageAlign.hppsrc/pipeline/datatype/AprilTags.cppsrc/pipeline/datatype/DatatypeEnum.cppsrc/pipeline/datatype/ImgDetections.cppsrc/pipeline/datatype/PointCloudData.cppsrc/pipeline/datatype/SegmentationMask.cppsrc/pipeline/datatype/SpatialImgDetections.cppsrc/pipeline/datatype/StreamMessageParser.cppsrc/pipeline/datatype/Tracklets.cppsrc/pipeline/datatype/Transformable.cppsrc/pipeline/node/host/Replay.cppsrc/utility/ProtoSerialize.cpp
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 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/host/Replay.cppsrc/pipeline/datatype/Transformable.cppsrc/pipeline/datatype/StreamMessageParser.cppsrc/pipeline/datatype/Tracklets.cppsrc/pipeline/datatype/AprilTags.cppsrc/pipeline/datatype/SpatialImgDetections.cppsrc/pipeline/datatype/ImgDetections.cppsrc/pipeline/datatype/PointCloudData.cppsrc/pipeline/datatype/SegmentationMask.cppsrc/pipeline/datatype/DatatypeEnum.cpp
🔇 Additional comments (28)
include/depthai/pipeline/node/ImageAlign.hpp (1)
40-40: LGTM!Also applies to: 49-51, 57-57, 102-102
include/depthai/pipeline/datatype/Transformable.hpp (3)
11-25: LGTM!
27-35: LGTM!
37-55: LGTM!src/pipeline/datatype/Transformable.cpp (1)
1-23: LGTM!include/depthai/pipeline/datatype/DatatypeEnum.hpp (1)
59-60: LGTM!src/pipeline/datatype/DatatypeEnum.cpp (2)
66-66: LGTM!
117-125: LGTM!include/depthai/pipeline/datatypes.hpp (1)
40-40: LGTM!src/pipeline/datatype/StreamMessageParser.cpp (2)
21-21: LGTM!
171-172: LGTM!src/pipeline/node/host/Replay.cpp (2)
69-69: LGTM!
153-153: LGTM!src/utility/ProtoSerialize.cpp (2)
232-232: LGTM!
435-435: LGTM!Also applies to: 466-466
CMakeLists.txt (1)
346-346: LGTM!bindings/python/CMakeLists.txt (1)
121-121: LGTM!include/depthai/pipeline/datatype/ImgDetections.hpp (1)
3-3: LGTM!Also applies to: 15-15, 155-161, 170-187, 193-204, 220-220
include/depthai/pipeline/datatype/ImgDetectionsT.hpp (1)
10-10: LGTM!Also applies to: 31-31
src/pipeline/datatype/ImgDetections.cpp (1)
3-6: LGTM!Also applies to: 14-14, 167-172, 193-195
bindings/python/src/pipeline/datatype/ImgDetectionsBindings.cpp (1)
23-24: LGTM!Also applies to: 88-90, 157-157
bindings/python/src/DatatypeBindings.cpp (1)
10-10: LGTM!Also applies to: 60-61, 128-128
bindings/python/src/pipeline/datatype/TransformableBindings.cpp (1)
1-34: LGTM!bindings/python/src/pipeline/datatype/PointCloudDataBindings.cpp (1)
20-21: LGTM!bindings/python/src/pipeline/datatype/SegmentationMaskBindings.cpp (1)
42-43: LGTM!bindings/python/src/pipeline/datatype/SpatialImgDetectionsBindings.cpp (1)
23-24: LGTM!include/depthai/pipeline/datatype/SpatialImgDetections.hpp (1)
7-18: LGTM!Also applies to: 28-31, 157-164, 172-228
src/pipeline/datatype/SpatialImgDetections.cpp (1)
118-118: LGTM!Also applies to: 185-204, 239-240
Comment on lines
+71
to
78
| .def("getTransformation", | ||
| [](Tracklets& msg) { | ||
| if(!msg.transformation.has_value()) { | ||
| throw std::runtime_error("Transformation is not set"); | ||
| } | ||
| return *msg.transformation; | ||
| }) | ||
| .def("setTransformation", [](Tracklets& msg, const ImgTransformation& transformation) { msg.transformation = transformation; }) |
Contributor
There was a problem hiding this comment.
Keep transformation optional in the Python API.
The underlying field is still optional, but this getter now throws on a perfectly valid unset state and the setter still cannot clear it. That breaks parity with the C++ contract and with the AprilTags binding in this same PR.
Suggested fix
- .def("getTransformation",
- [](Tracklets& msg) {
- if(!msg.transformation.has_value()) {
- throw std::runtime_error("Transformation is not set");
- }
- return *msg.transformation;
- })
- .def("setTransformation", [](Tracklets& msg, const ImgTransformation& transformation) { msg.transformation = transformation; })
+ .def("getTransformation", [](Tracklets& msg) { return msg.transformation; })
+ .def("setTransformation", [](Tracklets& msg, const std::optional<ImgTransformation>& transformation) { msg.transformation = transformation; })📝 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.
Suggested change
| .def("getTransformation", | |
| [](Tracklets& msg) { | |
| if(!msg.transformation.has_value()) { | |
| throw std::runtime_error("Transformation is not set"); | |
| } | |
| return *msg.transformation; | |
| }) | |
| .def("setTransformation", [](Tracklets& msg, const ImgTransformation& transformation) { msg.transformation = transformation; }) | |
| .def("getTransformation", [](Tracklets& msg) { return msg.transformation; }) | |
| .def("setTransformation", [](Tracklets& msg, const std::optional<ImgTransformation>& transformation) { msg.transformation = transformation; }) |
🤖 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/python/src/pipeline/datatype/TrackletsBindings.cpp` around lines 71
- 78, The getter currently throws when msg.transformation is unset; change
Tracklets::getTransformation to preserve the optional semantics (return an
optional/None to Python instead of throwing) and update setTransformation to
accept an optional (or None) so callers can clear the field. Specifically,
modify the lambda for "getTransformation" to return msg.transformation as an
optional (or py::none when !has_value()) and change the "setTransformation"
binder to accept std::optional<ImgTransformation> (or py::object/None) and set
msg.transformation = value or reset/clear it when None is received; keep the
function names Tracklets::getTransformation, Tracklets::setTransformation and
the field msg.transformation to locate the changes.
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
3 participants
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.
Purpose
This PR is the first step towards a general implementation of the AlignNode and general message remapping between different coordinate systems. It adds an abstract
TransformableBufferclass to unify message types that require ImgTransformations (ImgDetections, AprilTags, Keypoints, .... ). Each of these messages should implement thetransformToInternalfunction and expose a publictransformToAPI that remaps the message to the target ImgTransformation.In addition,
ImgDetectionTandImgDetectionshave been updated to implement the class and the transformTo functions. A test was also added.Specification
None / not applicable
Dependencies & Potential Impact
None / not applicable
Deployment Plan
None / not applicable
Testing & Validation
A test was also added.
AI Usage
Assisted-by: codex:gpt-5-4 Extra High
Submitted code was reviewed by a human: YES
The author is taking the responsibility for the contribution: YES
Summary by CodeRabbit
Release Notes