Fix crash in export and freeze in infill generation #1518
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This would theoretically increase memory usage, but in practice leads to compiler optimisations since it won't need to cast to smaller ints. Let's hope it's not a noticeable impact. This also seems to fix a rare bug where the references to these coordinates were invalidated when exporting g-code. Ints and coord_t shouldn't be passed by reference anyway. Contributes to issue CURA-8622.
This can lead to overflow errors. If we start using points with those really far coordinates, we may end up with coordinates where we will get integer overflows if we calculate distances between them. The coordinate check performed here checks if the points are within 2.1km of the origin. I think that's pretty safe for most printers. When we get printers to print cities we'll be in trouble, but we'll see about that when we get there... Such integer overflows were causing the simplify function to accept intersection points that were extremely far away, since the calculated distance to them ended up negative. These far intersection points were then included in the resulting polygon, resulting in attempts to fill polygons of literally 300km length. Not nice. Contributes to issue CURA-8622.
rburema
reviewed
Oct 29, 2021
rburema
approved these changes
Oct 29, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This fixes one suspected issue and one actual issue.
Commit 3a9307c fixes a suspected issue where the g-code export was writing uninitialised memory. I still don't know exactly why it was causing an issue since to me, the g-code coordinates looked like they were still in proper memory. However apparently not. Before fixing this, I was getting an assertion failure about coordinates being out of reasonable range (>1m from the origin). After fixing this, that error disappeared.
Aside from that, the change is also neater. We should not use references for objects that are the size of a pointer or smaller, and coord_t is 64 bits for all of our releases, which is the size of a pointer too on those platforms. And we should use coord_t, not int, for coordinates.
Commit 618a8ab is an actual fix of a serious bug in the simplify function, or perhaps more generally. The simplify function can choose to omit a line segment by introducing a new vertex in the intersection point of the two line segments around it. So rather than printing 3 line segments where the middle one is tiny, it chooses to continue in the direction of the first line segment until they intersect and can continue in a straight line to the last line segment, saving 1 vertex. The intersection point is checked to see if it's not too far away. However it was encountering an intersection point that was so far away (~300km) that calculating the distance to it ended up a negative number due to integer overflow. The distance check thought it was then actually really close by, accepting the far vertex into the resulting polygon. This caused the infill area in this model to become 300km wide, resulting in CuraEngine freezing trying to optimize millions of infill lines that filled that area.
The fix regards lines as not-intersecting if the intersection point is really far away. It'll mean that these lines are almost parallel. Pretending that they are parallel then works practically, and also prevents overflow errors like this.
Fixes Ultimaker/Cura#10505 .
Fixes CURA-8622.