blodow / realtime_urdf_filter

ROS package that can filter geometry defined in URDF models from Kinect depth images. Can also preprocess data for the OpenNI tracker, to remove backgrounds, robots etc.
Other
89 stars 46 forks source link

use all geometries per link #34

Closed christian-rauch closed 3 years ago

christian-rauch commented 3 years ago

This PR touches the geometry rendering and adds the following features:

  1. allow rendering the collision geometries in place of the visual geometries (default: visual)
  2. render all geometries in a link (a link can have multiple visual or collision geometries)
  3. optional scale to change the size of the geometry dimensions (default: 1)
  4. optionally ignore some links from the mask (default: none)

I added the new parameters to the example yaml file, but they can be omitted as they use default values when not set.

christian-rauch commented 3 years ago

@blodow Interested in merging this?

blodow commented 3 years ago

@blodow Interested in merging this?

Sure, i was not sure how much of a moving target it was. I'll have a look.

christian-rauch commented 3 years ago

I am successfully using a configuration that looks like:

fixed_frame: /world
models:
- model: "robot_description"
  geometry_type: "collision" # "visual" or "collision"
  scale: 1.3
  ignore: ["head_link1", "head_link2"]
depth_distance_threshold: 0.05
show_gui: false

So I am actively using all the features.

I just pushed an unrelated small fix for the plugin path, for which I didn't want to create a new PR.

JimmyDaSilva commented 3 years ago

Hi @blodow and @christian-rauch, and sorry to "interrupt".

I am just wondering if it wouldn't be better to dilate the robot mask, instead of scaling the meshes. I guess the use of this scaling factor is to make sure there are no remaining robot parts on the depth map. Which usually comes from an inaccurate camera/robot calibration. What do you guys think?

blodow commented 3 years ago

Hi @JimmyDaSilva, no need to apologize! I was under the impression the mesh scaling was intended for fixing wrong robot models, or for unit conversion etc. I agree that for 'padding' the mesh, I don't thing that a scale factor is the right thing to do. It did strike me as odd that one would scale/convert the individual links but not their relative placement.

W.r.t padding the model, there is the depth_distance_threshold which pads in viewing direction. If a calibration residual leads to ghosting or fringing or similar (orthogonal to viewing direction), I agree with Jimmy that this should be addressed differently.

christian-rauch commented 3 years ago

I am just wondering if it wouldn't be better to dilate the robot mask, instead of scaling the meshes.

The dilation operation would only impact the x and y, but not the depth distance.

I guess the use of this scaling factor is to make sure there are no remaining robot parts on the depth map.

I am using the scaling to get rid of cables around the links. But then you maybe also cannot expect that all your CAD models are perfectly aligned with the image view. If your joint encoders are just a bit off, you can get a high FK error. Scaling the meshes and thus adding some 3D boundary around them is a simple fix for this.

It did strike me as odd that one would scale/convert the individual links but not their relative placement.

Scaling the individual links makes sense if you have loose cables around some of them. But I don't get why you also would want to change their relative placement.

blodow commented 3 years ago

If the scaling were done to e.g. convert units, you would want to scale the whole model, links and their placement.

Scaling each link separately to pad the model is only a rough approximation IMO. Imagine a UR5 or similar, say its cylinder links are 500mm long and 80mm thick (no idea). To get 3cm padding, you would need 80mm -> 140mm diameter, so a scaling factor of 1.75, which would make the link 875 mm long, no?

What would work better here is to expand the model by moving each vertex along the surface normal by some configurable amount. This could be done in the vertex shader to pad the model cleanly by a non-multiplicative, but fixed amount, say 30mm again. We could then set the depth_distance_threshold in the fragment shader to zero.

christian-rauch commented 3 years ago

Imagine a UR5 or similar, say its cylinder links are 500mm long and 80mm thick (no idea). To get 3cm padding, you would need 80mm -> 140mm diameter, so a scaling factor of 1.75, which would make the link 875 mm long, no?

If required, you could extend that scalar scale to a scale vector [x, y, z] to scale every mesh dimensions differently.

What would work better here is to expand the model by moving each vertex along the surface normal by some configurable amount.

For sure there will be other, more sophisticated, approaches to add padding. But using the scale (or a scale vector) is a straightforward approach that does not require a lot of additional processing. If it solves the problem, I don't see why a more complex method should be used. For this, I would wait for a use-case that cannot be solved with the scalar or vector scale.

JimmyDaSilva commented 3 years ago

I like the non-multiplicative padding idea @blodow, but have no idea how difficult it is to do this using the shader.

@christian-rauch is right. If scaling the model is enough for you guys. Go for it. The issue here is that it seems @blodow intended scale to be used for conversions, so requires to be applied everywhere, also on transformations. Whereas multiplicative padding, wanted by Christian, requires only mesh resize. Maybe this could be two different params? Although I don't really see the purposes of the conversion one myself

blodow commented 3 years ago

First off, let me say I'm glad that this node is helpful and people are using this node and even contributing. So thanks :)

For the sake of discussion, I would argue that the method that let's you configure a metric, meaningful value like '3cm padding' is less complex than offering a triple scalar per link that needs to be set to a scale value that must vary with link size to get the same absolute value. A large link will need a smaller scale factor than a small link to get the same padding.

Having said that, I don't particularly mind if there is a scaling value, even if it's only a single scalar. @JimmyDaSilva I didn't want to add a conversion factor, I just pointed out that I misunderstood its purpose at first as it was introduced in this PR. So I would advise we add a comment explaining that parameter, or call it per_link_scale or something to prevent others from falling in the same trap.

christian-rauch commented 3 years ago

Note, the scalar scale is applied to each link. That means that each link is scaled by the same factor. If you want to add a specific metric size of padding to every link, then this becomes much more complicated as you would need to define 3 scale values per link. In such a case, doing this in OpenGL with a configured scalar padding size (e.g. the 3cm) would be much better.

So in this case, the user-interface would be much easier as the user only has to define a single metric value, but I think the implementation would be more complex, than just using a scalar scale.

For my purpose, the single scale is sufficient. If you would like to use something else than the scale, then I could remove that commit from this PR and keep it in my own fork.

blodow commented 3 years ago

I cannot play with this right now, but something like this should work:

#version 130
out vec4 normal;
void main() {
  const float pad = 0.05;
  vec3 temp = gl_NormalMatrix * gl_Normal;
  normal = vec4 (-temp.x, temp.y, -temp.z, 1.0); /// <-- those flips look suspicious
  vec4 posPadded =  gl_Vertex + pad * normal;
  gl_Position = gl_ModelViewProjectionMatrix * posPadded;
}

Then it's a matter of making that const float pad a uniform and set it from a parameter.

christian-rauch commented 3 years ago

Do you require to have the padding done this way, for this PR to be merged? Or would you also go ahead with the scale?

In the default settings, this PR does not change the current behaviour. If you don't want the scale padding, are the other commits in this PR useful? In this case I would exclude the scaling from this PR to get at least the other things (render all links, use collision meshes, link blacklist) merged.

christian-rauch commented 3 years ago

@blodow Would you merge this PR without the scale parameter commit?

blodow commented 3 years ago

@christian-rauch I will merge this now, but I would seriously appreciate it if you could test the shader idea!