WIP: Add Axis-Aligned Bounding Box option to Plane-Slicer Planner #123

DavidMerzJr · 2020-12-14T20:30:28Z

Functionality requested by @Levi-Armstrong. I am working on a test case to demonstrate the effectiveness of this change. I would appreciate some help puzzling through the JSON serialization code associated with the config structures. (Since I modified the config structure, I will need to update the JSON serialization to keep it fully functional.)

marip8 · 2020-12-14T20:47:08Z

I would recommend not re-introducing the raster_wrt_global_axes parameter. This parameter is unnecessary because it can be implicitly represented by the values/norm of the raster_direction: if the values are all zero, the input is invalid and it cannot be used; otherwise the input is valid.

noether/tool_path_planner/src/surface_walk_raster_generator.cpp

Lines 1242 to 1243 in f250219

    
           Eigen::Vector3d cut_dir{ config_.cut_direction }; 
        
           if (!cut_dir.isApprox(Eigen::Vector3d::Zero()))

Also, I would recommend keeping the naming the same for this parameter for both the surface walk and plane slice planner. I don't really care if it's called cut_direction or raster_direction but they ought to be called the same thing since they represent the same functionality

marip8 · 2020-12-14T21:03:08Z

Actually maybe there are 2.5 valid means of determining the raster axis:

Option 1

Use the longest axis of the object-oriented bounding box (done)

Option 2

Use an axis defined in the mesh (i.e. global) coordinate system (done in the surface walk planner as of #121)

noether/tool_path_planner/src/surface_walk_raster_generator.cpp

Lines 1243 to 1249 in f250219

    
           if (!cut_dir.isApprox(Eigen::Vector3d::Zero())) 
        
           { 
        
             // Project the cut direction onto the plane of the mesh defined by the average normal at [0, 0, 0] 
        
             Eigen::Hyperplane<double, 3> plane(avg_norm, 0.0); 
        
             raster_axis = plane.projection(cut_dir).normalized() * max.norm(); 
        
             rotation_axis = avg_norm.normalized(); 
        
           }

Option 2.5

Use the longest axis of the axis-aligned bounding box (proposed in this PR)

This seems redundant with (and less flexible than) option 2. I could see a small amount of value in adding this capability (which essentially automates the selection of the raster axis), but we might think about whether it's worth adding another configuration parameter just for this. I think specifying this manually or determining the axis before calling the planner would be easy enough and wouldn't add any additional redundant or ambiguous parameters to the configuration

DavidMerzJr · 2020-12-14T21:19:27Z

I would recommend not re-introducing the raster_wrt_global_axes parameter. This parameter is unnecessary because it can be implicitly represented by the values/norm of the raster_direction: if the values are all zero, the input is invalid and it cannot be used; otherwise the input is valid.

As far as I am aware, this value never existed for this planner before. Nevertheless, it simplifies the solution to a number of issues we are experiencing on a project. I will attempt to describe below, but for now I will just say that I expect raster_wrt_global_axes to see far more use than raster_direction: it is far simpler to use, will resolve many cases wherein someone wants to change the raster direction, and still adjusts slightly if the surface changes orientation.

noether/tool_path_planner/src/surface_walk_raster_generator.cpp

Lines 1242 to 1243 in f250219

Eigen::Vector3d cut_dir{ config_.cut_direction };

if (!cut_dir.isApprox(Eigen::Vector3d::Zero()))

I have adjusted the code to use the probably-much-more stable .isApprox() from this snippet.

Also, I would recommend keeping the naming the same for this parameter for both the surface walk and plane slice planner. I don't really care if it's called cut_direction or raster_direction but they ought to be called the same thing since they represent the same functionality

~~Didn't realize this parameter existed in the surface walk planner, I can adjust to use the same name.~~

Edit: I think this parameter is actually orthogonal to the parameter in the surface walk planner; this one is perpendicular to the direction of traversal of the resulting path, and instead indicates the direction from one path to the next. Perhaps it is better to keep a distinct name.

Use the longest axis of the object-oriented bounding box (done)

Unfortunately, this method is proving somewhat unstable on nearly-rectangular surfaces. A perfectly rectangular part gets a more or less sensible direction, but a slightly-no-longer-rectangular gets odd diagonals that are proving very difficult for our robot to plan. This is why we added the raster_wrt_global_axes tag: it allows us to quickly and easily toggle to ensure we get paths aligned with world axes, which motion plan much better.

Use an axis defined in the mesh (i.e. global) coordinate system (done in the surface walk planner as of #121)

This is very close to what we actually did.

Use the longest axis of the axis-aligned bounding box

This is done over 2 because of improved response in this particular planner. Since the planes do not follow the surface at all but only blindly propagate in a constant direction, this planner really only works on nearly-planar parts. Using the longest axis of the AABB allows the user to care a little less whether the part is vertical or horizontal (which, in our use case, could change without the opportunity to come back and re-do parameters). It also more closely mirrors the behavior of the OBB/local axes case.

DavidMerzJr · 2020-12-14T21:54:51Z

I have pushed an attempt to complete the JSON serialization of the new config parameters. I would appreciate a once-over to ensure correct behavior.

I will push a test case shortly.

marip8 · 2020-12-14T22:25:32Z

I would recommend not re-introducing the raster_wrt_global_axes parameter. This parameter is unnecessary because it can be implicitly represented by the values/norm of the raster_direction: if the values are all zero, the input is invalid and it cannot be used; otherwise the input is valid.

As far as I am aware, this value never existed for this planner before.

True, but it did exist in the surface walk parameter and was removed in #121

I will just say that I expect raster_wrt_global_axes to see far more use than raster_direction: it is far simpler to use, will resolve many cases wherein someone wants to change the raster direction, and still adjusts slightly if the surface changes orientation.

I would argue that raster_wrt_global_axes is very ambiguous, especially if you are allowed to specify raster_direction (which is added but not used in this PR). What happens when:

raster_wrt_global_axes = true, raster_direction is zero
- Use axis-aligned bounding box
raster_wrt_global_axes = true, raster_direction is non-zero
- ???
raster_wrt_global_axes = false, raster_direction is zero
- Use object-oriented bounding box
raster_wrt_global_axes = false but raster_direction is non-zero
- ???

Two are ambiguous cases that have no clear indication about which parameter takes priority. This is why I am advocating for the single parameter raster_direction. Do the calculation of the desired axis beforehand (simple bounds calculation like you did in this PR), and specify only the raster_direction parameter

Edit: I think this parameter is actually orthogonal to the parameter in the surface walk planner; this one is perpendicular to the direction of traversal of the resulting path, and instead indicates the direction from one path to the next. Perhaps it is better to keep a distinct name.

I see. I think we should settle on a single parameter (either direction of the raster line or the direction of traversal) and use it the same way in both planners. I think it would be very confusing for two very similarly named parameters in two very similar planners to generate very different behaviors. IMO, a user should be able to substitute the plane slice planner for the surface walk planner (and vice-versa) with minimal parameter changes and receive essentially the same output (at least in terms of raster direction, etc.)

DavidMerzJr · 2020-12-14T23:14:47Z

True, but it did exist in the surface walk parameter and was removed in #121

I see now this is true. (I'll note that your PR removing the parameter occurred after I made this branch, so I wasn't aware it was gone.)

I would argue that raster_wrt_global_axes is very ambiguous, especially if you are allowed to specify raster_direction (which is added but not used in this PR). What happens when:

I will come back to this.

I see. I think we should settle on a single parameter (either direction of the raster line or the direction of traversal) and use it the same way in both planners. I think it would be very confusing for two very similarly named parameters in two very similar planners to generate very different behaviors.

This is, in part, why I left substantial comments in the _config.msg file. I can copy them to the header.

IMO, a user should be able to substitute the plane slice planner for the surface walk planner (and vice-versa) with minimal parameter changes and receive essentially the same output (at least in terms of raster direction, etc.)

I think this sentiment is noble and was dismayed to see that they did not actually share an interface. However, deeper investigation made me move away from the idea of a shared interface in this case. The plane slice planner is severely limited in ways the surface walker is not. On many meshes, a user switching from surface walker to plane slicer would never be able to get similar results, even with careful re-tuning of parameters.

While the surface walker turns to keep its local cutting planes locally normal to the surface, the plane slicer uses perfectly parallel planes. On almost any curved surface, the distances between rasters generated by the plane slicer can vary wildly, whereas the surface walker will produce approximately equal-spaced paths. Ironically, the plane slicer can actually work on a set of surfaces the surface walker cannot: perfectly planar surfaces. But this distinction means that for any surface that is either not perfectly planar or has curvature along more than one direction, these two planners will never be able to put out the same results, regardless of any tuning.

Again, I think it is noble to try to keep api similar and I do very much like interchangeable code. Unfortunately, apples cannot be oranges, and I think differences that meet varied requirements or highlight varied strengths is ... acceptable.

marip8 · 2020-12-15T00:15:54Z

I see. I think we should settle on a single parameter (either direction of the raster line or the direction of traversal) and use it the same way in both planners. I think it would be very confusing for two very similarly named parameters in two very similar planners to generate very different behaviors.

This is, in part, why I left substantial comments in the _config.msg file. I can copy them to the header.

If we're going to add this change, let's do our due diligence and actually implement this the same way for both planners, not just comment how they're different. I'm fine with either behavior (line direction or traversal direction); it just needs to be the same for both planners. And obviously we should document what that behavior is.

The point I'm making is that, for a reasonably flat and smooth mesh, the surface walk and plane slice methods should produce roughly the same output given the same "shared" parameters. I understand that each planner has its limitations, but their outputs shouldn't be dramatically different for a relatively flat and smooth mesh. Sharing an interface might be a nice way to enforce this, but the first and easiest step is to make sure that the parameters that they share (i.e. parameters that have the same names) result in the same type of behavior

…ents_legnths <= 0

…anner

Generate extra rasters

jrgnicho · 2021-01-19T22:56:19Z

noether_msgs/msg/PlaneSlicerRasterGeneratorConfig.msg

+float64 min_segment_size     # The minimum segment size to allow when finding intersections; small segments will be discarded
+float64 min_hole_size 	     # A path may pass over holes smaller than this, but must be broken when larger holes are encounterd.
+float64 tool_offset 	     # How far off the surface the tool needs to be
+bool generate_extra_rasters  # Whether an extra raster stroke should be added to the beginning and end of the raster pattern


This perhaps could be made to be num_of_extra_rasters so that more than one could be added if needed. Zero would be the default case

This is a good idea. thanks

jrgnicho · 2021-01-19T23:00:50Z

tool_path_planner/include/tool_path_planner/halfedge_edge_generator.h

@@ -66,6 +66,9 @@ class HalfedgeEdgeGenerator : public PathGenerator

    /**@brief point distance parameter used in conjunction with the spacing method */
    double point_dist = 0.01;
+
+    /** @brief maximum segment length */
+    double max_segment_length = 1.0;


What is this parameter used for in the Plane Slicer?

This is in the halfedge edge generator header, which is used for generating edge paths. I assume based on the description that it is the max allows edge segment size. @drchrislewis Is this correct?

Yes, this controls the maximum length of an edge segment. There are alternate parameters and methods for subdividing edges that may result in better performance, but each had corner cases that were more complex, so I went with the simplest method to get us going. For example, since exterior edges always start and end at the same point, we could simply sub-divide by some number whenever there was a circuit. It would be really nice to find the best places to subdivide rather than do it evenly. Again these are complex. One could also classify edges and horizontal-left, hor-rt, ver-up and vert-down. and break them up into 4. Once again this is complex to implement, but may be necessary for some applications/customers.

jrgnicho · 2021-01-19T23:09:26Z

tool_path_planner/include/tool_path_planner/plane_slicer_raster_generator.h

+      raster_dir["x"] = raster_direction.x();
+      raster_dir["y"] = raster_direction.y();
+      raster_dir["z"] = raster_direction.z();
+      jv["raster_direction"] = raster_dir;


I think this will work, yaml might've been a better choice as it can directly casts to std vectors and maps

jrgnicho · 2021-01-19T23:13:32Z

tool_path_planner/src/plane_slicer_raster_generator.cpp

+                                                                     // coordinate frame
+    extents.push_back(Vector3d::UnitY() * (bounds[3] - bounds[2]));  // Extent in y-direction of supplied mesh
+                                                                     // coordinate frame
+    extents.push_back(Vector3d::UnitZ() * (bounds[5] - bounds[4]));  // Extent in z-direction of supplied mesh


It might be good to call cwise.abs() to ensure that the values of extends only have positive values even though theoretically it should always be positive

jrgnicho · 2021-01-19T23:15:56Z

tool_path_planner/src/plane_slicer_raster_generator.cpp

-  Vector3d raster_dir = (rotation_offset * Vector3d::UnitY()).normalized();
+
+  // Calculate direction of raster strokes, rotated by the above-specified amount
+  Vector3d raster_dir = (rotation_offset * config_.raster_direction).normalized();


I'm not sure what kind of results you would get if your raster_direction vector has more than one non-zero component or if it is (0, 0, 1)? Perhaps we should impose some restrictions on what values the raster_dir vector can take

jrgnicho · 2021-01-19T23:17:24Z

tool_path_planner/src/plane_slicer_raster_generator.cpp

@@ -681,7 +714,15 @@ boost::optional<ToolPaths> PlaneSlicerRasterGenerator::generate()
  }

  // converting to poses msg now
-  rasters = convertToPoses(rasters_data_vec);
+  if (config_.generate_extra_rasters)


If generate_extra_rasters was an integer you could run a subsequent for loop that generates the requested number of "extra rasters"

TODO: make extra_rasters an unsigned integer, defaulted to zero, when non-zero, generate as many as requested

jrgnicho · 2021-01-19T23:17:58Z

tool_path_planner/src/surface_walk_raster_generator.cpp

+bool SurfaceWalkRasterGenerator::setConfiguration(const Config& config)
+{
+  config_ = config;
+  return true;


newer compiler seems to flag these as errors

jrgnicho · 2021-01-19T23:18:55Z

tool_path_planner/src/utilities.cpp

+  if (!test_raster_direction.isApprox(Eigen::Vector3d::Zero()))
+  {
+    config.raster_direction = test_raster_direction;
+  }


Perhaps a warning should be thrown when this validation fails

@marip8 and @jdlangs What are your thoughts? I think it may be better to throw an exception opposed to silently failing with a warning.

If we assume that we intend to remove raster_wrt_global_axes and use the fact that norm(raster_direction) == 0 implies use default alignment method, and that norm(raster_direction)>0 means to use it, then this syntax is ok.

jrgnicho · 2021-01-19T23:19:36Z

tool_path_planner/src/utilities.cpp

+{
+  auto p1 = t1.translation();
+  auto p2 = t2.translation();
+  return sqrt(pow(p1.x() - p2.x(), 2) + pow(p1.y() - p2.y(), 2) + pow(p1.z() - p2.z(), 2));


You could do (p2 - p2).norm() I think and the name of the method should be something more indicative of the kind of distance being returned.

This is an artifact born from the use of the keyword auto. When a programmer is not sure what kind of thing they are working with, they might assume it will take some extra work to dig out the data they want. I think we should remove this function, and also replace auto from nearly every for loop we have in our libraries. It helps readability and maintainability. auto should only be used when it is clear from nearby code exactly what the type is.

jrgnicho · 2021-01-19T23:21:10Z

tool_path_planner/src/utilities.cpp

+  return sqrt(pow(p1.x() - p2.x(), 2) + pow(p1.y() - p2.y(), 2) + pow(p1.z() - p2.z(), 2));
+}
+
+ToolPaths splitSegments(ToolPaths tool_paths, double max_segment_length)


What use case is there for this method?

It is used by the half-edge-edge-path generator to split an edge segment into sub-segments because the robot is rarely able to traverse a full circuit without winding up.

drchrislewis · 2021-01-20T14:08:45Z

@DavidMerzJr @marip8 This is a comment regarding the raster_wrt_global_axes, raster_direction, and raster_rotation. The interfaces should match for every rastering method. There are in fact three methods to use. 1. automatic, align raster strokes with the major axis of the geometry. 2. Same as 1, but add a rotation around the minor inertia axis of the geometry, and 3 align with some global axis. However, I believe the implementation of the axis aligned bounding box was not doing 1 correctly on the plane slicer which resulted in poor performance. See the heat_raster method for an example of how to compute the major, minor and middle axes correctly. It does this to cut the surface when no source cut is provided. When using the offline tool, it was very convenient to let the method run amok, and then adjust with the single rotate term. It would be more cumbersome to enter a raster axis. So there are really two methods, aligned with major axis, but rotated by an angle, and aligned with a global axis. If both an rotation angle and a global axis is provided, what should happen? Since global axis is likely to be seldom used, I suggest when it is non-zero, it overrides.

…objects

Edge Path Splitting by Axes

…hanges and comments

… paths for sanding

…me distance rather than using just one

…s is still a WIP.

…nterleaving rasters

…face and vertex normals from mesh either as shape_msg or pcl::Polygonmesh. Modified plane slicer to use MLS for its normal estimation.

…n in half-edge code.

Smoothing and interleaving rebased

…o that it defaults to longest side of bounding box

Fixed default raster direction

Added ability to hop inlets and peninsulas

DavidMerzJr force-pushed the feature/axis-aligned-bounding-box branch from f30d772 to 12cf52c Compare December 14, 2020 20:40

DavidMerzJr force-pushed the feature/axis-aligned-bounding-box branch from 5905069 to 8bf55c3 Compare December 17, 2020 20:49

Add Axis-Aligned Bounding Box option to Plane-Slicer Planner

812f52e

DavidMerzJr force-pushed the feature/axis-aligned-bounding-box branch from 8bf55c3 to 812f52e Compare December 17, 2020 20:50

DavidMerzJr and others added 8 commits December 17, 2020 15:03

Clang fixes and PlaneSlicerConfig Serialization

4e32857

implemented simple max_edge_segment function

4e5c022

added max_segment_length msg params

a52ac5a

fixed lenth->length typo and made splitSegments turn off for max_segm…

12181b3

…ents_legnths <= 0

added generate_extra_rasters capability to the plane slicer raster pl…

40551b3

…anner

clang format

54dffd5

Merge pull request #2 from drchrislewis/generate_extra_rasters

7dd640d

Generate extra rasters

inprogress, but rebasing

0a88246

jrgnicho reviewed Jan 19, 2021

View reviewed changes

steviedale and others added 29 commits May 12, 2021 12:55

removed test node

d845ba5

fixed fail cases

e9671a0

removed unused dependency from CMakeLists.txt

3688fa4

support for splitting ToolPath objects with multiple ToolPathSegment …

78b6fc2

…objects

Merge pull request #6 from steviedale/feature/segment_by_axes

01bec84

Edge Path Splitting by Axes

Trying to Correct Malformed Post-Filtering Meshes

ef6fa61

reinstated a heat_raster interface and made a few minor simplifying c…

49837e1

…hanges and comments

clang tidy

8cc8584

added call to smoother and a utility function to generate interleaved…

b79f539

… paths for sanding

added search_radius to service so that we average the normals over so…

6419c78

…me distance rather than using just one

fixed quite a few bugs in the plane slicer. These caused crashes. Thi…

ef05d02

…s is still a WIP.

added parameters to service call to plane slicer for smoothing, and i…

b53758d

…nterleaving rasters

updated utilities to allow normal estimation using MLS

8ea64ab

updated method for setting x-direction of pose

07e3f7f

added MLS normal estimation and other utility functions(e.g. compute …

caad679

…face and vertex normals from mesh either as shape_msg or pcl::Polygonmesh. Modified plane slicer to use MLS for its normal estimation.

removed include file no longer necessary

d6a59f1

clang, removed unnecessary and complex if(), renamed a static functio…

189eef2

…n in half-edge code.

text of error message corrected

224d6fb

Fixed extra raster_directions param that slipped through on rebase

4f0a139

Merge pull request #12 from marrts/smoothing_and_interleaving_rebase

fdbf72e

Smoothing and interleaving rebased

Fixed default raster direction to be zero vector rather than unit Y s…

74246a1

…o that it defaults to longest side of bounding box

Merge pull request #13 from marrts/fix/raster_direction_default

e41a18e

Fixed default raster direction

Reorganizing and Missing Headers

5899b4d

Sequencer: Guard against disallowed front() back()

41a65ae

Added ability to hop inlets and peninsulas

e751fc6

Clang formatting and renamed variables to be clearer

958d584

Clarified param units and better infinite loop check

f033397

Ensures monotonically decreasing toolpath sizes

6713141

Merge pull request #14 from marrts/feature/peninsula_and_inlet_hopper

0135230

Added ability to hop inlets and peninsulas

marip8 mentioned this pull request Jun 17, 2022

Plane slice planner #168

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Add Axis-Aligned Bounding Box option to Plane-Slicer Planner #123

WIP: Add Axis-Aligned Bounding Box option to Plane-Slicer Planner #123

DavidMerzJr commented Dec 14, 2020

marip8 commented Dec 14, 2020

marip8 commented Dec 14, 2020

DavidMerzJr commented Dec 14, 2020 •

edited

Loading

DavidMerzJr commented Dec 14, 2020

marip8 commented Dec 14, 2020

DavidMerzJr commented Dec 14, 2020

marip8 commented Dec 15, 2020 •

edited

Loading

jrgnicho Jan 19, 2021

drchrislewis Jan 20, 2021

jrgnicho Jan 19, 2021

Levi-Armstrong Jan 20, 2021

drchrislewis Jan 20, 2021

jrgnicho Jan 19, 2021

jrgnicho Jan 19, 2021

jrgnicho Jan 19, 2021 •

edited

Loading

jrgnicho Jan 19, 2021

drchrislewis Jan 20, 2021

jrgnicho Jan 19, 2021

drchrislewis Jan 20, 2021

jrgnicho Jan 19, 2021

Levi-Armstrong Jan 20, 2021

drchrislewis Jan 20, 2021

jrgnicho Jan 19, 2021 •

edited

Loading

drchrislewis Jan 20, 2021

jrgnicho Jan 19, 2021

drchrislewis Jan 20, 2021

drchrislewis commented Jan 20, 2021

WIP: Add Axis-Aligned Bounding Box option to Plane-Slicer Planner #123

Are you sure you want to change the base?

WIP: Add Axis-Aligned Bounding Box option to Plane-Slicer Planner #123

Conversation

DavidMerzJr commented Dec 14, 2020

marip8 commented Dec 14, 2020

marip8 commented Dec 14, 2020

Option 1

Option 2

Option 2.5

DavidMerzJr commented Dec 14, 2020 • edited Loading

DavidMerzJr commented Dec 14, 2020

marip8 commented Dec 14, 2020

DavidMerzJr commented Dec 14, 2020

marip8 commented Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrgnicho Jan 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrgnicho Jan 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drchrislewis commented Jan 20, 2021

DavidMerzJr commented Dec 14, 2020 •

edited

Loading

marip8 commented Dec 15, 2020 •

edited

Loading

jrgnicho Jan 19, 2021 •

edited

Loading

jrgnicho Jan 19, 2021 •

edited

Loading