[RSDK-12054] Including distortion parameters in get_properties response#52
Conversation
SebastianMunozP
requested review from
hexbabe,
lia-viam,
oliviamiller and
vijayvuyyuru
| p.distortion_parameters.model = "BROWN_CONRADY_K6"; | ||
| break; | ||
| case OB_DISTORTION_KANNALA_BRANDT4: | ||
| p.distortion_parameters.model = "KANNALA_BRANDT4"; |
There was a problem hiding this comment.
Where are we getting these strings from? If the orbbec sdk exposes them as enums/string consts, could we import them instead?
There was a problem hiding this comment.
OrbbecSDK only expose these models as a enum, they provide a way to convert some of its enums to strings, but not for these enums in particular
| if (call_get_properties) { | ||
| VIAM_SDK_LOG(info) << "[do_command] calling get_properties"; | ||
| auto const props = get_properties(); | ||
| VIAM_SDK_LOG(info) << "[do_command] get_properties called"; | ||
| vsdk::ProtoStruct resp; | ||
| resp["supports_pcd"] = props.supports_pcd; | ||
| resp["intrinsic_parameters"] = vsdk::ProtoStruct{{"width_px", props.intrinsic_parameters.width_px}, | ||
| {"height_px", props.intrinsic_parameters.height_px}, | ||
| {"focal_x_px", props.intrinsic_parameters.focal_x_px}, | ||
| {"focal_y_px", props.intrinsic_parameters.focal_y_px}, | ||
| {"center_x_px", props.intrinsic_parameters.center_x_px}, | ||
| {"center_y_px", props.intrinsic_parameters.center_y_px}}; | ||
| vsdk::ProtoStruct distortion_params; | ||
| distortion_params["model"] = props.distortion_parameters.model; | ||
| std::vector<viam::sdk::ProtoValue> proto_params; | ||
| for (const auto& val : props.distortion_parameters.parameters) { | ||
| proto_params.emplace_back(val); // Each double becomes a ProtoValue | ||
| } | ||
| distortion_params["parameters"] = viam::sdk::ProtoValue(proto_params); | ||
| resp["distortion_parameters"] = distortion_params; | ||
| VIAM_SDK_LOG(info) << "[do_command] get_properties returning"; | ||
| return resp; | ||
| } |
There was a problem hiding this comment.
I'm confused. Why do we need this as a docommand route if get_properties exists as a method?
There was a problem hiding this comment.
I just wanted a easy way to iterate on calling get_properties and verify its output. I.e. that I could manually call it for these purposes.
We may not be interested in keeping this, in which case I can just delete it.
src/module/orbbec.cpp
Outdated
| p.distortion_parameters.parameters = std::vector<double>{distortion_props.k1, | ||
| distortion_props.k2, | ||
| distortion_props.k3, | ||
| distortion_props.k4, | ||
| distortion_props.k5, | ||
| distortion_props.k6, | ||
| distortion_props.p1, | ||
| distortion_props.p2}; |
There was a problem hiding this comment.
Let's change the order these are returned in to k1, k2, p1, p2, k3, k4, k5, k6.
This is more in line with opencv standards https://docs.opencv.org/4.x/dc/dbb/tutorial_py_calibration.html
There was a problem hiding this comment.
Also do we need to update the README to document which coefficient each element of this array represents, so users don't have to dive into code to find out? I know this is a high prio ask but may be nice if we have time
There was a problem hiding this comment.
Absolutely, this makes sense
2 participants
This is needed for the openCV version of the FrameCalibration module.
But in general, we should always be populating this data.
Note that the distortion parameters, as well as the intrinsic ones, have names, but the distortion_parameters interface only accepts a vector of doubles, so we must make an implicit agreement on what each value means, this should definitely be fixed, but it's not the scope of this PR
Tested adding a debug do_command, we can keep it if we think it's valuable to have these kinds of commands, or I can remove it
Input:
Output: