docs: fiducial markers guide#482
Conversation
…iny correction to Camera Streamer.
SprGrf
left a comment
There was a problem hiding this comment.
Thanks! First quick review:
bec2ef4 to
0613b51
Compare
|
@SprGrf Thanks |
|
Should there be a stand alone guide to introduce the |
SprGrf
left a comment
There was a problem hiding this comment.
Nice! The backbone is here, some first comments I had while reading:
Co-authored-by: Spyros Garyfallidis <spyros@aica.tech>
Co-authored-by: Spyros Garyfallidis <spyros@aica.tech>
Co-authored-by: Spyros Garyfallidis <spyros@aica.tech>
Co-authored-by: Dominic Reber <71256590+domire8@users.noreply.github.com>
MithraGhlm
left a comment
There was a problem hiding this comment.
Upper and Lower limits for Error_correction of STag Detector
| - **Marker selection**: The name(s) of the marker(s) that we intend to be recognized. If any of these markers enters the camera frame, the `is_any_selected_marker_detected` predicate is set to **True**. This name should always be prepended with the value of `Prefix`. | ||
| - **Marker size**: Determins the side length of a square that specifies the marker in meters. | ||
| - **Library**: This is the ID number of the HD library utilized by STag markers. The allowed numbers are `[11, 13, 15, 17, 19, 21, 23]`. | ||
| - **Error correction**: |
There was a problem hiding this comment.
0 ≤ errorCorrection ≤ (libraryHD - 1) / 2 ah that is a shame that the error bounds depend on the library selection.. This is something we can't validate easily in Studio directly.
You could then open a PR on the relevant description files to set the minimum to 0 and also to set the maximum to (23 - 1) / 2 = 11. You could even improve the component implementation such that a detail ROS log is spit out when the error correction is too high for the given library.
Co-authored-by: Dominic Reber <71256590+domire8@users.noreply.github.com>
…finition enhancements.
domire8
left a comment
There was a problem hiding this comment.
Nice, this is really coming together nicely. As we enter the final stages, could you now please try to format the file fiducial-markers.md?
I am using Prettier. I also changed the print width to 120. Isn't it working as intended? |
Well no, most lines are still way longer than 120 characters |
…ration documentation.
domire8
left a comment
There was a problem hiding this comment.
I have a few last nitpicks for the fiducial markers guide.
In order to proceed here, please
- revert the changes from the ur-hardware-interface.md file, you can do that in #485
- Remove the hand-eye-calibration guide from this PR. We'll make a separate PR for it.
- Format the fiducial-markers.md file such that no lines are longer than 120 characters
I added |
Why should they be reverted? |
Unless we have actual guidelines how we want to improve style and formatting, there is no need increasing the scope of this PR to files that are unrelated to hand-eye calibration. It's good practice to keep scopes of PRs narrow. I'm not against the changes alltogether, but this is not the place to do it. A reformatting PR would be more appropriate |
Before I change anything there, do you want the existing review comments be preserved or is it acceptable to move it to a new branch and create a fresh PR? This is what I want to do: (Since the name of this PR is already
So the existing PR would be only for Then I'll open a new PR for the Is this workflow accepted? |
As you correctly commented once on the #485 (This PR contains all commits from #482, could you untangle this?), I untangled #482 from #485. So I don't understand what you mean by: increasing the scope of this PR to files that are unrelated to hand-eye calibration. It's good practice to keep scopes of PRs narrow. Since we don't have any specific guidelines yet, how about just merging the small changes and closing #485, instead of reverting it? |
|
All 4 of my previous review comments are still unresolved |
domire8
left a comment
There was a problem hiding this comment.
Just two last formatting nitpicks. Otherwise all good!
| - **Is any marker detected**: This predicate will be set to **True** if any marker is detected in the camera frame, even | ||
| though its name is not indicated in the 'Marker Selection' parameter. As you can see in the screenshot below, the | ||
| marker name specified in the `Marker selection` parameter is stag_1, but the marker recognized in the camera frame is | ||
| stag_0, yet `Is any marker detected` predicate is set to **True**. | ||
|
|
||
| <div class="text--center"> | ||
| <img src={stagMarkerNumOne} alt="Is any marker detected at all" style={{ borderRadius: "8px" }}/> | ||
| </div> | ||
|
|
||
| - **Is any selected marker detected**: If one or more marker names are indicated in the `Marker selection` parameter, | ||
| and if any of them appears in the camera frame, this predicate with be set to **True**. If the names of none of the | ||
| markers present in the camera frame is indicated as a parameter, this predicate will remain **False**. In the | ||
| screenshot below the name of the marker appearing in the camera frame matches the name indicated in the | ||
| `Marker selection` parameter. | ||
|
|
||
| <div class="text--center"> | ||
| <img src={stagMarkerNumZero} alt="Is any of the selected markers detected" style={{ borderRadius: "8px" }}/> | ||
| </div> | ||
|
|
||
| - **Is a marker bundle detected**: If a registered group of markers is detected by the camera, this parameter will be | ||
| set to **True**. Otherwise it will remain **False**. |
There was a problem hiding this comment.
| - **Is any marker detected**: This predicate will be set to **True** if any marker is detected in the camera frame, even | |
| though its name is not indicated in the 'Marker Selection' parameter. As you can see in the screenshot below, the | |
| marker name specified in the `Marker selection` parameter is stag_1, but the marker recognized in the camera frame is | |
| stag_0, yet `Is any marker detected` predicate is set to **True**. | |
| <div class="text--center"> | |
| <img src={stagMarkerNumOne} alt="Is any marker detected at all" style={{ borderRadius: "8px" }}/> | |
| </div> | |
| - **Is any selected marker detected**: If one or more marker names are indicated in the `Marker selection` parameter, | |
| and if any of them appears in the camera frame, this predicate with be set to **True**. If the names of none of the | |
| markers present in the camera frame is indicated as a parameter, this predicate will remain **False**. In the | |
| screenshot below the name of the marker appearing in the camera frame matches the name indicated in the | |
| `Marker selection` parameter. | |
| <div class="text--center"> | |
| <img src={stagMarkerNumZero} alt="Is any of the selected markers detected" style={{ borderRadius: "8px" }}/> | |
| </div> | |
| - **Is a marker bundle detected**: If a registered group of markers is detected by the camera, this parameter will be | |
| set to **True**. Otherwise it will remain **False**. | |
| - **Is any marker detected**: This predicate will be set to **True** if any marker is detected in the camera frame, even | |
| though its name is not indicated in the 'Marker Selection' parameter. As you can see in the screenshot below, the | |
| marker name specified in the `Marker selection` parameter is stag_1, but the marker recognized in the camera frame is | |
| stag_0, yet `Is any marker detected` predicate is set to **True**. | |
| <div class="text--center"> | |
| <img src={stagMarkerNumOne} alt="Is any marker detected at all" style={{ borderRadius: "8px" }}/> | |
| </div> | |
| - **Is any selected marker detected**: If one or more marker names are indicated in the `Marker selection` parameter, | |
| and if any of them appears in the camera frame, this predicate with be set to **True**. If the names of none of the | |
| markers present in the camera frame is indicated as a parameter, this predicate will remain **False**. In the | |
| screenshot below the name of the marker appearing in the camera frame matches the name indicated in the | |
| `Marker selection` parameter. | |
| <div class="text--center"> | |
| <img src={stagMarkerNumZero} alt="Is any of the selected markers detected" style={{ borderRadius: "8px" }}/> | |
| </div> | |
| - **Is a marker bundle detected**: If a registered group of markers is detected by the camera, this parameter will be | |
| set to **True**. Otherwise it will remain **False**. |
purely a formatting change here. By adding the indents before the image <div> elements, the images will have the same width as the enumeration.
There was a problem hiding this comment.
So the image being aligned with the text is the preferred format over the image being aligned with the bullet points?
Great. Got it :)
There was a problem hiding this comment.
Should other images be also shifted a tab to the right, or it only applies to those who are next to the bullet points?
There was a problem hiding this comment.
I think the ones in enumerations only is better. The main point is that the images should look like they are part of the enumeration, not that they interrupt the alignment of the enumerations.
domire8
left a comment
There was a problem hiding this comment.
Alright looks good now! Good work with this 👍
Description
Initial draft of documentation for camera frame calibration guide.
This PR closes the camera frame calibration guide #479
This PR solves the issue by adding two stand-alone guides to the documentation.
The guide for intrinsic camera calibration already exists.
One guide is about the configuration and the process of recognizing STag marker (WIP) and a second one would be dedicated to calibrating the robot.
Review guidelines
Estimated Time of Review: 10 minutes
Feedback Needed