-
Notifications
You must be signed in to change notification settings - Fork 159
Feature/vpp node #1528
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Feature/vpp node #1528
Conversation
9dda8e1 to
4c899d0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Just left some comment about missing setter functions and their docstrings and pybindings.
If possible, add an example to its easier to interprit the results. Otherwise LGTM
| InjectionParameters injectionParameters; | ||
| int maxNumThreads = 8; | ||
| int maxFPS = 0; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other configs we have setter functions with docstrings and pybindings to make it easier to understand what each parameter does. It will also make it easier to set InjectionParameters if we add their setters to the VppConfig class instead of having to acces the struct itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added here: 030b0e5 Add setters and docstrings for VppConfig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Looks good, just found some typos. You can resolve this comment once they are fixed.
| // Node | ||
| vpp.def_readonly("inputConfig", &Vpp::inputConfig, DOC(dai, node, Vpp, inputConfig)) | ||
| .def_property_readonly( | ||
| "left", [](Vpp& node) { return node.left; }, py::return_value_policy::reference_internal) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add docstrings to all Inputs and bindings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done here: 844ef2f Add docstrings to python bindings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a new VPP (Virtual Projection Pattern) node to the DepthAI pipeline, enabling virtual projection pattern application to stereo images based on disparity maps.
Key changes:
- Introduces the
Vppnode with configuration capabilities for patch size, blending, coloring types, and injection parameters - Adds comprehensive test coverage for different image types (RAW8, GRAY8) and dynamic configuration updates
- Includes Python bindings for the new node and configuration datatype
Reviewed Changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
include/depthai/pipeline/node/Vpp.hpp |
Defines the Vpp node class with inputs for left/right images, disparity, and confidence maps |
include/depthai/pipeline/datatype/VppConfig.hpp |
Declares VppConfig datatype with algorithm parameters and injection settings |
include/depthai/properties/VppProperties.hpp |
Defines properties structure for Vpp node configuration |
src/pipeline/node/Vpp.cpp |
Implements Vpp node construction and internal pipeline building |
src/pipeline/datatype/VppConfig.cpp |
Provides VppConfig destructor implementation |
tests/src/ondevice_tests/vpp_test.cpp |
Adds comprehensive tests for RAW8/GRAY8 formats and multi-config scenarios |
bindings/python/src/pipeline/node/VppBindings.cpp |
Python bindings for Vpp node |
bindings/python/src/pipeline/datatype/VppConfigBindings.cpp |
Python bindings for VppConfig datatype |
| Various integration files | Updates to enums, serialization, CMake files to integrate the new node |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
42c0314 to
030b0e5
Compare
|
All comments/suggestions were resolved. |
| [](Vpp& node) { return node.confidence; }, | ||
| py::return_value_policy::reference_internal, | ||
| "Confidence of the dispatiry (in integers - 16 times bigger).") | ||
| .def_readonly("syncedInputs", &Vpp::syncedInputs, DOC(dai, node, Vpp, syncedInputs), "Synchronised Left Img, Right Img, Dispatiy and confidence input.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this compile with bindings enabled? DOC(dai, node, Vpp, syncedInputs) pulls the docstrings from the function dai::node::Vpp::syncedInputs So you can just update the docstrings there.
56d2059 to
fe677ac
Compare
fe677ac to
361c95f
Compare
Add Vpp node
For more details, see: https://gitlab.luxonis.com/luxonis/depthai/depthai-device-kb/-/merge_requests/215