From f1d52cb41209e1e7cca525242ccab024a62aa00c Mon Sep 17 00:00:00 2001 From: Vojtech Bubnik Date: Fri, 24 Jun 2022 15:08:35 +0200 Subject: [PATCH 1/5] Fixes of cooling buffer logic: Fixed division by zero on zero length extrusions. Added bunch of asserts to check for divisions by zero. --- src/libslic3r/GCode/CoolingBuffer.cpp | 35 ++++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 4 deletions(-) diff --git a/src/libslic3r/GCode/CoolingBuffer.cpp b/src/libslic3r/GCode/CoolingBuffer.cpp index 301577e58..771fdf80b 100644 --- a/src/libslic3r/GCode/CoolingBuffer.cpp +++ b/src/libslic3r/GCode/CoolingBuffer.cpp @@ -136,6 +136,7 @@ struct PerExtruderAdjustments assert(line.time_max >= 0.f && line.time_max < FLT_MAX); line.slowdown = true; line.time = line.time_max; + assert(line.time > 0); line.feedrate = line.length / line.time; } time_total += line.time; @@ -151,6 +152,7 @@ struct PerExtruderAdjustments if (line.adjustable(slowdown_external_perimeters)) { line.slowdown = true; line.time = std::min(line.time_max, line.time * factor); + assert(line.time > 0); line.feedrate = line.length / line.time; } time_total += line.time; @@ -182,8 +184,10 @@ struct PerExtruderAdjustments assert(this->min_print_speed < min_feedrate + EPSILON); for (size_t i = 0; i < n_lines_adjustable; ++ i) { const CoolingLine &line = lines[i]; - if (line.feedrate > min_feedrate) + if (line.feedrate > min_feedrate) { + assert(min_feedrate > 0); time_stretch += line.time * (line.feedrate / min_feedrate - 1.f); + } } return time_stretch; } @@ -196,6 +200,7 @@ struct PerExtruderAdjustments for (size_t i = 0; i < n_lines_adjustable; ++ i) { CoolingLine &line = lines[i]; if (line.feedrate > min_feedrate) { + assert(min_feedrate > 0); line.time *= std::max(1.f, line.feedrate / min_feedrate); line.feedrate = min_feedrate; line.slowdown = true; @@ -250,7 +255,7 @@ float new_feedrate_to_reach_time_stretch( } } assert(denom > 0); - if (denom < 0) + if (denom <= 0) return min_feedrate; new_feedrate = nomin / denom; assert(new_feedrate > min_feedrate - EPSILON); @@ -404,11 +409,16 @@ std::vector CoolingBuffer::parse_layer_gcode(const std:: } line.feedrate = new_pos[4]; assert((line.type & CoolingLine::TYPE_ADJUSTABLE) == 0 || line.feedrate > 0.f); - if (line.length > 0) + if (line.length > 0) { + assert(line.feedrate > 0); line.time = line.length / line.feedrate; + assert(line.time > 0); + } line.time_max = line.time; - if ((line.type & CoolingLine::TYPE_ADJUSTABLE) || active_speed_modifier != size_t(-1)) + if ((line.type & CoolingLine::TYPE_ADJUSTABLE) || active_speed_modifier != size_t(-1)) { + assert(adjustment->min_print_speed >= 0); line.time_max = (adjustment->min_print_speed == 0.f) ? FLT_MAX : std::max(line.time, line.length / adjustment->min_print_speed); + } if (active_speed_modifier < adjustment->lines.size() && (line.type & CoolingLine::TYPE_G1)) { // Inside the ";_EXTRUDE_SET_SPEED" blocks, there must not be a G1 Fxx entry. assert((line.type & CoolingLine::TYPE_HAS_F) == 0); @@ -428,7 +438,23 @@ std::vector CoolingBuffer::parse_layer_gcode(const std:: } current_pos = std::move(new_pos); } else if (boost::starts_with(sline, ";_EXTRUDE_END")) { + // Closing a block of non-zero length extrusion moves. line.type = CoolingLine::TYPE_EXTRUDE_END; + if (active_speed_modifier != size_t(-1)) { +#ifndef _NDEBUG + assert(active_speed_modifier < adjustment->lines.size()); + CoolingLine &sm = adjustment->lines[active_speed_modifier]; + // There should be at least some extrusion move inside the adjustment block. + // However if the block has no extrusion (which is wrong), fix it for the cooling buffer to work. + assert(sm.length > 0); + assert(sm.time > 0); + if (sm.time <= 0) { + // Likely a zero length extrusion, it should not be emitted, however the zero extrusions should not confuse firmware either. + // Prohibit time adjustment of a block of zero length extrusions by the cooling buffer. + sm.type &= ~CoolingLine::TYPE_ADJUSTABLE; + } +#endif // _NDEBUG + } active_speed_modifier = size_t(-1); } else if (boost::starts_with(sline, m_toolchange_prefix)) { unsigned int new_extruder = (unsigned int)atoi(sline.c_str() + m_toolchange_prefix.size()); @@ -568,6 +594,7 @@ static inline void extruder_range_slow_down_non_proportional( float time_adjustable = 0.f; for (auto it = adj; it != by_min_print_speed.end(); ++ it) time_adjustable += (*it)->adjustable_time(true); + assert(time_adjustable > 0); float rate = (time_adjustable + time_stretch) / time_adjustable; for (auto it = adj; it != by_min_print_speed.end(); ++ it) (*it)->slow_down_proportional(rate, true); From 0c99a14b63ab15b864b30a951fdf0e3b4a73965c Mon Sep 17 00:00:00 2001 From: Vojtech Bubnik Date: Fri, 24 Jun 2022 16:46:49 +0200 Subject: [PATCH 2/5] Follow-up to f1d52cb41209e1e7cca525242ccab024a62aa00c Fixes of cooling buffer logic: Fixed division by zero on zero length extrusions. --- src/libslic3r/GCode/CoolingBuffer.cpp | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/libslic3r/GCode/CoolingBuffer.cpp b/src/libslic3r/GCode/CoolingBuffer.cpp index 771fdf80b..ef0d63c93 100644 --- a/src/libslic3r/GCode/CoolingBuffer.cpp +++ b/src/libslic3r/GCode/CoolingBuffer.cpp @@ -54,6 +54,9 @@ struct CoolingLine TYPE_WIPE = 1 << 9, TYPE_G4 = 1 << 10, TYPE_G92 = 1 << 11, + // Would be TYPE_ADJUSTABLE, but the block of G-code lines has zero extrusion length, thus the block + // cannot have its speed adjusted. This should not happen (sic!). + TYPE_ADJUSTABLE_EMPTY = 1 << 12, }; CoolingLine(unsigned int type, size_t line_start, size_t line_end) : @@ -441,7 +444,6 @@ std::vector CoolingBuffer::parse_layer_gcode(const std:: // Closing a block of non-zero length extrusion moves. line.type = CoolingLine::TYPE_EXTRUDE_END; if (active_speed_modifier != size_t(-1)) { -#ifndef _NDEBUG assert(active_speed_modifier < adjustment->lines.size()); CoolingLine &sm = adjustment->lines[active_speed_modifier]; // There should be at least some extrusion move inside the adjustment block. @@ -452,8 +454,9 @@ std::vector CoolingBuffer::parse_layer_gcode(const std:: // Likely a zero length extrusion, it should not be emitted, however the zero extrusions should not confuse firmware either. // Prohibit time adjustment of a block of zero length extrusions by the cooling buffer. sm.type &= ~CoolingLine::TYPE_ADJUSTABLE; + // But the start / end comment shall be removed. + sm.type |= CoolingLine::TYPE_ADJUSTABLE_EMPTY; } -#endif // _NDEBUG } active_speed_modifier = size_t(-1); } else if (boost::starts_with(sline, m_toolchange_prefix)) { @@ -789,7 +792,7 @@ std::string CoolingBuffer::apply_layer_cooldown( new_gcode += GCodeWriter::set_fan(m_config.gcode_flavor, m_config.gcode_comments, m_fan_speed); } else if (line->type & CoolingLine::TYPE_EXTRUDE_END) { // Just remove this comment. - } else if (line->type & (CoolingLine::TYPE_ADJUSTABLE | CoolingLine::TYPE_EXTERNAL_PERIMETER | CoolingLine::TYPE_WIPE | CoolingLine::TYPE_HAS_F)) { + } else if (line->type & (CoolingLine::TYPE_ADJUSTABLE | CoolingLine::TYPE_ADJUSTABLE_EMPTY | CoolingLine::TYPE_EXTERNAL_PERIMETER | CoolingLine::TYPE_WIPE | CoolingLine::TYPE_HAS_F)) { // Find the start of a comment, or roll to the end of line. const char *end = line_start; for (; end < line_end && *end != ';'; ++ end); @@ -804,7 +807,7 @@ std::string CoolingBuffer::apply_layer_cooldown( new_feedrate = line->slowdown ? int(floor(60. * line->feedrate + 0.5)) : atoi(fpos); if (new_feedrate == current_feedrate) { // No need to change the F value. - if ((line->type & (CoolingLine::TYPE_ADJUSTABLE | CoolingLine::TYPE_EXTERNAL_PERIMETER | CoolingLine::TYPE_WIPE)) || line->length == 0.) + if ((line->type & (CoolingLine::TYPE_ADJUSTABLE | CoolingLine::TYPE_ADJUSTABLE_EMPTY | CoolingLine::TYPE_EXTERNAL_PERIMETER | CoolingLine::TYPE_WIPE)) || line->length == 0.) // Feedrate does not change and this line does not move the print head. Skip the complete G-code line including the G-code comment. end = line_end; else @@ -849,7 +852,7 @@ std::string CoolingBuffer::apply_layer_cooldown( } // Process the rest of the line. if (end < line_end) { - if (line->type & (CoolingLine::TYPE_ADJUSTABLE | CoolingLine::TYPE_EXTERNAL_PERIMETER | CoolingLine::TYPE_WIPE)) { + if (line->type & (CoolingLine::TYPE_ADJUSTABLE | CoolingLine::TYPE_ADJUSTABLE_EMPTY | CoolingLine::TYPE_EXTERNAL_PERIMETER | CoolingLine::TYPE_WIPE)) { // Process comments, remove ";_EXTRUDE_SET_SPEED", ";_EXTERNAL_PERIMETER", ";_WIPE" std::string comment(end, line_end); boost::replace_all(comment, ";_EXTRUDE_SET_SPEED", ""); From d01f6099c394d881a1a75d5093fb8493f870584f Mon Sep 17 00:00:00 2001 From: Vojtech Bubnik Date: Fri, 24 Jun 2022 17:28:09 +0200 Subject: [PATCH 3/5] Fixing Polyline::split_at() to handle correctly splitting at the first / last point. --- src/libslic3r/Polyline.cpp | 52 ++++++++++++++++++-------------------- 1 file changed, 24 insertions(+), 28 deletions(-) diff --git a/src/libslic3r/Polyline.cpp b/src/libslic3r/Polyline.cpp index 45ead8643..18ec759e0 100644 --- a/src/libslic3r/Polyline.cpp +++ b/src/libslic3r/Polyline.cpp @@ -132,37 +132,33 @@ template void Polyline::simplify_by_visibility(const ExPoly void Polyline::split_at(const Point &point, Polyline* p1, Polyline* p2) const { - if (this->points.empty()) return; - - // find the line to split at - size_t line_idx = 0; - Point p = this->first_point(); - double min = (p - point).cast().norm(); - Lines lines = this->lines(); - for (Lines::const_iterator line = lines.begin(); line != lines.end(); ++line) { - Point p_tmp = point.projection_onto(*line); - if ((p_tmp - point).cast().norm() < min) { - p = p_tmp; - min = (p - point).cast().norm(); - line_idx = line - lines.begin(); + if (this->size() < 2 || this->points.front() == point) { + *p1 = *this; + p2->clear(); + return; + } + + auto min_dist2 = std::numeric_limits::max(); + auto min_point_it = this->points.cbegin(); + Point prev = this->points.front(); + for (auto it = this->points.cbegin() + 1; it != this->points.cend(); ++ it) { + Point proj = point.projection_onto(Line(prev, *it)); + auto d2 = (proj - point).cast().squaredNorm(); + if (d2 < min_dist2) { + min_dist2 = d2; + min_point_it = it; } + prev = *it; } + + p1->points.assign(this->points.cbegin(), min_point_it); + if (p1->points.back() != point) + p1->points.emplace_back(point); - // create first half - p1->points.clear(); - for (Lines::const_iterator line = lines.begin(); line != lines.begin() + line_idx + 1; ++line) - if (line->a != p) - p1->points.push_back(line->a); - // we add point instead of p because they might differ because of numerical issues - // and caller might want to rely on point belonging to result polylines - p1->points.push_back(point); - - // create second half - p2->points.clear(); - p2->points.push_back(point); - for (Lines::const_iterator line = lines.begin() + line_idx; line != lines.end(); ++line) { - p2->points.push_back(line->b); - } + p2->points = { point }; + if (*min_point_it == point) + ++ min_point_it; + p2->points.insert(p2->points.end(), min_point_it, this->points.cend()); } bool Polyline::is_straight() const From c6bcaedba9a9e967a227d721838e6fa3d9651f67 Mon Sep 17 00:00:00 2001 From: Vojtech Bubnik Date: Fri, 24 Jun 2022 17:46:26 +0200 Subject: [PATCH 4/5] Follow-up to d01f6099c394d881a1a75d5093fb8493f870584f Fixing unit tests. --- src/libslic3r/Polyline.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/libslic3r/Polyline.cpp b/src/libslic3r/Polyline.cpp index 18ec759e0..c706aa50b 100644 --- a/src/libslic3r/Polyline.cpp +++ b/src/libslic3r/Polyline.cpp @@ -132,12 +132,17 @@ template void Polyline::simplify_by_visibility(const ExPoly void Polyline::split_at(const Point &point, Polyline* p1, Polyline* p2) const { - if (this->size() < 2 || this->points.front() == point) { + if (this->size() < 2) { *p1 = *this; p2->clear(); return; } + if (this->points.front() == point) { + *p1 = point; + *p2 = *this; + } + auto min_dist2 = std::numeric_limits::max(); auto min_point_it = this->points.cbegin(); Point prev = this->points.front(); From f0886b46fb9fbef331e734342cb1cf68b64979c0 Mon Sep 17 00:00:00 2001 From: Vojtech Bubnik Date: Mon, 27 Jun 2022 10:42:42 +0200 Subject: [PATCH 5/5] Follow-up to c6bcaedba9a9e967a227d721838e6fa3d9651f67 --- src/libslic3r/Polyline.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libslic3r/Polyline.cpp b/src/libslic3r/Polyline.cpp index c706aa50b..aaaa647c0 100644 --- a/src/libslic3r/Polyline.cpp +++ b/src/libslic3r/Polyline.cpp @@ -139,7 +139,7 @@ void Polyline::split_at(const Point &point, Polyline* p1, Polyline* p2) const } if (this->points.front() == point) { - *p1 = point; + *p1 = { point }; *p2 = *this; }