Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DOCS-982: Document obstacles_depth model of segmenter in vision service #1721

Conversation

sguequierre
Copy link
Contributor

  • Make a new section in segmenters docs for obstacles_depth

@viambot viambot added the safe to build This pull request is marked safe to build from a trusted zone label Aug 30, 2023
| `h_min_m` | Optional | The minumum vertical height in meters acceptable so that any obstacle strictly lower than is not deemed an obstacle by the segmenter. <br> Default: `0.0` </br> |
| `h_max_m` | Optional | The maximum vertical height in meters acceptable so that any obstacle strictly higher than is not deemed an obstacle by the segmenter. <br> Default: `1.0` </br> |
| `theta_max_deg` | Optional | The largest slope in degrees acceptable for non-obstacle surface ramps. <br> Default: `45` </br> |
| `with_geometries` | Optional | If you have configured the geometries, or, spatial orientation of the components of your robot, within the frame system. `true` is necessary to `return_pcds` properly, if `"return_pcds": "true"` <br> Example: `false` </br> |
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kharijarrett Any suggestions here on the tables are very welcome, I'm not sure if I'm phrasing properly esp. with_geometries

| `return_pcds` | Required | If you would like point clouds or a single point returned within the `GeometryInFrame` object captured by this segmenter. `true` indicates wanting point clouds. <br> Example: `"false"` </br> |
| `h_min_m` | Optional | The minimum vertical height in meters acceptable so that any obstacle strictly lower than is not deemed an obstacle by the segmenter. <br> Default: `0.0` </br> |
| `h_max_m` | Optional | The maximum vertical height in meters acceptable so that any obstacle strictly higher than is not deemed an obstacle by the segmenter. <br> Default: `1.0` </br> |
| `theta_max_deg` | Optional | The largest slope in degrees acceptable for non-obstacle surface ramps. <br> Default: `45` </br> |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| `theta_max_deg` | Optional | The largest slope in degrees acceptable for non-obstacle surface ramps. <br> Default: `45` </br> |
| `theta_max_deg` | Optional | The largest slope in degrees acceptable for non-obstacle surface ramps. <br> Default: `45` </br> |

What does acceptable mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

giving it a crack below

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, edited w khari suggestion!

Copy link
Member

@kharijarrett kharijarrett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! This looks mostly good I appreciate the added clarity about the frame system and intrinsics. The main thing left is a lil confusion about with_geometries vs. return_pcds. I hope what I said makes sense.

@@ -19,6 +19,7 @@ The types of segmenters supported are:

- [**Obstacles point cloud (`obstacles_pointcloud`)**](#configure-an-obstacles_pointcloud-segmenter): A segmenter that identifies well-separated objects above a flat plane.
- [**Object detector (`detector_3d_segmenter`)**](#configure-a-detector_3d_segmenter): This model takes 2D bounding boxes from an object detector and projects the pixels in the bounding box to points in 3D space.
- [**Obstacles depth (`obstacles_depth`)**](#configure-an-obstacles_depth-segmenter): A segmenter for depth cameras that returns a projection of a bounding box or boxes with a Pose as a vector from the depth camera lens to the center of the obstacle identified.
Copy link
Member

@kharijarrett kharijarrett Sep 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change to "... that returns the perceived obstacles as a set of 3-dimensional bounding boxes, each with a Pose as a vector.... "

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, perfect thank you!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait I'm sorry I messed up. I'm actually wrong because in some cases (no intrinsics) this isn't true. I think the most accurate thing is "A segmenter for depth cameras that returns the perceived obstacles as a set of one or more GeometriesInFrame, each with a Pose as a vector from the depth camera lens to the center of the obstacle identified."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment @sguequierre


To configure an `obstacles_depth` segmenter, first decide if you want to use the segmenter with or without intrinsic parameters.

This is determined by the value of the configuration attribute `return_pcds`:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not true.. this is actually determined by with_geometries.

return_pcds is a separate optional parameter (default false) that determines whether pointclouds should be included within the GeometryInFrame object.

So if w_g and r_p were both true, you would get 10 GeometryInFrame objects, each with a geometry and a pointcloud inside. If w_g was false but r_p was true, you'd get a single GeometryInFrame object that would have a 1-point-pointcloud and a geometry (point). When r_p is false, you get the same things just without the pointcloud. Geometry only.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok tried to update!! Sorry for the confusion


| Parameter | Inclusion | Description |
| --------- | --------- | ----------- |
| `return_pcds` | Required | If you would like point clouds or a single point returned within the `GeometryInFrame` object captured by this segmenter. `true` indicates wanting point clouds. <br> Example: `"false"` </br> |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again this is with_geometries. (See explanation above)

| `h_min_m` | Optional | The minimum vertical height in meters for an object to be considered an obstacle. <br> Default: `0.0` </br> |
| `h_max_m` | Optional | The maximum vertical height in meters at which an object is considered an obstacle. <br> Default: `1.0` </br> |
| `theta_max_deg` | Optional | The largest slope in degrees acceptable for non-obstacle surface ramps. <br> Default: `45` </br> |
| `return_pcds` | Optional | If you have configured the geometries or spatial orientation of the components of your robot, within the frame system. <br> Example: `"false"` </br> |
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kharijarrett I flipped with_geometries and return_pcds re. your feedback-- is this accurate now or still flipped up?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Whether you would like pointclouds to be included within the GeometryInFrame object captured by this segmenter."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, done/added!


| Parameter | Inclusion | Description |
| --------- | --------- | ----------- |
| `with_geometries` | Required | Whether you would like point clouds or a single point returned within the `GeometryInFrame` object captured by this segmenter. If `true`, return point clouds. <br> Example: `"false"` </br> |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Whether you would like multiple boxes (true) or a single point (false) returned within the GeometryInFrame object captured by this segmenter."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TY, sorry for the confusion. Added!

@sguequierre sguequierre requested a review from npentrel September 8, 2023 16:55
This segmenter model is for depth cameras, and is best for motion planning with transient obstacles.

Use the segmenter to identify well separated objects above a flat plane.
Configure the `with_geometries` attribute according to whether you've [configured the frame system](/services/frame-system/#configuration) to provide intrinsic parameters:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@npentrel Edited this re. your review/suggestion to combine diff sections

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't get this - you're saying here "wheter you've configured the frame system" and on 232 "start by configuring your frame system". Either you have already configured it or you start configuring it in 232. Or do you just configure part of the frame system subsequently. Also how do I choose whether I want to use a frame system. I think this should say something like "If you need to receive a point cloud, ..." "If you only need a single point, then you do not need ..."

This segmenter model is for depth cameras, and is best for motion planning with transient obstacles.

Use the segmenter to identify well separated objects above a flat plane.
Configure the `with_geometries` attribute according to whether you've [configured the frame system](/services/frame-system/#configuration) to provide intrinsic parameters:
Copy link
Contributor Author

@sguequierre sguequierre Sep 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Configure the `with_geometries` attribute according to whether you've [configured the frame system](/services/frame-system/#configuration) to provide intrinsic parameters:
First, choose whether you want to configure the `with_geometries` attribute to provide intrinsic parameters from [configuring the frame system](/services/frame-system/#configuration).
The options are as follows:

@npentrel I think I see why you're confused-- removed so it's not directing to configure-- what i mean to say is that you have to choose whether you want to or not as the beginning of the configuration process, bc this attribute should be determined based on what you have done in the past -- configured the frame system or not. Does that make any sense?

I would just remove this full section, but I think it's important to have the options listed so people can see immediately that their choice to configure frame system or not/selection for this attribute will then have this effect on what your segmenter returns

Comment on lines 224 to 232
This segmenter model is for depth cameras, and is best for motion planning with transient obstacles.

Use the segmenter to identify well separated objects above a flat plane.
Configure the `with_geometries` attribute according to whether you've [configured the frame system](/services/frame-system/#configuration) to provide intrinsic parameters:

- `true`: this segmenter will return point clouds within the `GeometryInFrame` object it captures.
- `false`: this segmenter will return a single point within the `GeometryInFrame` object it captures.

If `true`, start by [configuring your frame system](/services/frame-system/#configuration) to configure the relative spatial orientation of the components of your robot within Viam's [frame system service](/services/frame-system/).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what this needs to do is explain better what the two options are. You're not choosing whether to configure a parameter to true or false, the reader should choose what functionality they need and based on that set the parameter. Now I am not entirely certain I am capturing the functionality correctly but the way I'd structure it is like this:

Suggested change
This segmenter model is for depth cameras, and is best for motion planning with transient obstacles.
Use the segmenter to identify well separated objects above a flat plane.
Configure the `with_geometries` attribute according to whether you've [configured the frame system](/services/frame-system/#configuration) to provide intrinsic parameters:
- `true`: this segmenter will return point clouds within the `GeometryInFrame` object it captures.
- `false`: this segmenter will return a single point within the `GeometryInFrame` object it captures.
If `true`, start by [configuring your frame system](/services/frame-system/#configuration) to configure the relative spatial orientation of the components of your robot within Viam's [frame system service](/services/frame-system/).
This segmenter model is for depth cameras, and is best for motion planning with transient obstacles.
You can use the segmenter to identify well seprated objects above a flat plane or to identify a single object (?):
- To identify multiple well separated objects above a flat plane, set `with_geometries: true`.
Ensure that you have configured your camera's [`intrinsic_parameters`](https://docs.viam.com/components/camera/calibrate/).
Then, [configure your frame system](/services/frame-system/#configuration) to configure the relative spatial orientation of the components of your robot within Viam's [frame system service](/services/frame-system/).
After configuring your frame system, your camera populates its own `Properties` with the intrinsic parameters from the frame system (camera? the next line seems to indicate that?).
You can then get those parameters from your camera through the [camera API](/components/camera/#getproperties).
The segmenter now returns point clouds within the `GeometryInFrame` object it captures.
- To identify a single object above a flat plane, set `with_geometries: false`.
The segmenter now returns a single point within the `GeometryInFrame` object it captures.

I think this way it provides one choice point and only one mention of the frame system. Though I am not sure if the second one also needed the frame system - I guess that is also currently confusing: do I need to configure the frame system in both scenarios or just one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to/shouldn't have configured the frame system if false, but if you do and still set to false, that's fine, you just will get a point back anyways! Is that clear enough now? The parameter determines the behavior of the segmenter-- if you set to true and didn't configure anything I assume it will error out

@viambot
Copy link
Member

viambot commented Sep 14, 2023

Overall readability score: 54.48 (🔴 -0.01)

File Readability
segmentation.md 61.57 (🔴 -4.45)
View detailed metrics

🟢 - Shows an increase in readability
🔴 - Shows a decrease in readability

File Readability FRE GF ARI CLI DCRS
segmentation.md 61.57 50.77 10.42 13.4 11.02 6.42
  🔴 -4.45 🔴 -8.97 🔴 -0.29 🔴 -1 🔴 -0.81 🟢 +0.04

Averages:

  Readability FRE GF ARI CLI DCRS
Average 54.48 45.88 10.88 13.36 11.88 7.77
  🔴 -0.01 🔴 -0.03 🟢 +0 🟢 +0 🟢 +0 🟢 +0
View metric targets
Metric Range Ideal score
Flesch Reading Ease 100 (very easy read) to 0 (extremely difficult read) 60
Gunning Fog 6 (very easy read) to 17 (extremely difficult read) 8 or less
Auto. Read. Index 6 (very easy read) to 14 (extremely difficult read) 8 or less
Coleman Liau Index 6 (very easy read) to 17 (extremely difficult read) 8 or less
Dale-Chall Readability 4.9 (very easy read) to 9.9 (extremely difficult read) 6.9 or less

You can then get those parameters from your camera through the [camera API](/components/camera/#getproperties).
The segmenter now returns point clouds within the `GeometryInFrame` object it captures.

If you choose not to configure the frame system to provide instrinsic parameters, set `with_geometries: false`.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@npentrel Added your suggestions here and above, I think! Let me know if it still doesn't make sense and we can maybe sync up next week to get this right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm still not clear what I am identifying when with_geometries: false. On lines 208/209, we say the segmenter identifies well separated objects above a flat plane, and if we want then we should set with_geometries: true. But am I still identifying well separated objects when I have that set to false? Is the difference just that I don't get the coordinates of where those are?

As a reader I still don't fully understand why I'd choose one over the other. An example may help here.

Copy link
Collaborator

@npentrel npentrel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's check whether we can get example output for the with and without geometries options - that would probably help the reader.

This LGTM to me now, but please have another person read over this too to ensure it's understandable

@viambot
Copy link
Member

viambot commented Sep 22, 2023

You can view a rendered version of the docs from this PR at https://docs-test.viam.dev/389216a95af84d8a808c6a3997f6853bf35fdc31/public

@sguequierre sguequierre merged commit c9dc76c into viamrobotics:main Sep 22, 2023
@sguequierre sguequierre deleted the DOCS-982/document-obstacles_depth-model branch September 22, 2023 19:38
@sguequierre sguequierre restored the DOCS-982/document-obstacles_depth-model branch September 22, 2023 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to build This pull request is marked safe to build from a trusted zone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants