Fix simplify() and test it extensively #1214

BagelOrb · 2020-03-07T12:35:28Z

Better tests for the simplify() function.

It is a very important function and the tests were flawed.

EDIT:
The tests proved that something was wrong with the function, and something was wrong with earlier tests.

Therefore I had to rewrite and fix simplify()

Thanks to @smartavionics for laying down the ground work for this PR.

The previous test only considered single corners, while simplify() works cumulatively. Even though one corner might not simplify based on its current geometry, it might be simplified if the previous corner was simplified. This seems to be related to the fact that the spiral has self-intersectiong geometry. The simplification of more segments can result in a smaller error because the spiral loops back unto itself. In the new test the whole spiral is considered to be a region of small errors and the whole thing is simplifiable, except that we added unsimplifiable geometry in order to prevent the collapse of the whole region, which was another problem of the original test

BagelOrb · 2020-03-07T13:45:22Z

New circle test:
Starts out really smooth in the East and becomes every rougher in the clockwise direction.
Before simplify: black, after: red

BagelOrb · 2020-03-07T13:49:14Z

Fixed spiral test:
added far away geometry to force the algorithm to start at a specific location with simplification
black: original polygon, red: FAILING test
The inner point of the spiral has to be in the output.

BagelOrb · 2020-03-07T14:05:56Z

Sinusoidal test:
Checks whether sinusoidal errors get simplified.
Black: original, red: simplified
Note that there is an error because the long segments on the right is simplified, but this test doesn't check for that.

… simplify() simplify() is not precise and might not optimally simplify We therefore allow for twice the optimal amount of segments in the output

due to intermediate simplification, the total error should be as large as the diameter of the spiral, not the radius.

…a vertex within the loop it was only checking the previous segment, at the end of the loop it was requiring either segment instead of both

BagelOrb · 2020-03-07T16:41:27Z

Extreme high poly test.
Segments are approx 3 micron

BagelOrb · 2020-03-07T16:44:08Z

This PR now also includes a fix for the changed long line segment problems shown in figure 2 and 3 of this thread.

captures the intent of the original spiral test called simplifyLimitedError

the Y component might be quite high, so we introduce points in between two consecutive X values with an intermediate Y also we add some arbitrary long segments in there to test robustness against a mix of high poly and low poly

for debugging purposes

BagelOrb · 2020-03-09T08:25:15Z

Zig zag test which captures the intent of the original spiral test.
This test proved the implementation was faulty. It used to simplify half of the bends it should have before I fixed it.

…implementation

If consecutive points are considered the colinearity is a moving target and the removal escalates and generates rough gcode when the input model is extremely high poly.

The function loops over as many points as size(), so we don't need to handle the first point outside of the loop. The code for checking colinearity was buggy and could escalate. Now we simply interpret the function requirements as stating that the smallest_line_segment criterion ca be ignored if the local deviation is smaller than 5 micron. We therefore use the same vertex removing code for those small errors as for larger errors.

whole polygons were removed when simplify could remove all first vertices until it got to the last one. When it checked whether it could remove the last one it checked a point against itself, which meant that the area was always zero and so it would always also remove the last 2 vertices as well.

The testing criteria didn't correspond to what the documentation requires of the function. The test case used to depend too much on implementation detail.

removes duplicate code should always execute the same

test which a degenerate vertex which lies far away from the polygon, but which doesn't alter the area of the polygon, gets removed.

BagelOrb · 2020-03-10T15:18:34Z

I've completed all my tests and verified that all automated tests succeed and that the 4 linked issues are solved by this PR as well.

JIRA ticket CURA-7243 (no github issue)
Ultimaker/Cura#6604 (JIRA: CURA-6955)
#1151
Ultimaker/Cura#6215

nallath · 2020-03-30T09:27:54Z

tests/utils/PolygonTest.cpp

+    sine_polygons.simplify(length / 2, 2 * deviation);
+    if (visualize)
+    {
+        SVG svg("output/simplifySineLimitedError.svg", AABB(sine_before));


There is a lot of code duplication with writing these svg files. I think we should convert it into a re-usable function

Well. It's not like there's any complexity in the code. Nothing can go wrong. It would maybe be a bit cleaner.. Sure,

Yeah, it's a bit nitpicking, but I like my code to be clean :)

The conceptual load of reading the current code is no more, or perhaps even less than reading a new function which fits on one line. I don't think the physical length of the code should be a criterion to decide on code-cleanliness.

I think it is nitpicking. Let's not discuss. I would be okay with you fixing this, but I am overburdened atm.

nallath · 2020-03-30T09:29:36Z

src/utils/polygon.h

+     * 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


This confuses me. Does it refer to 4? (if so, it should be a part of 4) If not, I don't understand what it does refer to.

Yes it refers to 4.

nallath · 2020-03-30T09:31:45Z

tests/utils/PolygonTest.cpp

+        svg.nextLayer();
+        svg.writePolygon(zigzag, SVG::Color::RED);
+    }
+    EXPECT_THAT(zigzag.size(), testing::Eq(zigzag_before.size() - simplifiable_bend_count)) << "Should simplify bends with height 100 up to 500";


That seems a bit light of a test, doesn't it? It's now only checking if the number of polygons is correct. Should there also not be some sort of test that actually checks if the error isn't too great?

No. It's not checking zigzag_polygons, it's checking the one polygon zigzag.

Actually this test case is testing the most important thing atm. It only succeeds if exactly the number of bends which we expect to be dsimplified out actually do. No more and no less.

nallath · 2020-03-30T11:52:29Z

Rounded corners do get worse though (it's more noticeable if you switch between the two images in image viewer):

Before:

After:

You can see it at the entire model as well:
Before

After

BagelOrb · 2020-03-31T10:34:05Z

Rounded corners do get worse though

Yes. That's the whole goal of simplify().

BagelOrb · 2020-03-31T10:41:58Z

Did you check the frontend PR for this PR??

The new simplify simplifies twice as hard as the old one, so I made a frontend PR to update all simplify settings to be halved. This also includes an upgrade system to update old profiles to new profiles with half the value.

Contributes to issue CURA-7282.

Ghostkeeper · 2020-03-31T15:28:13Z

I've tested the basic functionality with a variety of organic and non-organic-but-rounded shapes and found no immediately visible concerns. The maximum resolution seems to be held in honour unless the maximum deviation is in concern. The maximum deviation seems to always be held in honour except for a minimum of 5 micrometres. Perhaps we want to set the minimum for Maximum Deviation in the front-end to 0.005mm?

I have a cylinder with 100,000 sides with a radius of 10. (this one). That results in side lengths of 0.000314mm, or approximately 0.3 micrometres, so basically just multiple vertices on every integer coordinate possible along the circle. I sliced this using a maximum resolution and maximum deviation of 0.005mm, so that the exception for "pretty much straight" doesn't get in the way. Let's take an inset of 0.175mm into account for the outer wall line width. You'd expect then that a maximum deviation of 0.005mm can lead to side lengths of up to 2 * sin(acos((9.825 - 0.005) / 9.825)) * 9.825 = 0.626817mm

Here is a snippet of g-code that I took from around the middle of the height of the cylinder (since each face is also divided diagonally into 2 triangles).

G1 X122.701 Y114.697 E88.99405
G1 X122.292 Y115.03 E88.99653
G1 X121.845 Y115.355 E88.99913
G1 X121.408 Y115.634 E89.00157
G1 X120.938 Y115.899 E89.00411
G1 X120.497 Y116.118 E89.00642

I'm seeing distances here in the ballpark of 0.5mm, which is a bit lower than I'd expect but in the correct order of magnitude.

I've also tested whether seams get removed from straight bits, which seems to work fine.

I've also sliced this model with new and old (skin and infill removed to reduce noise from things that aren't simplified):

The new algorithm has a deviation of 0.025mm rather than 0.05mm, but the resolution is the same. The old file was 12.0MB. The new g-code file is 10.5MB. This suggests that in this case the simplify algorithm removed a bit more, even with the lower deviation setting.

It's not entirely immune against leaving in vertices in a straight line. Here's an example where it chose the wrong vertices to remove. It chose the diagonals of a flat face. Parameters are increased to make it visible:

However the criterion that it needs to remove the best vertex is not in the scope of this algorithm so I think this should be accepted.

Bug CURA-7243 seems to be fixed.
Bug CURA-6729 / Ultimaker/Cura#6215 seems to be fixed.
Bug CURA-6933 / #1151 seems to be fixed.
Bug CURA-6955 / Ultimaker/Cura#6604 seems to be fixed.

In general it's pretty hard to find faults with this.

BagelOrb · 2020-03-31T15:56:48Z

Did you guys look at the frontend changes? Ultimaker/Cura#7247

Im not sure whether it's easy to port these changes to a new version upgrade system for 4.7 instead of 4.6

Ghostkeeper · 2020-04-01T07:43:48Z

Yes, I did. Like I said in my comparison:

The new algorithm has a deviation of 0.025mm rather than 0.05mm, but the resolution is the same.

I didn't apply the version upgrade in my tests, but I did use the new defaults to compare.

To change the front-end for 4.7, we'd need to change the setting version there to 13 and rename the plug-in. It's not so hard. It does change those 1500 profiles again though.

BagelOrb · 2020-04-01T13:57:23Z

Ah ok. Good!

jornada812 · 2021-01-24T15:59:48Z

I upgraded to 4.8 and re-sliced my model. It looks like the simplification changes were not made as planned by the developer. It got worse https://yadi.sk/i/oyUH92ART1Pteg

nallath · 2021-01-25T11:50:51Z

I upgraded to 4.8 and re-sliced my model. It looks like the simplification changes were not made as planned by the developer. It got worse https://yadi.sk/i/oyUH92ART1Pteg

That looks an awefull lot like coasting. Try disabling that. If that doesn't work, please create a new issue.

test simplify max introduced error

131fac5

BagelOrb mentioned this pull request Mar 7, 2020

Fix simplify #1114

Closed

BagelOrb force-pushed the simplify_test branch from 62eac6b to 1f1d9bf Compare March 7, 2020 13:32

clarify simplify function requirements

462130c

add new sinusoidal simplify test

c1596a3

BagelOrb added 7 commits March 7, 2020 15:18

make test simplifySineLimitedError more lenient for future changes to…

0f49238

… simplify() simplify() is not precise and might not optimally simplify We therefore allow for twice the optimal amount of segments in the output

lil documentation comment

908c9d1

lil documentation

188ec4f

fix test simplifyLimitedError

86e1228

due to intermediate simplification, the total error should be as large as the diameter of the spiral, not the radius.

new test simplifyHighPoly

8ac1e76

enforce smallest_line_segment_squared constraint on both segments of …

38e4930

…a vertex within the loop it was only checking the previous segment, at the end of the loop it was requiring either segment instead of both

test simplifySineHighPoly

b3cb2b1

BagelOrb added 2 commits March 7, 2020 17:42

lil: clarify code

2b2e757

ensure high poly test creates segments smaller than 5 micron

b53b870

BagelOrb changed the title ~~test simplify~~ Fix simplify() and test it extensively Mar 7, 2020

BagelOrb added 6 commits March 7, 2020 18:27

lil doc fix

fb8caa1

test simplifyIncreasingLimitedError

cc055f4

captures the intent of the original spiral test called simplifyLimitedError

make simplifySineHighPoly test extremely high poly

2a6ad5a

the Y component might be quite high, so we introduce points in between two consecutive X values with an intermediate Y also we add some arbitrary long segments in there to test robustness against a mix of high poly and low poly

make simplifyCircleHighPoly test mix of high and low poly

d59e906

option to visualize simplify tests

a7f6ae5

for debugging purposes

fix area calculation

fbdc945

BagelOrb mentioned this pull request Mar 9, 2020

Version upgrade 4.5 to 4.6 for Maximum Deviation changes Ultimaker/Cura#7247

Closed

update documentation of implementation to the original intent of the …

85bb776

…implementation

BagelOrb added 2 commits March 9, 2020 18:42

dont expect that simplify removes all nearly colinear points

debdfc8

If consecutive points are considered the colinearity is a moving target and the removal escalates and generates rough gcode when the input model is extremely high poly.

BagelOrb force-pushed the simplify_test branch from 6124bd6 to e346cbd Compare March 10, 2020 09:21

BagelOrb added 5 commits March 10, 2020 10:26

add value for used settings in SlicePhaseTest

2e1f5af

fix simplifyZigzag test

d26845d

The testing criteria didn't correspond to what the documentation requires of the function. The test case used to depend too much on implementation detail.

lil refactor in simplify

6ccc37d

removes duplicate code should always execute the same

test simplifyDegenerateVertex

e330def

test which a degenerate vertex which lies far away from the polygon, but which doesn't alter the area of the polygon, gets removed.

BagelOrb force-pushed the simplify_test branch from e346cbd to e330def Compare March 10, 2020 09:26

This was referenced Mar 17, 2020

[4.5.0] Holes in wall the same thickness as nozzle and top layers missing Ultimaker/Cura#7265

Open

Poor slicing modeling Ultimaker/Cura#7273

Open

nallath requested changes Mar 30, 2020

View reviewed changes

Ghostkeeper added 2 commits March 31, 2020 15:54

Merge branch 'master' into simplify_test

62adb39

Fix compiler warning about unused variable

b0ace0f

Contributes to issue CURA-7282.

Ghostkeeper mentioned this pull request Apr 1, 2020

Version upgrade 4.6 to 4.7 for Maximum Deviation changes Ultimaker/Cura#7385

Merged

nallath merged commit b0ace0f into master Apr 3, 2020

nallath deleted the simplify_test branch April 3, 2020 15:10

This was referenced Apr 21, 2020

[4.4.0] Cura produces way to many segements/commands and makes printer stutter Ultimaker/Cura#6748

Open

Support next to perfectly vertical wall Ultimaker/Cura#6604

Closed

rburema mentioned this pull request Sep 6, 2020

[4.7.0] Glitch Causing Slowdowns in Circular Perimeters Ultimaker/Cura#8321

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix simplify() and test it extensively #1214

Fix simplify() and test it extensively #1214

BagelOrb commented Mar 7, 2020 •

edited

Loading

BagelOrb commented Mar 7, 2020

BagelOrb commented Mar 7, 2020

BagelOrb commented Mar 7, 2020

BagelOrb commented Mar 7, 2020

BagelOrb commented Mar 7, 2020

BagelOrb commented Mar 9, 2020

BagelOrb commented Mar 10, 2020 •

edited by nallath

Loading

nallath Mar 30, 2020

BagelOrb Mar 30, 2020

nallath Mar 30, 2020

BagelOrb Mar 31, 2020

nallath Mar 30, 2020

BagelOrb Mar 31, 2020

nallath Mar 30, 2020

BagelOrb Mar 31, 2020

nallath commented Mar 30, 2020

BagelOrb commented Mar 31, 2020

BagelOrb commented Mar 31, 2020

Ghostkeeper commented Mar 31, 2020

BagelOrb commented Mar 31, 2020

Ghostkeeper commented Apr 1, 2020 •

edited

Loading

BagelOrb commented Apr 1, 2020

jornada812 commented Jan 24, 2021

nallath commented Jan 25, 2021

Fix simplify() and test it extensively #1214

Fix simplify() and test it extensively #1214

Conversation

BagelOrb commented Mar 7, 2020 • edited Loading

BagelOrb commented Mar 7, 2020

BagelOrb commented Mar 7, 2020

BagelOrb commented Mar 7, 2020

BagelOrb commented Mar 7, 2020

BagelOrb commented Mar 7, 2020

BagelOrb commented Mar 9, 2020

BagelOrb commented Mar 10, 2020 • edited by nallath 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

nallath commented Mar 30, 2020

BagelOrb commented Mar 31, 2020

BagelOrb commented Mar 31, 2020

Ghostkeeper commented Mar 31, 2020

BagelOrb commented Mar 31, 2020

Ghostkeeper commented Apr 1, 2020 • edited Loading

BagelOrb commented Apr 1, 2020

jornada812 commented Jan 24, 2021

nallath commented Jan 25, 2021

BagelOrb commented Mar 7, 2020 •

edited

Loading

BagelOrb commented Mar 10, 2020 •

edited by nallath

Loading

Ghostkeeper commented Apr 1, 2020 •

edited

Loading