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

Fix simplify leaving way too small line segments. #1321

Merged
merged 15 commits into from Sep 17, 2020
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/slicer.cpp
Expand Up @@ -1089,7 +1089,7 @@ coord_t Slicer::interpolate(const coord_t x, const coord_t x0, const coord_t x1,
{
const coord_t dx_01 = x1 - x0;
coord_t num = (y1 - y0) * (x - x0);
num += num > 0 ? dx_01 / 2 : -dx_01 / 2; // add in offset to round result
num += num > 0 ? dx_01 / 4 : -dx_01 / 4; // add in offset to round result
return y0 + num / dx_01;
}

Expand Down
80 changes: 73 additions & 7 deletions src/utils/polygon.cpp
Expand Up @@ -295,6 +295,34 @@ Polygons ConstPolygonRef::offset(int distance, ClipperLib::JoinType join_type, d
return ret;
}

bool lineLineIntersection(Point A, Point B, Point C, Point D, Point* output)
nallath marked this conversation as resolved.
Show resolved Hide resolved
{
// Line AB represented as a1x + b1y = c1
double a1 = B.Y - A.Y;
double b1 = A.X - B.X;
double c1 = a1*(A.X) + b1*(A.Y);

// Line CD represented as a2x + b2y = c2
double a2 = D.Y - C.Y;
double b2 = C.X - D.X;
double c2 = a2*(C.X)+ b2*(C.Y);

double determinant = a1 * b2 - a2 * b1;

if (determinant == 0)
{
// The lines are parallel
return false;
}
else
{
output->X = (b2 * c1 - b1 * c2) / determinant;
output->Y = (a1 * c2 - a2 * c1) / determinant;
return true;
}
}


void PolygonRef::simplify(const coord_t smallest_line_segment_squared, const coord_t allowed_error_distance_squared)
{
if (size() < 3)
Expand All @@ -309,6 +337,7 @@ void PolygonRef::simplify(const coord_t smallest_line_segment_squared, const coo

ClipperLib::Path new_path;
Point previous = path->back();
Point previous_previous = path->at(path->size() -3);
Point current = path->at(0);

/* When removing a vertex, we check the height of the triangle of the area
Expand Down Expand Up @@ -370,21 +399,58 @@ void PolygonRef::simplify(const coord_t smallest_line_segment_squared, const coo
//h^2 = L^2 / b^2 [factor the divisor]
const coord_t height_2 = area_removed_so_far * area_removed_so_far / base_length_2;
if ((height_2 <= 1 //Almost exactly colinear (barring rounding errors).
&& LinearAlg2D::getDist2FromLine(current, previous, next) <= 1) // make sure that height_2 is not small because of cancellation of positive and negative areas
|| (length2 < smallest_line_segment_squared
&& next_length2 < smallest_line_segment_squared // Segments are small
nallath marked this conversation as resolved.
Show resolved Hide resolved
&& height_2 <= allowed_error_distance_squared) // removing the vertex doesn't introduce too much error.
)
&& LinearAlg2D::getDist2FromLine(current, previous, next) <= 1)) // make sure that height_2 is not small because of cancellation of positive and negative areas
{
continue; //Remove the vertex.
continue;
}

if (length2 < smallest_line_segment_squared
&& height_2 <= allowed_error_distance_squared) // removing the vertex doesn't introduce too much error.)
{
if (next_length2 > smallest_line_segment_squared)
{
// Special case; The next line is long. If we were to remove this, it could happen that we get quite noticeable artifacts.
nallath marked this conversation as resolved.
Show resolved Hide resolved
// We should instead move this point to a location where both edges are kept and then remove the previous point that we wanted to keep.
// By taking the intersection of these two lines, we get a point that perseves the direction (so it makes the corner a bit more pointy)
Point intersection_point;
bool has_intersection = lineLineIntersection(previous_previous, previous, current, next, &intersection_point);

if (!has_intersection || LinearAlg2D::getDist2FromLine(intersection_point, previous, current) > allowed_error_distance_squared)
{
if(length2 < 25)
{
// We're allowed to always delete segments of less than 5 micron.
Copy link
Contributor

@BagelOrb BagelOrb Sep 16, 2020

Choose a reason for hiding this comment

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

Wait no this is dangerous! The whole idea of this function was to avoid escalating segment removal. The default argument allowed_error_distance is already 5mu; this whole function was built to be able to remove 5mu segments, but do it in a safe way.

The if-case and continue here should be removed.

Copy link
Member Author

@nallath nallath Sep 16, 2020

Choose a reason for hiding this comment

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

It only does it if the intersection fails. The intersection only fails if the direction of the two lines is almost exactly the same.

So it's dangerous if you don't take it into context.

The alternative is that I simply always delete the point, regardless if the length is <5mu, which is the solution I had before this.

If we don't delete these tiny points, we still get these very tiny line segments in some spots, which wreak havoc on a print.

Also note that it was (and still is) part of the criteria:
Unless the segments or the distance is smaller than the rounding error of 5 micron

Copy link
Contributor

@BagelOrb BagelOrb Sep 16, 2020

Choose a reason for hiding this comment

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

The intersection only fails if the direction of the previous and next line is almost exactly the same, but not the same as the current line segment, so it's not like the collinearity means that this case can be ignored.

The alternative is that I simply always delete the point, regardless if the length is <5mu, which is the solution I had before this.

The alternative is option C or D, not option B. That is: either leave the small segment in, or move either of the verts of the current segment along the long segment, such that the current segment becomes as long as smallest_line_segment.

If we don't delete these tiny points, we still get these very tiny line segments in some spots, which wreak havoc on a print.

That may be the case for some printers, but I'd like to challenge the idea that this is inherently a problem for any firmware. Only an abundance of tiny line segments should wreak havoc, but as long as they are connected to a long segment it shouldn't cause buffer underruns and so it shouldn't be a problem.

I don't think simplify should be made to guarantee that no small line segments exist. The function is called all over the place in the engine; if it's a gcode problem then maybe we need a separate filtering step at the gcode generation phase. For example, alter LayerPlan::addExtrusionMove to only do anything if the line segment is long enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Actually...

We are in the case where if (next_length2 > smallest_line_segment_squared) so there is no escalation problem.

Still I think it's better to adhere to the criterion that no segment longer than shortest_line_segment may have it's direction altered, which is possible with option C or D.

Copy link
Contributor

Choose a reason for hiding this comment

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

In a live talk we decided that the effort and code complexity wasnt worth it.

continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this should be a continue. Note that this code is in the case where the next segment is too long to be able to remove the vertex according to criterium 1.

image
black = original; red = simplified according to this line.

Copy link
Member Author

Choose a reason for hiding this comment

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

The distance is only too long if the lines are almost paralel. At that point, it will create an intersection that is very far away, which results in weird ass spikes being put there.

Copy link
Contributor

Choose a reason for hiding this comment

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

There are 3 alternatives: either we A) remove the current vertex, or B) we remove the current and last vertex and use the intersection point, or we C) leave both in. In this if-case you opt for A, but I am saying we should opt for C. You misinterpreted my comment to mean that we should opt for B.

Copy link
Contributor

Choose a reason for hiding this comment

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

Possible 4th alternative: cut a long segment so that the shortcut is as long as the allowed error distance (or smallest segment length ?)
image

Copy link
Member Author

Choose a reason for hiding this comment

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

Current solution (A):
CurrentSolution
Leave both in (C):
KeepBoth

Option C is retaining some of the weird points. I also think that my previous assessment is incorrect; It happens to be wrong if both of the lines are almost in the same direction; It's probably because of a rounding error that the found point is very far away.

Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed with @Ghostkeeper, we've opted to go for option E.

It's basically option C, but when the line segment is below 5 mu, we still discard it.

Copy link
Contributor

Choose a reason for hiding this comment

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

This whole function was built to remove line segments below 5mu, but to do it in a safe way. If you choose option E then we lose the safety this function should guarantee. We might introduce escalation problems again where a lot of consecutive 5mu line segments get removed and introduce a too big error.

}
// We can't find a better spot for it, but the size of the line is more than 5 micron.
// So the only thing we can do here is leave it in...
}
else
{
// New point seems like a valid one.
current = intersection_point;

// If there was a previous point added, remove it.
if(new_path.size() > 0)
{
new_path.pop_back();
}
}
}
else
{
continue; //Remove the vertex.
}
}

//Don't remove the vertex.

accumulated_area_removed = removed_area_next; // so that in the next iteration it's the area between the origin, [previous] and [current]
previous_previous = previous;
previous = current; //Note that "previous" is only updated if we don't remove the vertex.
new_path.push_back(current);
}


*path = new_path;
}

Expand Down
20 changes: 12 additions & 8 deletions src/utils/polygon.h
Expand Up @@ -449,15 +449,19 @@ class PolygonRef : public ConstPolygonRef
/*!
* Removes consecutive line segments with same orientation and changes this polygon.
*
* Removes verts which are connected to line segments which are both too small.
* Removes verts which detour from a direct line from the previous and next vert by a too small amount.
* 1. Removes verts which are connected to line segments which are too small.
* 2. Removes verts which detour from a direct line from the previous and next vert by a too small amount.
* 3. Moves a vert when a small line segment is connected to a much longer one. in order to maintain the outline of the object.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should call it 'merging the 2 verts' rather than 'moving one vert'.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not quite merging them. I would interpret merging as averaging the two points.

Copy link
Contributor

@BagelOrb BagelOrb Sep 16, 2020

Choose a reason for hiding this comment

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

Yeah, but it's even less 'moving one point'. Maybe just 'dissolving the middle segment'?

* 4. Don't remove a vert when the impact on the outline of the object is too great.
*
* Criteria:
* 1. Never remove a vertex if either of the connceted segments is larger than \p smallest_line_segment
* 2. Never remove a vertex if the distance between that vertex and the final resulting polygon would be higher than \p allowed_error_distance
* 3. Simplify uses a heuristic and doesn't neccesarily remove all removable vertices under the above criteria.
* 4. But simplify may never violate these criteria.
* 5. Unless the segments or the distance is smaller than the rounding error of 5 micron
* Note that the simplify is a best effort algorithm. It does not guarantee that no lines below the provided smallest_line_segment_squared are left.
*
* The following example (Two very long line segments (" & , respectively) that are connected by a very small line segment (i) is unsimplifable by this
* function, even though the actual area change of removing line segment i is very small. The reason for this is that in the case of long lines, even a small
* deviation from it's original direction is very noticeable in the final result, especially if the polygons above make a slightly different choice.
nallath marked this conversation as resolved.
Show resolved Hide resolved
*
* """"""""""""""""""""""""""""""""i,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,

*
* \param smallest_line_segment_squared maximal squared length of removed line segments
* \param allowed_error_distance_squared The square of the distance of the middle point to the line segment of the consecutive and previous point for which the middle point is removed
Expand Down
8 changes: 8 additions & 0 deletions tests/utils/PolygonTest.cpp
Expand Up @@ -335,6 +335,14 @@ TEST_F(PolygonTest, simplifyLimitedError)
// apply simplify iteratively for each point until nothing is simplifiable any more
spiral_polygons.simplify(10000, max_height);
}

if (visualize)
{
SVG svg("output/simplifyLimitedError.svg", AABB(spiral_before));
svg.writePolygon(spiral_before);
svg.nextLayer();
svg.writePolygon(spiral, SVG::Color::RED);
}

EXPECT_THAT(spiral.size(), testing::Eq(4)) << "Should simplify all spiral points except those connected to far away geometry.";
}
Expand Down