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

Improve handling of small areas #2089

Merged
merged 1 commit into from
Jun 21, 2024

Conversation

ThomasRahm
Copy link
Contributor

Description

I looked into the issue Ultimaker/Cura#18397 and would be interested in feedback regarding the following opinions:

  1. WallToolPaths::removeSmallLines should not remove outer walls. By my understanding it was added to prevent very small lines filling holes. An outer wall will never fill a hole.
  2. In SkeletalTrapezoidation there is no functional difference between a single local maxima or multiple very close to each other. If the resulting outer wall path is so short that it not even reaches its own width, maybe it would be better to combine all these local maxima into a single one and handle it accordingly.

I am not convinced that this is the correct way to fix it, as I think arachne should not even generate such to small paths, even if the graph consists of multiple local maxima close to each other. I just want to highlight one possible solution to avoid completely missing areas.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

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

@rburema
Copy link
Member

rburema commented Jun 12, 2024

Hi @ThomasRahm ! --

What a coincidence, we where just about to address this issue ourselves (given the excellent writeup @casperlamboo made, it seemed at least doable...)

In any case, thanks for another contribution :-)

I am not convinced that this is the correct way to fix it, as I think arachne should not even generate such to small paths, even if the graph consists of multiple local maxima close to each other.

That's debatable. In any case, fixing that would take a lot of time and/or introduce a lot more uncertainty -- not something we can handle with the currently (hopefully temporarily) reduced Cura-'core'-team (or at least not without pausing a lot of other things for a long time).

Even if this can be considered a hack, it's built on top of an existing one that's already in the code. As such, I think it does a lot more good than harm, given the results I'm seeing.

I've just made a few very small cosmetic changes, and I think I can just send this off as the current bugfix for this particular issue. (One of the very few things still bothering me slightly, is that the circle doesn't adhere to the original shape, but the solution in place before didn't do that either...)

@HellAholic HellAholic merged commit aa56dbe into Ultimaker:main Jun 21, 2024
17 of 18 checks passed
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.

3 participants