CURA-13250 chamfered tree base#2346
Conversation
CURA-13250 The approximateConvexHull function relied on doing a very large outer offset, then reverting it back, to get a convex-hull like rounded polygon. However, this method seemed to produce self-intersecting polygons in some cases, which propagated to doing non-closed skirt lines. We now use the actual convex hull calculation, which also happens to be much faster. In practice, that means the following changes: * Skirt is less rounded, which should actually be preferable because it uses less material and should help adhesion by doing more long straight lines * Objects far from each other (usually prime tower) are now included in the same convex skirt, where they could be disjoint with a weird shape before
Test Results31 tests 31 ✅ 5s ⏱️ Results for commit c9fc367. ♻️ This comment has been updated with latest results. |
Erwan MATHIEU (wawanbreton)
left a comment
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'C++ Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 1c13d54 | Previous: 5a53fc2 | Ratio |
|---|---|---|---|
SimplifyTestFixture/simplify_slot_noplugin |
4.2190767982101764 ns/iter |
2.5989174745839803 ns/iter |
1.62 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: Jelle Spijker (@jellespijker) Erwan MATHIEU (@wawanbreton) Casper Lamboo (@casperlamboo) HellAholic
Casper Lamboo (casperlamboo)
left a comment
There was a problem hiding this comment.
I don;t fully understand the need of changing x.getInfillArea() to x.outline_
| return index.value; | ||
| } | ||
|
|
||
| constexpr LayerIndex distance(const LayerIndex& lhs, const LayerIndex& rhs) |
There was a problem hiding this comment.
This function feels a bit weird to me since
- distance in this case can be negative
- distance returns an
Indextype
A better name for this function would belayerDelta,layersBetweenorlayersToin my opinion.
There was a problem hiding this comment.
Well, it has to be called distance to match the standard std::distance so that we can use LayerIndex directly in a parallel_for loop.
Maybe a comment would be helpful ?
| /*! | ||
| * Generates a series of outsetting walls around a given shape | ||
| * @param shape The shape to be wrapped | ||
| * @param width The maximum width to be reached, actual result may be thinner but never larger | ||
| * @param line_width The line width to be used to print each outset line | ||
| * @return The generated outset lines and the actual width of the outermost contour | ||
| */ | ||
| static InsetOutset generateOutset(const Shape& shape, const coord_t width, const coord_t line_width); | ||
|
|
||
| /*! | ||
| * Generates a series of insetting walls inside a given shape | ||
| * @param shape The shape to be filled | ||
| * @param width The maximum width to be reached, actual result may be thinner but never larger | ||
| * @param line_width The line width to be used to print each inset line | ||
| * @return The generated inset lines and the actual width of the innermost contour | ||
| */ | ||
| static InsetOutset generateInset(const Shape& shape, const coord_t width, const coord_t line_width); |
There was a problem hiding this comment.
Can't this be achieved using a single function where width can be either negative or positive, same for generateInsetOutset? It would also be more inline with the offset function where the `offset parameter can be positive or negative.
There was a problem hiding this comment.
That is actually what is done under the hood. There is a private generateInsetOutset method that is called by both. As an external API I find it nicer to have two explicit functions rather than giving an extra argument.
| Shape covered_area = storage_.getLayerOutlines( | ||
| 0, | ||
| /*include_support*/ true, | ||
| /*include_prime_tower*/ adhesion_type_ == EPlatformAdhesion::SKIRT); | ||
| /*include_prime_tower*/ adhesion_type_ == EPlatformAdhesion::SKIRT, | ||
| /*external_polys_only*/ false, | ||
| /*extruder_nr*/ -1, | ||
| /*include_models*/ true, | ||
| /*include_support_base*/ false); |
There was a problem hiding this comment.
I think we should be using named variables for consistency.
| Shape covered_area = storage_.getLayerOutlines( | |
| 0, | |
| /*include_support*/ true, | |
| /*include_prime_tower*/ adhesion_type_ == EPlatformAdhesion::SKIRT); | |
| /*include_prime_tower*/ adhesion_type_ == EPlatformAdhesion::SKIRT, | |
| /*external_polys_only*/ false, | |
| /*extruder_nr*/ -1, | |
| /*include_models*/ true, | |
| /*include_support_base*/ false); | |
| constexpr LayerIndex layer_nr = 0; | |
| constexpr bool include_support = true; | |
| const bool include_prime_tower = adhesion_type_ == EPlatformAdhesion::SKIRT; | |
| constexpr coord_t extruder_nr = -1; | |
| constexpr bool include_models = true; | |
| constexpr bool include_support_base = false; | |
| Shape covered_area = storage_.getLayerOutlines( | |
| layer_nr, | |
| include_support, | |
| include_prime_tower, | |
| external_polys_only, | |
| extruder_nr, | |
| include_models, | |
| include_support_base | |
| ); |
| }), | ||
| base_inset.end()); | ||
|
|
||
| if (base_inset.length() > 0) |
There was a problem hiding this comment.
| if (base_inset.length() > 0) | |
| if (! base_inset.empty()) |
There was a problem hiding this comment.
In this case, length() gives the contained lines total length, so it does check that the list is not empty, and also that it contains non-empty polylines. Probably a bit redundant, but slightly safer. I think I actually moved this check from somewhere else.
Co-authored-by: Casper Lamboo <casperlamboo@gmail.com>
|
This PR's main goal is to implement the chamfered base for the support, both tree and normal. The base makes the bottom of the support much sturdier and also improves adhesion to the buildplate, so it is now possible to use higher supports, or tree supports with less trunks.
In addition to that, it contains a few other significant changes:
-o "with_curaviz=True"at conan install timeRequires Ultimaker/Cura#21714