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

Alternative fix for 'encompassing hole' tree support issue #2152

Merged

Conversation

ThomasRahm
Copy link
Contributor

@ThomasRahm ThomasRahm commented Oct 22, 2024

Description

This pull request does 3 things:

  1. It reverts [CURA-12153] Fix 'encompassing hole' issue (tree support). #2145 and adds an alternative solution for what it intended.
    I think explicitly excluding holes with branches inside them from being removed is better than re-adding the branches later.
    My opinion about the fix in [CURA-12153] Fix 'encompassing hole' issue (tree support). #2145:
    In the best case if the branches are added back, there is no difference between the hole being removed or not. Worst case something breaks:
    If I understood the change there correctly it adds all branches in a hole aka negative-holes in a positive-hole, as a positive-hole, which later is then reversed and added back to the support area as a negative-hole. This should mean that if support density is larger than 0 this fix will cause confused people wondering why suddenly the area around the branches is filled with support infill, but the branch itself is not:

grafik

This is what it looks with this pull request:

grafik

(Why the branch paths are different between these two screenshots is an investigation for another day)

  1. It correctly adds the change from Fix Tree-support lines overlapping with the model #2088
    How the other solution, where I tried to solve it without an offset found its way back I do not understand. As seen by the example file given in the pull request [CURA-12153] Fix 'encompassing hole' issue (tree support). #2145 it sadly does not work and can cause inconsistent hole removal.

  2. While debugging I noticed that this function takes longer than I would have expected. I found a small change that drastically increases the performance in this test-case. While it is not related to the things above, I don't see any reason to not include it.

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

@github-actions github-actions bot added the PR: Community Contribution 👑 Community Contribution PR's label Oct 22, 2024
@ThomasRahm ThomasRahm marked this pull request as draft October 22, 2024 09:17
@ThomasRahm
Copy link
Contributor Author

The diff reads a bit confusing. It does not handle me adding an if and converting the if after it to an else if particularly well, so here a visual aid:
grafik

If the diff confuses me (who has written the code) for like 5 to 10 minutes, anyone reviewing it could also be confused by it.

@ThomasRahm ThomasRahm marked this pull request as ready for review October 22, 2024 09:35
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.

Thanks! The screenshots of the diff where helpful, it made it clear how small this fix is :-)

@HellAholic HellAholic merged commit 3602113 into Ultimaker:5.9 Oct 23, 2024
20 of 21 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