Updating distortion model description #139
Conversation
…ementation and use
Co-authored-by: John Hurliman <[email protected]>
.vscode/settings.json
Outdated
| @@ -1,7 +1,7 @@ | |||
| // -*- jsonc -*- | |||
| { | |||
| "editor.codeActionsOnSave": { | |||
| "source.fixAll.eslint": true | |||
| "source.fixAll.eslint": "explicit" | |||
There was a problem hiding this comment.
This seemed to auto-update when I ran yarn update-generated-files. I'll change it back.
package.json
Outdated
| @@ -42,7 +42,7 @@ | |||
| "eslint-plugin-import": "2.27.5", | |||
| "eslint-plugin-jest": "27.2.1", | |||
| "eslint-plugin-prettier": "4.2.1", | |||
| "jest": "29.4.3", | |||
| "jest": "^29.7.0", | |||
There was a problem hiding this comment.
is this also necessary for this change?
There was a problem hiding this comment.
Same as other comment, seemed to auto update on yarn update-generated-files. Will revert.
internal/schemas.ts
Outdated
| @@ -693,7 +693,7 @@ const CameraCalibration: FoxgloveMessageSchema = { | |||
| name: "distortion_model", | |||
| type: { type: "primitive", name: "string" }, | |||
| description: | |||
| "Name of distortion model\n\nSupported values: `plumb_bob` and `rational_polynomial`", | |||
| "Name of distortion model\n\nFoxglove supports `plumb_bob` (k1, k2, p1, p2, k3) and `rational_polynomial` (k1, k2, p1, p2, k3, k4, k5, k6) distortion based on [OpenCV's implementation](https://docs.opencv.org/2.4/modules/calib3d/doc/camera_calibration_and_3d_reconstruction.html) of the [Brown-Conrady pinhole camera model](https://en.wikipedia.org/wiki/Distortion_%28optics%29#Software_correction). This is the same [implementation used by ROS](http://docs.ros.org/en/diamondback/api/image_geometry/html/c++/pinhole__camera__model_8cpp_source.html)", | |||
There was a problem hiding this comment.
I'd drop the reference to Brown-Conrady – I don't see that anywhere in OpenCV? In fact I believe OpenCV may have come up with their own implementation for certain things – don't quite remember where I learned that so I can't cite a source, maybe Brown–Conrady is accurate.
Also nit: I don't think we say "Foxglove supports…" in other places, we just say "supported values":
There was a problem hiding this comment.
For the ROS link you might want to link to a newer version, like https://github.com/ros-perception/vision_opencv/blob/iron/image_geometry/src/pinhole_camera_model.cpp
There was a problem hiding this comment.
Updated the language to use "Supported values..." language.
Hmm interesting point w/ the model. Its a gripe of mine that they don't reference it well anywhere as far as I can tell. I know the "plumb bob" model is in fact a sub-set of the brown-conrady model but poking around it looks like you're right the rational polynomial is a deviation from it that the internet seems confused on exactly who to reference on it... I'll remove the reference and let people look at the sources.
For the ROS link, I intentionally pointed to that particular model because I believe its the one we implement. The updated one you're pointing at is what we should support when have time because it has the equidistant model as well for fisheye.
There was a problem hiding this comment.
Though none of this matters if I can never get the build system and auto-gen files to be happy lol
yarn.lock
Outdated
There was a problem hiding this comment.
revert this change and your build should pass
This reverts commit eff2d06. undoing messing with yarn.lock#
Public-Facing Changes
Updates description for distortion used in doc generation. Added details on expected parameters, underlying model, and reference implementation used. Currently this page is lacking that information.