-
Notifications
You must be signed in to change notification settings - Fork 15
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
Add PixelMapping #251
Add PixelMapping #251
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -65,6 +65,7 @@ | |
infield_correction, | ||
Matrix4x4, | ||
data_model, | ||
PixelMapping, | ||
projection, | ||
ProjectedImage, | ||
presets, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
from zivid.experimental._pixel_mapping import PixelMapping |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
"""Module for experimental pixel mapping. This API may change in the future.""" | ||
|
||
import _zivid | ||
|
||
|
||
class PixelMapping: | ||
"""Pixel mapping from subsampled to full resolution. | ||
|
||
Required when mapping an index in a subsampled point cloud to e.g. a full resolution 2D image. | ||
""" | ||
|
||
def __init__(self, row_stride=1, col_stride=1, row_offset=0.0, col_offset=0.0): | ||
self.__impl = _zivid.PixelMapping( | ||
row_stride, col_stride, row_offset, col_offset | ||
) | ||
|
||
@property | ||
def row_stride(self): | ||
return self.__impl.row_stride() | ||
|
||
@property | ||
def col_stride(self): | ||
return self.__impl.col_stride() | ||
|
||
@property | ||
def row_offset(self): | ||
return self.__impl.row_offset() | ||
|
||
@property | ||
def col_offset(self): | ||
return self.__impl.col_offset() | ||
|
||
def __str__(self): | ||
return str(self.__impl) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -62,8 +62,16 @@ namespace ZividPython::Calibration | |
}, | ||
py::arg("camera"), | ||
py::arg("settings_2d")) | ||
.def("estimate_intrinsics", [](ReleasableFrame &releasableFrame) { | ||
return Zivid::Experimental::Calibration::estimateIntrinsics(releasableFrame.impl()); | ||
}); | ||
.def("estimate_intrinsics", | ||
[](ReleasableFrame &releasableFrame) { | ||
return Zivid::Experimental::Calibration::estimateIntrinsics(releasableFrame.impl()); | ||
}) | ||
.def( | ||
"pixel_mapping", | ||
[](ReleasableCamera &releasableCamera, const Zivid::Settings &settings) { | ||
return Zivid::Experimental::Calibration::pixelMapping(releasableCamera.impl(), settings); | ||
}, | ||
py::arg("camera"), | ||
py::arg("settings")); | ||
Comment on lines
+69
to
+75
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is experimental in C++, and thus should really be in a experimental namespace in Python. It seems we have done a bad mistake for the other ones abvove here by not separating Experimental to a separate namespace/module. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm I guess this is internal code only and that the public part is infact experimental. Then it looks Ok There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah this is how we've done it before, with for example infield correction. |
||
} | ||
} // namespace ZividPython::Calibration |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
#pragma once | ||
|
||
#include <Zivid/Experimental/PixelMapping.h> | ||
|
||
#include <pybind11/pybind11.h> | ||
|
||
namespace ZividPython | ||
{ | ||
void wrapClass(pybind11::class_<Zivid::Experimental::PixelMapping> pyClass) | ||
{ | ||
pyClass.def(pybind11::init(), "Initializes all values to their defaults") | ||
.def(pybind11::init<int, int, float, float>()) | ||
.def("row_stride", &Zivid::Experimental::PixelMapping::rowStride) | ||
.def("col_stride", &Zivid::Experimental::PixelMapping::colStride) | ||
.def("row_offset", &Zivid::Experimental::PixelMapping::rowOffset) | ||
.def("col_offset", &Zivid::Experimental::PixelMapping::colOffset); | ||
} | ||
} // namespace ZividPython |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
#pragma once | ||
|
||
#include <Zivid/Experimental/PixelMapping.h> | ||
|
||
#include <pybind11/pybind11.h> | ||
|
||
namespace ZividPython | ||
{ | ||
void wrapClass(pybind11::class_<Zivid::Experimental::PixelMapping> pyClass); | ||
} // namespace ZividPython |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
def test_pixel_mapping(file_camera): | ||
from zivid.experimental.calibration import pixel_mapping | ||
from zivid.settings import Settings | ||
|
||
pixel_mapping_handle = pixel_mapping( | ||
camera=file_camera, settings=Settings(acquisitions=[Settings.Acquisition()]) | ||
) | ||
assert isinstance(pixel_mapping_handle.row_stride, int) | ||
assert isinstance(pixel_mapping_handle.col_stride, int) | ||
assert isinstance(pixel_mapping_handle.row_offset, float) | ||
assert isinstance(pixel_mapping_handle.col_offset, float) | ||
Comment on lines
+8
to
+11
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should also check the exact values that are used, IMO. To ensure that it is mapped correctly in the pybind wrapper. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
assert pixel_mapping_handle.row_stride == 1 | ||
assert pixel_mapping_handle.col_stride == 1 | ||
assert pixel_mapping_handle.row_offset == 0.0 | ||
assert pixel_mapping_handle.col_offset == 0.0 | ||
|
||
blue_subsample2x2_settings = Settings() | ||
blue_subsample2x2_settings.acquisitions.append(Settings.Acquisition()) | ||
blue_subsample2x2_settings.sampling.pixel = Settings.Sampling.Pixel.blueSubsample2x2 | ||
pixel_mapping_handle = pixel_mapping( | ||
camera=file_camera, settings=blue_subsample2x2_settings | ||
) | ||
assert pixel_mapping_handle.row_stride == 2 | ||
assert pixel_mapping_handle.col_stride == 2 | ||
assert pixel_mapping_handle.row_offset == 0.0 | ||
assert pixel_mapping_handle.col_offset == 0.0 | ||
|
||
red_subsample2x2_settings = Settings() | ||
red_subsample2x2_settings.acquisitions.append(Settings.Acquisition()) | ||
red_subsample2x2_settings.sampling.pixel = Settings.Sampling.Pixel.redSubsample2x2 | ||
pixel_mapping_handle = pixel_mapping( | ||
camera=file_camera, settings=red_subsample2x2_settings | ||
) | ||
assert pixel_mapping_handle.row_stride == 2 | ||
assert pixel_mapping_handle.col_stride == 2 | ||
assert pixel_mapping_handle.row_offset == 1.0 | ||
assert pixel_mapping_handle.col_offset == 1.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.
I think it is a bit strange that we here have a
_zivid.PixelMapping
, but we don't pass that into the constructor, instead the constructor creates another_zivid.PixelMapping
.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.
It constructs and returns
zivid.experimental._pixel_mapping
which is mapped tozivid.experimental.PixelMapping
.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.
There was a chicken and egg story here, right Eskil?
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.
The problem was that we really needed two constructors:
Since Python can't really overload constructors, we opted to only have the first one.
Open to better solutions, but I think this is fine.
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.
Yeah, I'm fine with keeping this. We can find a cleaner solution in the future.