-
-
Notifications
You must be signed in to change notification settings - Fork 879
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
Use Arachne to vary line width #1210
Conversation
Can we move these files into a separate folder? There are a lot of beading strategy classes in the main source folder now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't get nearly as far as I hoped. It's tough to review with so little documentation. So I'll continue tomorrow.
At the moment this branch doesn't compile due to a billion missing imports.
src/SkeletalTrapezoidation.cpp
Outdated
continue; | ||
} | ||
|
||
// copy start to end edge to graph |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
? Sentence? Are we copying twice, or...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test reply! If github works properly you should see his!
src/SkeletalTrapezoidation.cpp
Outdated
node_t* starting_node = vd_node_to_he_node[starting_vd_edge->vertex0()]; | ||
starting_node->data.distance_to_boundary = 0; | ||
// starting_edge->prev = nullptr; | ||
// starting_edge->from->data.distance_to_boundary = 0; // TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, the library is not finished yet!
src/SkeletalTrapezoidation.cpp
Outdated
edge_locator.emplace(&*edge_it, edge_it); | ||
for (auto node_it = graph.nodes.begin(); node_it != graph.nodes.end(); ++node_it ) | ||
node_locator.emplace(&*node_it, node_it ); | ||
auto safelyRemoveEdge = [this, &edge_locator](edge_t* to_be_removed, std::list<edge_t>::iterator& current_edge_it, bool& edge_it_is_updated) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider making this a normal function of the HalfEdgeGraph
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like i said for the other comment, this one works on the edge_locator
collection defined in the function rather than the graph itself.
Moving the edge_locator
to the edge class is a bit overkill imho, since it only occurs within this function (the surrounding one, not the closure).
src/SkeletalTrapezoidation.cpp
Outdated
|
||
for (auto edge_it = graph.edges.begin(); edge_it != graph.edges.end();) | ||
{ | ||
if (edge_it->prev) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document: This is not the start of a quad then, right?
src/SkeletalTrapezoidation.cpp
Outdated
bool quad_mid_is_removed = false; | ||
if (quad_mid && should_collapse(quad_mid->from, quad_mid->to)) | ||
{ | ||
assert(quad_mid->twin); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean... This will happen by itself on line 647 😛
src/utils/polygon.cpp
Outdated
Point c = poly[(i + 1) % poly.size()]; | ||
if (LinearAlg2D::pointIsLeftOfLine(b, a, c) >= 0) | ||
{ | ||
poly.remove(i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to copy the data to a separate polygon and then std::swap
for better theoretical performance and simplification of this code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've implemented this now.
So here are the broad strokes of things that still need to happen for this feature before we can start testing:
|
b5c9fed
to
fe8551f
Compare
src/utils/HalfEdgeNode.h
Outdated
@@ -48,6 +48,21 @@ class HalfEdgeNode | |||
} | |||
return odd_path_count > 2; | |||
} | |||
|
|||
bool isMarked() const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proposal: Merge isMultiIntersection
and isMarked
into a single function that checks whether the isMarked
count of going around an edge is larger-or-equal than '2 or 1, respectively', which can be the argument.
(We could also muck around with writing a sort of map-around-the-vertex function that takes a closure as argument, but that's probably overkill.) (Anyway, at least isMultiIntersection
can also have an early-out and then return false just like isMarked
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are the comments that have not yet been resolved but were marked as "outdated" by Github due to code being changed or moved around it, except some more documentation comments that I didn't bother to mention.
src/SkeletalTrapezoidation.h
Outdated
* \param going_up Whether we are traveling in the upward direction as seen from the \p origin_transition. If this doesn't align with the direction according to the R diff on a consecutive edge we know there was a local optimum | ||
* \return whether the origin transition should be dissolved | ||
*/ | ||
std::list<TransitionMidRef> dissolveNearbyTransitions(edge_t* edge_to_start, TransitionMiddle& origin_transition, coord_t traveled_dist, coord_t max_dist, bool going_up, std::unordered_map<edge_t*, std::list<TransitionMiddle>>& edge_to_transitions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use std::vector
rather than std::list
, at least for the return type. Vector is going to give us much more performance, and these transitions are going to be quite common. See my previous comment here: #1210 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... except that he does a splice
on at least one of the results, which is available for list
s only (would be inefficient for vector
s).
to_be_dissolved.splice(to_be_dissolved.end(), to_be_dissolved_here); // Transfer to_be_dissolved_here into to_be_dissolved
src/SkeletalTrapezoidationGraph.cpp
Outdated
|
||
void SkeletalTrapezoidationGraph::makeRib(edge_t*& prev_edge, Point start_source_point, Point end_source_point, bool is_next_to_start_or_end) | ||
{ | ||
Point p = LinearAlg2D::getClosestOnLineSegment(prev_edge->to->p, start_source_point, end_source_point); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understood correctly, we can use getClosestOnLine
here, rather than getClosestOnLineSegment
. We've already checked bounds of the projection so we don't need to do that again. See my previous comment here: #1210 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the sense that it wouldn't make much sense to have a rib that is not connected to the line segment (partially) defining it, and we have already committed to making the rib here, right?
Is it possible to have a description and some diagrams to show how this PR is intended to work, so that we can comment on the Pros and Cons of the intended solution - which might help to get to a great solution quicker or might be simply an annoying distraction? |
@Sophist-UK The closest thing to an explanation right now is probably this paper which also includes figures and diagrams -- the initial work and lions' share for this PR was done by the first author. |
[CURA-8354](redux) unit-tests for 'Fix width adjust simplify'
This reverts commit 8695be6. That commit was making the wrong assumption that holes are not visible from the outside. They are. And there might be other shapes inside of the hole, such as nested rings. Internal holes can have any shape, and don't necessarily need to be convex. They can have protruding parts that need to be avoided. We need to treat inside walls just the same way as outside walls. Contributes to issue CURA-8117.
Conflicts: src/skin.cpp -> Code changed in an area that was removed. src/utils/polygon.h -> Include statements changed while we added one for Arachne nearby.
This is to fix the case, where: - We are travelling from one part to another. - The start (or end) part is not convex. - The start (or end) part is so thin that traversing it through the optimal combing boundary (far from the walls) is not possible, but traversing through the minimum combing boundary (slightly wider, but close to the outer wall) is possible. In this case, what happened is that it was trying to find the spot inside the starting part closest to the destination and would try to comb there. But because the part is so thin, it failed to find a comb path with the optimal combing boundary. Instead, it would just not add a combing path. It would then move outside on the starting point which is nearby a hole in the model. The hole is closed, so there is no travel path through air to the destination part, so that will just take a straight line to the destination part. This straight line intersects multiple times with the starting part, which could have been avoided. This solution causes the combing to try the minimum combing boundary if the optimal combing boundary fails. Previously this was only used for the case where we're travelling within the same part. Now it'll also be used to prepare travelling to different parts. The CPU time needed for this is only bigger if this case actually occurs. For normal, sufficiently thick parts there should be no increase in slicing time. Contributes to issue CURA-8117.
Rather than the minimum ones that were used to calculate the crossing with. Contributes to issue CURA-8117.
Forgot to update this one in the case of the end part. Contributes to issue CURA-8117.
…imum_bound If combing inside fails, allow combing through minimum bound
Turns out that we could still use the majority of the spreadDotsAera that T. wrote for Lightening fill. My bad! Co-authored-by: BagelOrb <t.kuipers@ultimaker.com>
Conflicts: src/FffPolygonGenerator.cpp -> Maximum layer count fix src/InsetOrderOptimizer.cpp -> Fix for seam position which is no longer located here. src/pathPlanning/Comb.cpp -> Several combing fixes discarded because combing is pretty much revamped in Arachne. src/skin.cpp -> Gap filling area fix, but gap filling is removed in Arachne. src/utils/PolygonProximityLinker.cpp -> Wall overlap compensation, but removed in Arachne. src/utils/algorithm.h -> Headers rearranged. src/utils/math.h -> Addition of some helper functions changed headers. src/utils/polygonUtils.h -> Code style
As discussed during the eCCB 12/11/21 this factor makes more sense as a Ratio.
Instead of the minimum. So previously a value of 2 would make it spread the error over 2 walls for an even number, or 3 for an odd. Now it makes it spread the error over 2 walls for an even number or 1 for an odd. This means that the minimum allowed value becomes 2, because for an even wall count it needs to place the error somewhere. Done as a 5 minute fix.
Ligning infill was first generated, then applied to the part. However, the enitre fill would be applied to each part. If a model consisted of more than one part, the lightning paths would be retraced for each part of the model in that layer. Now it limits the output to the outlines that are currently processed. It's relatively efficient, since it only has to check one node of each tree against the outlines of a single part. fixes Cura/#10793
Now the length is measured along the branches, which is also good. Co-authored-by: Ghostkeeper <Ghostkeeper@users.noreply.github.com>
part of CURA-8702
Use functions that already exist instead of implementing new ones that mess up. part of CURA-8702
This reverts commit 55d0e58. That commit was done as an unsure 'well if it can't harm' kind of way in order to _maybe_ halfway fix a bug that wasn't possibly wasn't affecting our prints and now appears to have been fixed in other ways, as I can't even reproduce it anymore. However, the now reverted fix may have casued the issue in CURA-8713 -- retracts in gcode preventing the UM2+ from finishing prints.
Is not often that one gets to merge 1377 commits in one push! |
For now, this is just a WIP comparison to be able to offer review comments on libArachne.
Do not merge!
See CURA-7235.