ros-industrial / noether

Tool path planning and surface segmenter
115 stars 44 forks source link

Possible bug with how VTK cutter and slicer is used to create rasters in plane raster planner? #242

Closed srsidd closed 2 weeks ago

srsidd commented 1 month ago

I'm following the implementation for the planar raster planner and it looks like PCA is used to find the longest vector of the mesh, and the cut_direction is then to create rasters evenly spaced by the line_spacing_ parameter.

I have a use case where this does not seem right. The mesh I'm using is a rectangle, and I want the segments generated along the longest edge like this - image

The cut direction is being specified with a FixedDirectionGenerator. Going over the code, we generate a max_extent from the length of the principle axes. However, this seems incorrect, as this would generate a vector along X-axis (From the above pic), but the cut_direction is specifying that we generate raster segments along the Y-axis.

Tracing the code further, it seems like we loop through the number of planes and make cuts at start_loc * i * line_spacing. Considering the shape of this mesh, even a small line_spacing covers the entire mesh with a few raster segments. Why are we looping through the longest edge and trying to generate cuts / planes at locations that are outside the mesh? If I reduce my line_spacing_, the number of planes go up drastically, compared to the number of segments that cover the mesh.

To me, this seems like a bug and we should not be creating planes by longest PCA vector, but the direction specified by the cut_direction.

Would appreciate any feedback / advice on if my thought process is right here.

marip8 commented 1 month ago

I think your assessment is correct. Some additional context: the PlaneSliceRasterPlanner class existed prior to the refactor of this repository. In the previous state, the cut direction was hard-coded to always be perpendicular to the longest principal axis, so the approach of estimating the number of planes using the PCA max extent made sense. It also technically works in the current implementation, but it can be overly conservative as you noted.

When we introduced the concepts of the DirectionGenerator and OriginGenerator during the refactor, we didn't do a particularly thorough job of porting this planner to use those concepts efficiently. You'll notice the origin generator is also completely ignored in the current implementation. As such, I think the plane slice planner needs a few changes to fully support all of the features provided by the architecture refactor:

I also don't think we're using VTK very efficiently in the current implementation of this planner, but it would be a separate effort to try to improve that

srsidd commented 1 month ago

Thanks for the response @marip8 , I can look into some of these changes you mentioned but it will take me a little bit to get to.

One question that comes up right away is regarding the origin generator - How does this work? I'm assuming I should be able to get the mesh bounds from the vtkSmartPointer<vtkPolyData>& mesh_data_ object. But how does the origin get defined with respect to the mesh? Are we assuming a frame of reference of some sort?

marip8 commented 1 month ago

But how does the origin get defined with respect to the mesh? Are we assuming a frame of reference of some sort?

The frame of reference is the mesh frame. The interface for the origin generator is defined here. The idea is that given only a mesh, the user decides where the rasters should start relative to the mesh origin. Our implementations currently include creating the origin at:

marip8 commented 2 weeks ago

Closing; addressed by #255, #257. Feel free to re-open if this does not solve this issue fully