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

Create a set of extra lines to support corners of skins above. [Ready for review+merge] #2122

Merged
merged 12 commits into from
Aug 19, 2024

Conversation

Hello1024
Copy link
Contributor

@Hello1024 Hello1024 commented Jul 20, 2024

Description

Create a set of extra lines to support skins above. Fixes Ultimaker/Cura#19388

Skins above need their boundaries supported at a minimum. A straight line needs support just at the ends. A curve needs support at various points along the curve. We find these points by finding the minimal polygon which is always within line_width of the desired support.

The strategy here is to figure out what is currently printed on this layer within the infill area by taking all currently printed lines and turning them into a giant hole-y shape.

Then figure out extra infill_lines to add to support all points that are unsupported. The extra lines will always be straight and will always go between existing infill lines. The lines are integrated into the print path for the infill so that (in most cases) there will be no travel moves.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • Added a test for this specific functionality, and also exercises the rest of the related code in FffGcodeWriter.
  • Manually inspected results of lots of combinations of infill patterns, gradual infill, infill density, Skin Edge Support.
  • Measured performance by testing very large objects with and without this feature and saw no substantial change (<3%). Did this at a few different infill densities to stress different parts.

Test Configuration:

  • Operating System: Linux, but changes are unlikely platform dependant.

Checklist:

  • My code follows the style guidelines of this project as described in UltiMaker Meta
  • I have read the Contribution guide
  • I have commented my code, particularly in hard-to-understand areas
  • I have uploaded any files required to test this change

Initial review please!

Todo Before Merge

  • Test this on a wider range of printers and configs - in a week I'll be back with more printers and will do tests on a few with different materials to verify there are no cases this feature makes print quality worse.
  • Decide if it is a suitable replacement for the Skin Expand Distance or Skin Edge Support options, or if it should be added as another setting, and if so, how should the settings interact if all are enabled. Currently this feature is always enabled, and works correctly when combined with skin Edge support and Skin Expand distance.
    • Replaced neither feature, but disabled SkinEdgeSupport by default.
  • Decide if its worth implementing some possible extensions:
    • Support more than just skins - there are places where walls also need support, for example when skins are disabled.
      • Decided not to implement.
    • Support the middles of some skins - for example, support the end of every line within the skin, rather than just the skin boundary.
      * Done! Now supports the end of straight lines for the LINES surface pattern, and corners of the CONCENTRIC surface pattern.

Sorry, something went wrong.

@github-actions github-actions bot added the PR: Community Contribution 👑 Community Contribution PR's label Jul 20, 2024
@Hello1024
Copy link
Contributor Author

Not sure whats wrong with the conan-package / conan-package-export / package-export test - but I think it unrelated to the code I added. Is it a known issue or am I doing something wrong?

@nallath
Copy link
Member

nallath commented Jul 22, 2024

I don't think that step failing is caused by anything that you did.

@Hello1024 Hello1024 changed the title Create a set of extra lines to support corners of skins above. [Initial Review Please] Create a set of extra lines to support corners of skins above. [Ready for review+merge] Jul 29, 2024
@Hello1024
Copy link
Contributor Author

Ultimaker/Cura#19446 needs to be merged first

rburema
rburema previously requested changes Aug 7, 2024
Copy link
Member

@rburema rburema left a comment

Choose a reason for hiding this comment

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

Hi!

Great to see a well documented review with unit-tests. You don't see that everyday, so kudos!

As you can see, I've made some ready-to-commit code-changes w.r.t. coding-style (mostly no-brace if/for statements).

There are also a few suggestions about optimization. I don't think I'd require those to be in but please take a look anyway.

Lastly, I've tested it and will post a few quick remarks to the front-end part as well, as they seen more relevant there.

Comment on lines 2011 to 2014
const Shape line_shape = OpenLinesSet(line).offset(line_width / 2);
for (const auto& point : points)
{
if (line_shape.inside(point))
Copy link
Member

Choose a reason for hiding this comment

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

I very much suspect calculating the distance to a line-segment would be a lot faster than an 'inside' check on a shape made by an expanded line.

Perhaps we can even just throw the lines in a LocToLineGrid, then get the closest line-segment to each point we query with, to get the minimal set of line-segments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. Fixed.

In the typical case it is only a handful of points and a handful of lines, so I'll leave it with the dirty-feeling O(n^2) algorithm rather than introduce complexity, especially since a loc2LineGrid doesn't perform well with lots of long parallel lines.

src/FffGcodeWriter.cpp Outdated Show resolved Hide resolved
src/FffGcodeWriter.cpp Outdated Show resolved Hide resolved
src/FffGcodeWriter.cpp Outdated Show resolved Hide resolved
src/FffGcodeWriter.cpp Show resolved Hide resolved
src/FffGcodeWriter.cpp Outdated Show resolved Hide resolved
src/FffGcodeWriter.cpp Outdated Show resolved Hide resolved
const int numAngles = 16;
const double angleStep = 180.0 / numAngles; // Step size between angles

for (int i = 0; i < numAngles; ++i)
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps estimating the longest line you can make into that area (so that'd probably can be approximated by the longest axis of a non-axis-aligned bounding-box) then taking the angle of that line could maybe take care of the repetition here.

(I was also thinking about threading, but that probably won't do anything, since this area is already parallelized 'upstream' as it where.)

Copy link
Contributor Author

@Hello1024 Hello1024 Aug 8, 2024

Choose a reason for hiding this comment

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

This 16 iteration loop was indeed using a reasonable amount of the runtime (under 1% of the total slice time, but still tens of milliseconds), so I have rethought. We now test 2 angles in most cases, and even in extreme cases of hundreds of points to support, we won't usually test more than ~10.

On average, the selected angles are better too, although there are ~10 additional lines of code to achieve it.

src/FffGcodeWriter.cpp Outdated Show resolved Hide resolved
src/FffGcodeWriter.cpp Outdated Show resolved Hide resolved
@Hello1024 Hello1024 requested a review from rburema August 8, 2024 18:43
@rburema rburema dismissed their stale review August 13, 2024 13:55

stale review

@HellAholic HellAholic merged commit f05cf24 into Ultimaker:main Aug 19, 2024
18 of 19 checks passed
@Hello1024 Hello1024 deleted the Hello1024-better-infill-support branch August 21, 2024 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Community Contribution 👑 Community Contribution PR's
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Smarter printing above sparse infill
4 participants