-
-
Notifications
You must be signed in to change notification settings - Fork 886
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
Cura 10811 improve smooth #1928
Conversation
This includes updated fluid motion logic. A new definition of fluid motion is introduced, here a tool-path-part is considered to be fluid if all of the following criterial hold a) Vectors [A,B] and [C,D] are "long" (settable through settings) b) Vector [B,C] is "short" (settable through settings) c) either on the angles between vectors - [A_,D_] and BC or, - [A_,B] and BC or, - [B,C] and [C,D_] is below a threshold (settable through the settings) B--C / \ A_ D_ / \ / \ A \ \ D Unit tests are added, both to show intent of the `isSmooth` predicate and to test against future regressions. CURA-10811
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.
clang-tidy
found issue(s) with the introduced code (1/5)
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.
clang-tidy
found issue(s) with the introduced code (2/5)
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.
clang-tidy
found issue(s) with the introduced code (1/3)
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.
clang-tidy
found issue(s) with the introduced code (2/3)
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.
clang-tidy
found issue(s) with the introduced code (1/1)
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.
Need to do a proper review, this is just me checking the workflow automations
include/utils/actions/smooth.h
Outdated
{ | ||
if (AB_magnitude == 0.0 || BC_magnitude == 0.0) | ||
return cosAngle<Point>(A, B, magnitude(A), magnitude(B)); |
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.
This seems valid please check
include/utils/actions/smooth.h
Outdated
requires utils::point2d<Vector> || utils::junction<Vector> | ||
auto cosAngle(Vector& A, Vector& B, const utils::floating_point auto A_magnitude, const utils::floating_point auto B_magnitude) const noexcept | ||
{ | ||
if (A_magnitude <= FLT_EPSILON || B_magnitude <= FLT_EPSILON) |
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.
include/utils/actions/smooth.h
Outdated
requires utils::point2d<Vector> || utils::junction<Vector> | ||
auto cosAngle(Vector& A, Vector& B, const utils::floating_point auto A_magnitude, const utils::floating_point auto B_magnitude) const noexcept | ||
{ | ||
if (A_magnitude <= FLT_EPSILON || B_magnitude <= FLT_EPSILON) |
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.
@@ -40,8 +46,15 @@ WallToolPaths::WallToolPaths(const Polygons& outline, const coord_t nominal_bead | |||
{ | |||
} | |||
|
|||
WallToolPaths::WallToolPaths(const Polygons& outline, const coord_t bead_width_0, const coord_t bead_width_x, | |||
const size_t inset_count, const coord_t wall_0_inset, const Settings& settings, const int layer_idx, SectionType section_type ) | |||
WallToolPaths::WallToolPaths( |
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.
valid, please check
bead_width_0, | ||
bead_width_x, | ||
wall_transition_length, | ||
transitioning_angle, |
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.
valid please change the float to a double
return toolpaths; | ||
} | ||
|
||
|
||
void WallToolPaths::stitchToolPaths(std::vector<VariableWidthLines>& toolpaths, const Settings& settings) | ||
{ | ||
const coord_t stitch_distance = settings.get<coord_t>("wall_line_width_x") - 1; //In 0-width contours, junctions can cause up to 1-line-width gaps. Don't stitch more than 1 line width. | ||
const coord_t stitch_distance | ||
= settings.get<coord_t>("wall_line_width_x") - 1; // In 0-width contours, junctions can cause up to 1-line-width gaps. Don't stitch more than 1 line width. | ||
|
||
for (unsigned int wall_idx = 0; wall_idx < toolpaths.size(); wall_idx++) |
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.
valid please change in IDE since this also requires changes in the loop
It was changing the wrong points CURA-10811
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.
clang-tidy
found issue(s) with the introduced code (1/4)
include/utils/actions/smooth.h
Outdated
const auto [AB_magnitude, BC_magnitude, CD_magnitude] = computeMagnitudes(A, B, C, D); | ||
if (! isWithinAllowedDeviations(A, B, C, D, fluid_angle, max_resolution, AB_magnitude, BC_magnitude, CD_magnitude)) | ||
const auto fluid_motion_shift_distance3 = 3 * fluid_motion_shift_distance; | ||
if (dist(*A, *B) < fluid_motion_shift_distance3 || dist(*B, *C) > fluid_motion_small_distance || dist(*C, *D) < fluid_motion_shift_distance3) |
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.
invalid case style for variable A
if (dist(*A, *B) < fluid_motion_shift_distance3 || dist(*B, *C) > fluid_motion_small_distance || dist(*C, *D) < fluid_motion_shift_distance3) | |
if (dist(*a, *B) < fluid_motion_shift_distance3 || dist(*B, *C) > fluid_motion_small_distance || dist(*C, *D) < fluid_motion_shift_distance3) { |
include/utils/actions/smooth.h
Outdated
const auto [AB_magnitude, BC_magnitude, CD_magnitude] = computeMagnitudes(A, B, C, D); | ||
if (! isWithinAllowedDeviations(A, B, C, D, fluid_angle, max_resolution, AB_magnitude, BC_magnitude, CD_magnitude)) | ||
const auto fluid_motion_shift_distance3 = 3 * fluid_motion_shift_distance; | ||
if (dist(*A, *B) < fluid_motion_shift_distance3 || dist(*B, *C) > fluid_motion_small_distance || dist(*C, *D) < fluid_motion_shift_distance3) |
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.
invalid case style for variable B
if (dist(*A, *B) < fluid_motion_shift_distance3 || dist(*B, *C) > fluid_motion_small_distance || dist(*C, *D) < fluid_motion_shift_distance3) | |
if (dist(*A, *b) < fluid_motion_shift_distance3 || dist(*B, *C) > fluid_motion_small_distance || dist(*C, *D) < fluid_motion_shift_distance3) { |
template<class Point> | ||
requires utils::point2d<Point> || utils::junction<Point> | ||
auto cosAngle(Point& A, Point& B, Point& C) const noexcept | ||
{ |
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.
invalid case style for parameter C
{ | |
return cosAngle(A, B, c, dist(A, B), dist(B, C)); |
template<class Point> | ||
requires utils::point2d<Point> || utils::junction<Point> | ||
auto cosAngle(Point& A, Point& B, Point& C) const noexcept | ||
{ |
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.
invalid case style for parameter C
{ | |
return cosAngle(A, B, C, dist(A, B), dist(B, c)); |
{ | ||
return cosAngle(A, B, C, dist(A, B), dist(B, C)); | ||
} | ||
|
||
template<class Point> | ||
requires utils::point2d<Point> || utils::junction<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.
invalid case style for parameter A
requires utils::point2d<Point> || utils::junction<Point> | |
auto cosAngle(Point& a, Point& B, Point& C, const utils::floating_point auto AB_magnitude, const utils::floating_point auto BC_magnitude) const noexcept |
template<class Point> | ||
requires utils::point2d<Point> || utils::junction<Point> | ||
constexpr auto computeMagnitudes(Point* A, Point* B, Point* C, Point* D) const noexcept | ||
auto cosAngle(Point& A, Point& B, Point& C, const utils::floating_point auto AB_magnitude, const utils::floating_point auto BC_magnitude) const noexcept | ||
{ |
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.
invalid case style for parameter A
{ | |
return cosAngle(a, B, B, C, AB_magnitude, BC_magnitude); |
{ | ||
return cosAngle(A, B, C, dist(A, B), dist(B, C)); | ||
} | ||
|
||
template<class Point> | ||
requires utils::point2d<Point> || utils::junction<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.
invalid case style for parameter B
requires utils::point2d<Point> || utils::junction<Point> | |
auto cosAngle(Point& A, Point& b, Point& C, const utils::floating_point auto AB_magnitude, const utils::floating_point auto BC_magnitude) const noexcept |
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.
clang-tidy
found issue(s) with the introduced code (2/4)
template<class Point> | ||
requires utils::point2d<Point> || utils::junction<Point> | ||
constexpr auto computeMagnitudes(Point* A, Point* B, Point* C, Point* D) const noexcept | ||
auto cosAngle(Point& A, Point& B, Point& C, const utils::floating_point auto AB_magnitude, const utils::floating_point auto BC_magnitude) const noexcept | ||
{ |
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.
invalid case style for parameter B
{ | |
return cosAngle(A, b, B, C, AB_magnitude, BC_magnitude); |
template<class Point> | ||
requires utils::point2d<Point> || utils::junction<Point> | ||
constexpr auto computeMagnitudes(Point* A, Point* B, Point* C, Point* D) const noexcept | ||
auto cosAngle(Point& A, Point& B, Point& C, const utils::floating_point auto AB_magnitude, const utils::floating_point auto BC_magnitude) const noexcept | ||
{ |
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.
invalid case style for parameter B
{ | |
return cosAngle(A, B, b, C, AB_magnitude, BC_magnitude); |
{ | ||
return cosAngle(A, B, C, dist(A, B), dist(B, C)); | ||
} | ||
|
||
template<class Point> | ||
requires utils::point2d<Point> || utils::junction<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.
invalid case style for parameter C
requires utils::point2d<Point> || utils::junction<Point> | |
auto cosAngle(Point& A, Point& B, Point& c, const utils::floating_point auto AB_magnitude, const utils::floating_point auto BC_magnitude) const noexcept |
template<class Point> | ||
requires utils::point2d<Point> || utils::junction<Point> | ||
constexpr auto computeMagnitudes(Point* A, Point* B, Point* C, Point* D) const noexcept | ||
auto cosAngle(Point& A, Point& B, Point& C, const utils::floating_point auto AB_magnitude, const utils::floating_point auto BC_magnitude) const noexcept | ||
{ |
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.
invalid case style for parameter C
{ | |
return cosAngle(A, B, B, c, AB_magnitude, BC_magnitude); |
{ | ||
return cosAngle(A, B, C, dist(A, B), dist(B, C)); | ||
} | ||
|
||
template<class Point> | ||
requires utils::point2d<Point> || utils::junction<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.
invalid case style for parameter AB_magnitude
requires utils::point2d<Point> || utils::junction<Point> | |
auto cosAngle(Point& A, Point& B, Point& C, const utils::floating_point auto ab_magnitude, const utils::floating_point auto BC_magnitude) const noexcept |
} | ||
|
||
template<class Point> | ||
requires utils::point2d<Point> || utils::junction<Point> | ||
constexpr auto cosAngle(Point* A, Point* B, Point* C, const utils::floating_point auto AB_magnitude, const utils::floating_point auto BC_magnitude) const noexcept | ||
auto cosAngle(Point& A, Point& B, Point& C, Point& D, const utils::floating_point auto AB_magnitude, const utils::floating_point auto BC_magnitude) const noexcept | ||
{ |
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.
invalid case style for parameter B
{ | |
Point VectorA = { std::get<"X">(B) - std::get<"X">(A), std::get<"Y">(b) - std::get<"Y">(A) }; |
requires utils::point2d<Point> || utils::junction<Point> | ||
auto cosAngle(Point& A, Point& B, Point& C, Point& D) const noexcept | ||
{ | ||
return cosAngle(A, B, C, D, dist(A, B), dist(C, D)); | ||
} | ||
|
||
template<class Point> | ||
requires utils::point2d<Point> || utils::junction<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.
invalid case style for parameter C
requires utils::point2d<Point> || utils::junction<Point> | |
auto cosAngle(Point& A, Point& B, Point& c, Point& D, const utils::floating_point auto AB_magnitude, const utils::floating_point auto BC_magnitude) const noexcept |
include/utils/actions/smooth.h
Outdated
{ | ||
if (AB_magnitude == 0.0 || BC_magnitude == 0.0) | ||
Point VectorA = { std::get<"X">(B) - std::get<"X">(A), std::get<"Y">(B) - std::get<"Y">(A) }; |
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.
invalid case style for parameter C
Point VectorA = { std::get<"X">(B) - std::get<"X">(A), std::get<"Y">(B) - std::get<"Y">(A) }; | |
Point VectorB = { std::get<"X">(D) - std::get<"X">(c), std::get<"Y">(D) - std::get<"Y">(C) }; |
include/utils/actions/smooth.h
Outdated
{ | ||
if (AB_magnitude == 0.0 || BC_magnitude == 0.0) | ||
Point VectorA = { std::get<"X">(B) - std::get<"X">(A), std::get<"Y">(B) - std::get<"Y">(A) }; |
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.
invalid case style for parameter C
Point VectorA = { std::get<"X">(B) - std::get<"X">(A), std::get<"Y">(B) - std::get<"Y">(A) }; | |
Point VectorB = { std::get<"X">(D) - std::get<"X">(C), std::get<"Y">(D) - std::get<"Y">(c) }; |
requires utils::point2d<Point> || utils::junction<Point> | ||
auto cosAngle(Point& A, Point& B, Point& C, Point& D) const noexcept | ||
{ | ||
return cosAngle(A, B, C, D, dist(A, B), dist(C, D)); | ||
} | ||
|
||
template<class Point> | ||
requires utils::point2d<Point> || utils::junction<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.
invalid case style for parameter D
requires utils::point2d<Point> || utils::junction<Point> | |
auto cosAngle(Point& A, Point& B, Point& C, Point& d, const utils::floating_point auto AB_magnitude, const utils::floating_point auto BC_magnitude) const noexcept |
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.
clang-tidy
found issue(s) with the introduced code (1/1)
include/utils/actions/smooth.h
Outdated
const auto cos_fluid_motion_angle = std::cos(fluid_motion_angle); | ||
if (! isSmooth(*A, *B, *C, *D, cos_fluid_motion_angle)) | ||
{ | ||
*B = shiftPointTowards(*B, *A, fluid_motion_shift_distance); |
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.
invalid case style for variable A
*B = shiftPointTowards(*B, *A, fluid_motion_shift_distance); | |
*B = shiftPointTowards(*B, *a, fluid_motion_shift_distance); |
include/utils/actions/smooth.h
Outdated
const auto cos_fluid_motion_angle = std::cos(fluid_motion_angle); | ||
if (! isSmooth(*A, *B, *C, *D, cos_fluid_motion_angle)) | ||
{ | ||
*B = shiftPointTowards(*B, *A, fluid_motion_shift_distance); |
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.
invalid case style for variable B
*B = shiftPointTowards(*B, *A, fluid_motion_shift_distance); | |
*b = shiftPointTowards(*B, *A, fluid_motion_shift_distance); |
include/utils/actions/smooth.h
Outdated
const auto cos_fluid_motion_angle = std::cos(fluid_motion_angle); | ||
if (! isSmooth(*A, *B, *C, *D, cos_fluid_motion_angle)) | ||
{ | ||
*B = shiftPointTowards(*B, *A, fluid_motion_shift_distance); |
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.
invalid case style for variable B
*B = shiftPointTowards(*B, *A, fluid_motion_shift_distance); | |
*B = shiftPointTowards(*b, *A, fluid_motion_shift_distance); |
include/utils/actions/smooth.h
Outdated
if (! isSmooth(*A, *B, *C, *D, cos_fluid_motion_angle)) | ||
{ | ||
*B = shiftPointTowards(*B, *A, fluid_motion_shift_distance); | ||
*C = shiftPointTowards(*C, *D, fluid_motion_shift_distance); |
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.
invalid case style for variable C
*C = shiftPointTowards(*C, *D, fluid_motion_shift_distance); | |
*c = shiftPointTowards(*C, *D, fluid_motion_shift_distance); |
include/utils/actions/smooth.h
Outdated
if (! isSmooth(*A, *B, *C, *D, cos_fluid_motion_angle)) | ||
{ | ||
*B = shiftPointTowards(*B, *A, fluid_motion_shift_distance); | ||
*C = shiftPointTowards(*C, *D, fluid_motion_shift_distance); |
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.
invalid case style for variable C
*C = shiftPointTowards(*C, *D, fluid_motion_shift_distance); | |
*C = shiftPointTowards(*c, *D, fluid_motion_shift_distance); |
include/utils/actions/smooth.h
Outdated
if (! isSmooth(*A, *B, *C, *D, cos_fluid_motion_angle)) | ||
{ | ||
*B = shiftPointTowards(*B, *A, fluid_motion_shift_distance); | ||
*C = shiftPointTowards(*C, *D, fluid_motion_shift_distance); |
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.
invalid case style for variable D
*C = shiftPointTowards(*C, *D, fluid_motion_shift_distance); | |
*C = shiftPointTowards(*C, *d, fluid_motion_shift_distance); |
ec5d22c
to
e10d0cf
Compare
Epsilon is a function aparently and not a constant CURA-10811
e10d0cf
to
456474b
Compare
Used snake case variable names Use pre-computed dist values CURA-10811
@casperlamboo might be best if you close this PR and open a new one. The Workflow should be disabled now |
This includes updated fluid motion logic. A new definition of fluid motion is introduced, here a tool-path-part is considered to be fluid if all of the following criterial hold
a) Vectors [A,B] and [C,D] are "long" (settable through settings)
b) Vector [B,C] is "short" (settable through settings)
c) either on the angles between vectors
- [A_,D_] and [B,C] or,
- [A_,B] and [B,C] or,
- [B,C] and [C,D_]
is below a threshold (settable through the settings)
Unit tests are added, both to show intent of the
isSmooth
predicate and to test against future regressions.Cura-10811