Add PixelMapping #251
Add PixelMapping #251
Conversation
torbsorb
requested review from
nedrebo,
eskaur and
apartridge
as code owners
| .def( | ||
| "pixel_mapping", | ||
| [](ReleasableCamera &releasableCamera, const Zivid::Settings &settings) { | ||
| return Zivid::Experimental::Calibration::pixelMapping(releasableCamera.impl(), settings); | ||
| }, | ||
| py::arg("camera"), | ||
| py::arg("settings")); |
There was a problem hiding this comment.
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.
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.
Yeah this is how we've done it before, with for example infield correction.
| @row_stride.setter | ||
| def row_stride(self, value): | ||
| self.__impl.row_stride = value |
There was a problem hiding this comment.
Do we need these setters? We dont have that in C++? Just the consturctor.
There was a problem hiding this comment.
I agree, leave it to only getters for now.
| 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) |
There was a problem hiding this comment.
We should also check the exact values that are used, IMO. To ensure that it is mapped correctly in the pybind wrapper.
| def __init__(self, row_stride=1, col_stride=1, row_offset=0, col_offset=0) -> None: | ||
| self.__impl = _zivid.PixelMapping( | ||
| row_stride, col_stride, row_offset, col_offset | ||
| ) |
There was a problem hiding this comment.
Maybe do row_stride=1, col_stride=1, row_offset=0.0, col_offset=0.0 to be consistent with the types.
| 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, col_offset=0) -> None: |
There was a problem hiding this comment.
We don't generally do type hints in this repo, so you can drop the "-> None".
torbsorb
force-pushed
the
MISC-2023-12-03-add-pixel-mapping
branch
2 times, most recently
from
1bc2593 to
4c25ea5
Compare
| camera._Camera__impl, # pylint: disable=protected-access | ||
| _to_internal_settings(settings), | ||
| ) | ||
| return PixelMapping( |
There was a problem hiding this comment.
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.
It constructs and returns zivid.experimental._pixel_mapping which is mapped to zivid.experimental.PixelMapping.
There was a problem hiding this comment.
There was a chicken and egg story here, right Eskil?
There was a problem hiding this comment.
The problem was that we really needed two constructors:
- One for the user that takes the four numbers
- One for us that takes a _zivid.PixelMapping
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.
Yeah, I'm fine with keeping this. We can find a cleaner solution in the future.
Add wrapper for `zivid.calibration.pixel_mapping()` as well as `zivid.experimental.PixelMapping`
torbsorb
force-pushed
the
MISC-2023-12-03-add-pixel-mapping
branch
from
aedd840 to
5728960
Compare
Add wrapper for
zivid.calibration.pixel_mapping()as well aszivid.experimental.PixelMapping