Add marker orientations #34
Conversation
Adimote
force-pushed
the
add-orientation
branch
from
e7ccc45 to
4b46eac
Compare
|
I'm just going to add another test with an image with 45 degrees in more than one axis, otherwise this is ready for review. Obviously this won't merge until #33 is merged |
|
is it not better to have this as a PR into #33? It means not only the diff looks nicer, but seeing as it's dependant anyway, it means it can't be merged without its dependant? |
|
I would normally, but #33 is already massive |
Previously the rotation of markers was returned as 90 degrees offset from their original angle. Change the expected shape so that the expectation of markers is so that the first corner is in the top left (as is returned in AprilTags)
Adimote
force-pushed
the
add-orientation
branch
from
4b46eac to
217b7a3
Compare
|
So Github doesn't really have a good way of showing this sort of thing. I think the idea would be having some built-in way of saying "this depends on $branch, but will be merged to master once that merges" which would then block the merge (at least in the web UI) until the relevant branch had merged. For this PR, I think it makes most sense to point it at #33 until just before that merges, then to switch it to master. Unfortunately if you point a PR at another PR and then the target PR (e.g: #33) gets merged, then the second-layer PR (e.g: this one) gets closed. |
tests/test_orientation.py
Outdated
| token, = vision.snapshot() | ||
|
|
||
| def approx_ang(expected): | ||
| return approx(expected, abs=ROTATION_TOLERANCE_DEGREES) |
There was a problem hiding this comment.
Given that the expected_orientation is in radians, I was surprised that this was in degrees. Could we call the expected here expected_degrees for clarity?
tests/test_orientation.py
Outdated
| def assert_angle(angle, expected, message): | ||
| expected = approx_ang(math.degrees(expected)) | ||
| # Check both +0 and +360 so approx can cover the jump between -180 and 180 | ||
| assert math.degrees(angle) == expected or math.degrees(angle) + 360 == expected, \ |
There was a problem hiding this comment.
I'd much rather we were explicit about whether we expect -90 or 270. I don't mind which we go with, but I think the tests should validate one or the other rather than both.
|
Please review this, it's blocking a release |
README.md
Outdated
| ### Orientation | ||
|
|
||
| Rotation of the marker around its centre (relative to the observer) | ||
| - `rot_x`: rotation about the Cartesian x-axis, positive angles represent the front going upwards. |
There was a problem hiding this comment.
This differs from what's noted in robot-api. Is that intentional?
There was a problem hiding this comment.
good catch, fixed in 638580b
| # Check both +0 and +360 so approx can cover the jump between -180 and 180 | ||
| assert math.degrees(angle_radians) == expected_degrees or math.degrees(angle_radians) + 360 == expected_degrees, \ | ||
| message | ||
| if allow_wrapping: |
There was a problem hiding this comment.
Conditionals in tests are a bad idea; they usually indicate that the test is trying to test too many things and should be broken apart. Tests should be so simple that they are obviously correct.
| assert \ | ||
| math.degrees(angle_radians) == expected_degrees or \ | ||
| math.degrees(angle_radians) + 360 == expected_degrees, \ | ||
| message |
There was a problem hiding this comment.
Why do we need to do this?
There was a problem hiding this comment.
so we can allow for a small error on a value which is close to 180 or -180. it means it checks both the 180 version and the -180 version in one check.
There was a problem hiding this comment.
Right, but these are tests -- they're dealing with fixed inputs which should mean that they have fixed outputs. We should know which side the result is and just assert that.
Are you saying that we get varying results from solvePnP even for constant input data?
An alternative way of spelling this, iff it's absolutely needed, might be to declare that the tests operate in a positive-only frame and run all the values through % 360 first.
I'm still not sure I like this approach as it means we're not really checking the direct output from the API. The not-checking-the-real-value aspect could be mitigated by adding a range bounds first, perhaps like this:
assert -180 < actual_degrees < 180, \
"{} is outside the expected range (-180° to 180°)".format(angle_name)
# angle_name is one of rot_x, rot_y, etc.
# Bound to positive numbers for easier comparisons
actual_degrees %= 360
assert actual_degrees == expected_degrees, ...While I'll admit there's a bit more processing here, degrees being a modulo thing is fairly obvious to everyone since we're used to the idea that 720° is the direction same as 0°.
There was a problem hiding this comment.
I respect what you're saying but I disagree that we should check exact values. It makes the tests extremely fragile for changes higher up the pipeline (i.e. thresholding). If we want to check that the outputs don't vary, we should have a separate test
There was a problem hiding this comment.
%360 doesn't solve the problem, it shifts the barrier to be between 0 and 360
Expose the orientation of markers (their rotation relative to the camera)
This change is based on #33,
https://github.com/sourcebots/sb-vision/pull/34/files/504f4b51993b8c17cf9bc15172d4cdc6b3ac326e..4b46eac9c00b4f4043113dbd937f21918a94542b is the diff you'll want to look at for specifically the changes added in this PR