Fix simplify leaving way too small line segments. #1321
Conversation
|
The currently proposed code change would violate the criteria described in the documentation of this function: I see that where this function is called the two parameters smallest_line_segment and allowed_error_distance are both set to meshfix_maximum_resolution, while in the default parameters of the function signature the former is twice the latter. Perhaps we should increase the smallest_line_segment to twice the meshfix_maximum_resolution. Or perhaps we should simply bump up the Max Resolution setting for these printers. Note that the old setting values for that setting were tweaked for a broken implementation of simplify() which removed too much, so it makes sense that now that it is working again these printer definitions will have a Max Resolution setting which doesn't remove enough. |
|
Please keep me in the loop. |
There was a problem hiding this comment.
I really like this approach and I think it can be made to work!
However, this function has been revised a gazillion times, so we should be super careful and have a really keen eye. I therefore tried to think through all of the steps and have added detailed comments.
Please update code and/or comments/documentation where needed.
| if(vSize2(current - intersection_point) > allowed_error_distance_squared) | ||
| { | ||
| // The intersection is way to far away. Drop it. | ||
| continue; |
There was a problem hiding this comment.
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.
black = original; red = simplified according to this line.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Possible 4th alternative: cut a long segment so that the shortcut is as long as the allowed error distance (or smallest segment length ?)
There was a problem hiding this comment.
Current solution (A):
Leave both in (C):
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
As per suggestion of Tim CURA-7709
nallath
force-pushed
the
CURA-7709_simply_leaves_tiny_segments
branch
from
512aa10
to
b5996eb
Compare
|
The function documentation should probably change, because the following would be unsimplifiable according to spec: |
| * 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. |
There was a problem hiding this comment.
I think you should call it 'merging the 2 verts' rather than 'moving one vert'.
There was a problem hiding this comment.
It's not quite merging them. I would interpret merging as averaging the two points.
There was a problem hiding this comment.
Yeah, but it's even less 'moving one point'. Maybe just 'dissolving the middle segment'?
| { | ||
| if(length2 < 25) | ||
| { | ||
| // We're allowed to always delete segments of less than 5 micron. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
In a live talk we decided that the effort and code complexity wasnt worth it.
Likely already adressed by the compiler, but good to be safe. part of CURA-7709
Ultimaker/Cura#8321
I've run some tests with this and it seems to resolve the issue reported.
MezzerSealWithFix
MezzerSealWithoutFix
BenchyWithFix
BenchyWithoutFix
Benchy before:
Benchy after:
CURA-7709