Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fix simplify leaving way too small line segments. #1321
Changes from all commits
d7a37e2
979b303
80b472c
e75bbf3
b5996eb
22fafe2
c9a8acd
c350b38
600055f
e749149
3fcf1e6
59030d0
d24f55d
ad750ee
5fe2564
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 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
.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, alterLayerPlan::addExtrusionMove
to only do anything if the line segment is long enough.There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.black = original; red = simplified according to this line.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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'.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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'?