Implement new vision system #33
Conversation
this is because the camera is the calibration subject in the new model, not the distance of the markers.
Adimote
changed the title
|
Just trying to get tests to pass before this is ready for review. |
| marker_id = apriltag_detection.id | ||
|
|
||
| # ************************************************************************* | ||
| # NOTE: IF YOU CHANGE THIS, THEN CHANGE ROBOT-API camera.py |
There was a problem hiding this comment.
You have changed this file, however robot-api hasn't been changed (ideally by design). Is this note necessary anymore?
There was a problem hiding this comment.
It happens to be that the changes I made here doesn't affect robot-api's version. If I were to add a new value (i.e. orientation), I'd need to add it in robot-api, so I think we still need have a need for the message
There was a problem hiding this comment.
I'm not sure that that's strictly true, and certainly nowhere near as dramatic as this comment appears.
Adding a value here makes it possible to also expose that value in robot-api. Removing a value here is (hopefully obviously) a breaking change, so we'd need to modify the consumers of the library anyway.
I'd be in favour of reducing the shoutyness of this comment. However, from what I can see the comment itself doesn't change here, so that would be a separate PR anyway.
sb_vision/vision.py
Outdated
|
|
||
| distance_model = self.camera.distance_model | ||
| camera_model = self.camera.camera_model | ||
| detections = list(self.apriltag_detector.detect_tags(img)) |
There was a problem hiding this comment.
Why are you casting this to a list, to then just iterate over it? I think keeping it as-is inside the list comprehension is much nicer
There was a problem hiding this comment.
Good catch, this was because I iterated over it twice at one point when experimenting, fixed in 5239714
tests/test_coordinates_from_file.py
Outdated
| rot_y=math.atan(-0.1), # or -5.710593137499643 degrees | ||
| dist=(0.01 + 1)**0.5, # or 1.004987562112089 | ||
| rot_y=math.atan(-0.1), | ||
| dist=(0.1 + 1) ** 0.5, |
There was a problem hiding this comment.
If the Cartesian position hasn't changed, then this too should be unchanged.
There was a problem hiding this comment.
Oops that's a miss-calculation, forgot these values are squared. I'll make it more explicit.
| polar_x=1.6704649792860642, | ||
| polar_y=1.5707963267948966, | ||
| dist=1.004987562112089, | ||
| ), |
There was a problem hiding this comment.
Why are we removing this? While it's deprecated, it's still supported. We therefore still need to actually have it so should be validating that it outputs the same values.
| # Rotation tolerance is independent of distance | ||
| ROTATION_TOLERANCE = 0.075 | ||
|
|
||
| CALIBRATIONS = Path(__file__).parent.parent / 'calibrations' / 'tecknet_25cm' |
There was a problem hiding this comment.
IMO it would be much better if we could have this still use the c270 samples (I realise that means also calibrating a c270) since we have confidence that the c270 model was correct. That would therefore make it much easier to have confidence that the new calibration mechanism is outputting consistent data.
There was a problem hiding this comment.
I don't think we should calibrate a c270 just so we don't need to change these tests. Calibrating has to be done manually using the interactive opencv script, and is thus more effort than it's worth.
tests/test_coordinates_from_file.py
Outdated
| rot_y=math.atan(-0.1), # or -5.710593137499643 degrees | ||
| dist=(0.01 + 1)**0.5, # or 1.004987562112089 | ||
| rot_y=math.atan(-0.1), | ||
| dist=(0.1 + 1) ** 0.5, |
There was a problem hiding this comment.
Why are you removing the comments here? They were present intentionally to help:
- humans reason about whether values are correct
- make it easier to understand the test failures (since the 'expected' values will be the calculated commented value, rather than the expression)
There was a problem hiding this comment.
re-added them in 5125186
tests/test_coordinates_from_file.py
Outdated
|
|
||
|
|
||
| def assertWithinTolerance(val, expected, tolerance, error_msg=""): | ||
| assert (expected - tolerance) < val < (expected + tolerance), error_msg |
There was a problem hiding this comment.
I've not used it, but you might enjoy pytest.approx
There was a problem hiding this comment.
@Adimote Out of curiosity: what are the error messages like from pytest.approx? Are they useful in helping understand the failure?
There was a problem hiding this comment.
Yes, they're actually as detailed as not using approx
sb_vision/distance_finding.py
Outdated
| return text | ||
|
|
||
|
|
||
| def get_calibration(file_name: Path) -> Dict: |
There was a problem hiding this comment.
This will (should) be failing type checking -- please can we specify the details of this Dict. If they're too complex, consider using a typing.NamedTuple instead.
There was a problem hiding this comment.
Fixed a while ago (CBA to find commit hash)
sb_vision/distance_finding.py
Outdated
| values = _get_text(element.find('data')) | ||
| if data_type == 'd': # doubles | ||
| data = np.reshape(np.array([float(v) for v in values]), | ||
| (rows, cols)) |
There was a problem hiding this comment.
What's the benefit of going into numpy here? If there's precision that we're aiming for (numpy's float64 is more precise than float is IIRC), then we should be using float64 to parse the string. Otherwise IMO we should stick to Python types here (not least as it makes the type signatures much easer to reason about).
sb_vision/distance_finding.py
Outdated
| import numpy as np | ||
|
|
||
|
|
||
| def _get_text(element: etree.Element): |
There was a problem hiding this comment.
This doesn't really return "text"; is there a better name for it?
sb_vision/distance_finding.py
Outdated
|
|
||
|
|
||
| @functools.lru_cache() | ||
| def load_camera_calibrations(file_name: Path) -> Tuple[np.ndarray, np.ndarray]: |
There was a problem hiding this comment.
Note that typing of numpy types doesn't actually work -- they're treated as Any, so if we can avoid using their array types and instead use built-in types that might be better.
sb_vision/vision.py
Outdated
|
|
||
| camera_model = self.camera.camera_model | ||
| detections = list(self.apriltag_detector.detect_tags(img)) | ||
| detections = self.apriltag_detector.detect_tags(img) |
There was a problem hiding this comment.
I think Jake's point was that we could leave this in the comprehension below entirely. This would both minimise the diff and also keep the clarity that the previous spelling had.
|
@Adimote thanks for those changes; I think this branch is now all good except the deployment plan & testing it on a Pi. My wrapping of As our usage of The combination of those allows us to ship this without any extra packages, which makes this much easier to ship! Once those PRs are merged into this one, this PR can be merged into master. |
|
@Adimote I've just spotted that this still lists some of the now-redundant dependencies in the debian package control file. We should remove those. Specifically we can remove |
|
@Adimote manual testing of this on a Pi mostly works, huzzah! There are a couple of easily fixable breakages:
Other than that, this now works well and seems to be fairly accurate :) |
If we use a calibration for a resolution other than that which it is intended, we'll get nonsense values. Ensure that that doesn't happen by requiring that `load_camera_calibrations` is passed the image resolution which will be used and then validating that against the value in the calibration file.
This merges 'new-vision-check-image-size' into new-vision. If we use a calibration for a resolution other than that which it is intended, we'll get nonsense values. Ensure that that doesn't happen by requiring that `load_camera_calibrations` is passed the image resolution which will be used and then validating that against the value in the calibration file.
Woo!
|
Now that this has been merged, is the new vision system in a state where we can load it up on the pi (after installing the libopencv/python headers) and try it out? :) |
|
@mxbi happily there aren't any new dependencies for this change, so all that's needed is to build the latest package and install it. Having done that you should find that the existing support in |
This PR is based off of #29 as it uses some images from the fixed data set in testing. The first commit you should look at is 5bebf1f .
distance_modelas the new parameter for the camera model xml file generated by opencvgame_specific.py