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

Fix xy-support distance #2000

Merged
merged 14 commits into from
Jan 2, 2024
Merged

Fix xy-support distance #2000

merged 14 commits into from
Jan 2, 2024

Conversation

casperlamboo
Copy link
Contributor

@casperlamboo casperlamboo commented Dec 18, 2023

Some history

PR with some fixes regrading the xy-distance. The XY-distance was a bit broken before. The issues were introduces by a feature called varying xy-distance (https://ultimaker.atlassian.net/browse/CURA-10297). The feature was nice, in theory, but the implementation was a bit over complicated. In cura we have two xy-distances; xy-disance and minimum xy-distance. The idea was that for vertical walls we would use the xy-distance to avoid support from coming too close to the walls. However, for angled walls we would use the min xy-distance. Reason for this is that, when using the xy-distance, the support would be to far away and not properly support the walls.

This has always worked quite nicely in the past. However, there is one somewhat unexpected consequence from this behavior, and that is that when a wall transitions from being "angled" to being completely vertical the xy distance would jump from the min xy-distance to the xy-distance.

CURA-10220 drawio2

In the example this behavior is a bit odd, but quite alright. However, when we have models with "organic near straight walls" the behavior would be come unpredictable and a bit chaotic. Case in point the screen shot below. Here the xy distance is visualized.

Screenshot 2023-02-15 at 19 21 37

This is solved using the varying xy-distance feature. Here the xy-distance is gradually changes with the wall angle. So for completely vertical walls the xy-distance is used, but as the wall changes angle the xy-distance gradually transitions with this angle.

When the feature first was implement a voronoi diagram was used to calculate the distance between a point on the current layer and the closest point to this point on the next layer. As you can imagine this produced some weird results. The particular code in question is now changed in this PR by a simple for loop that for each point $p_a$ on the current layer loops through all points $p_b$ of the next layer in order to find the closes point to $p_a$.

Reviewer, please pay attention to the performance of this ticket; the algorithm in question is changed from a $O(\log(n))$ algorithm to a $O(n^2)$ algorithm. However, since voronoi performance was a bit unpredictable anyways, and the new code is much simpler I don't think this is a big issue.

xy-distance before

Screenshot 2023-12-19 at 10 10 16

xy-distance after

Screenshot 2023-12-19 at 10 04 05

casperlamboo and others added 6 commits October 16, 2023 22:28
But instead calculate for each point the closest point on the "other" layer. Unfortunatly this means a nest for loop, looping through all points of the current layer, and for each of those points looping through all points of the other layer. This could mean a decrease in performance, but code is a lot more managable.

CURA-11098
# Conflicts:
#	src/support.cpp
@casperlamboo casperlamboo marked this pull request as draft December 18, 2023 20:41
@casperlamboo casperlamboo marked this pull request as ready for review December 19, 2023 09:46
@saumyaj3 saumyaj3 self-assigned this Dec 22, 2023
src/support.cpp Show resolved Hide resolved
src/support.cpp Outdated Show resolved Hide resolved
@casperlamboo casperlamboo merged commit 86107a7 into main Jan 2, 2024
20 checks passed
@saumyaj3 saumyaj3 deleted the CURA-11098- branch January 2, 2024 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants