From 02d45c32584aeb72bdb8f55336568f9a24520ee4 Mon Sep 17 00:00:00 2001 From: Joseph Robertson <32232925+HarrierPigeon@users.noreply.github.com> Date: Thu, 4 Jun 2026 14:40:45 -0500 Subject: [PATCH] Finish Fixes from Copilot Review (#39) * fix: restore BuildVolume bounds when toggling belt mode set_belt_printer() mutated m_bboxf when enabling but never restored the original extents on disable or when switching infinite_y true->false, leaving stale max.y/max.z values that broke collision and object_state checks. Recompute m_bboxf from m_bed_shape + m_max_print_height at the top of each call, then apply belt-specific adjustments on top. Addresses Copilot review comment on PR #12998 (BuildVolume.cpp:196). * chore: drop [BELT-DEBUG] to_machine_coords log to trace Was emitting at warning level once per 0.2mm Z bucket during every belt print export, polluting default user logs. Trace level matches the rest of the belt diagnostics and is silent in production. Addresses Copilot review comment on PR #12998 (BeltGCodeWriter.cpp:86). * chore: drop [BELTRACE] make_perimeters/support logs to trace Eight warning-level traces around make_perimeters and generate_support_material were emitting on every call/exit during normal slicing, cluttering default logs. They're concurrency-debug breadcrumbs not user-facing diagnostics, so drop them to trace. Addresses Copilot review comment on PR #12998 (PrintObject.cpp:438). * perf: gate BeltSliceStrategy diagnostic bbox tracking behind compile flag apply_to_trafo() walked every model vertex twice (once for min_z, once for per-volume mesh/slicer bboxes) and emitted seven trace logs per call. The bboxes and logs are diagnostic only; min_z is the load-bearing output. Wrap the bbox accumulation, logging, and supporting headers in SLIC3R_BELT_DIAGNOSTIC_LOG so production builds do the bare min_z scan. Addresses Copilot review comment on PR #12998 (BeltSliceStrategy.cpp:95). * fix: apply part_cooling_fan_min_pwm to first-layer plane fan crossings apply_first_layer_plane_fan_eval emitted band-crossing M106 commands through GCodeWriter::set_fan() without the per-printer PWM floor that every other set_fan call in CoolingBuffer applies. On printers with a non-zero part_cooling_fan_min_pwm, fans could fail to spin up at low requested speeds near the belt surface. Addresses Copilot review comment on PR #12998 (CoolingBuffer.cpp:1227). --- src/libslic3r/BeltGCodeWriter.cpp | 2 +- src/libslic3r/BeltSliceStrategy.cpp | 30 +++++++++++++++++++++++---- src/libslic3r/BuildVolume.cpp | 6 ++++++ src/libslic3r/GCode/CoolingBuffer.cpp | 6 +++++- src/libslic3r/PrintObject.cpp | 16 +++++++------- 5 files changed, 46 insertions(+), 14 deletions(-) diff --git a/src/libslic3r/BeltGCodeWriter.cpp b/src/libslic3r/BeltGCodeWriter.cpp index 751a7f19d8..9f1f32a375 100644 --- a/src/libslic3r/BeltGCodeWriter.cpp +++ b/src/libslic3r/BeltGCodeWriter.cpp @@ -76,7 +76,7 @@ Vec3d BeltGCodeWriter::to_machine_coords(const Vec3d &pos) const int z_bucket = static_cast(std::floor(pos.z() * 5.0)); // every 0.2mm if (z_bucket != s_last_logged_z) { s_last_logged_z = z_bucket; - BOOST_LOG_TRIVIAL(warning) << "[BELT-DEBUG] to_machine_coords" + BOOST_LOG_TRIVIAL(trace) << "[BELT-DEBUG] to_machine_coords" << " slicer_in=(" << pos.x() << "," << pos.y() << "," << pos.z() << ")" << " after_back=(" << after_back.x() << "," << after_back.y() << "," << after_back.z() << ")" << " after_remap=(" << after_remap.x() << "," << after_remap.y() << "," << after_remap.z() << ")" diff --git a/src/libslic3r/BeltSliceStrategy.cpp b/src/libslic3r/BeltSliceStrategy.cpp index ddaabdef0b..fd1bfcc191 100644 --- a/src/libslic3r/BeltSliceStrategy.cpp +++ b/src/libslic3r/BeltSliceStrategy.cpp @@ -2,9 +2,11 @@ #include "Model.hpp" #include +#ifdef SLIC3R_BELT_DIAGNOSTIC_LOG #include #include #include +#endif namespace Slic3r { @@ -40,7 +42,8 @@ void BeltSliceStrategy::apply_to_trafo(Transform3d &trafo, // coordinates rather than object-space coordinates, so volumes translated // along the slicer's Z axis are silently excluded from the bound check. if (has_remap || m_has_rotation) { - // [BELT-DEBUG] Capture the incoming trafo for diagnostic logging. +#ifdef SLIC3R_BELT_DIAGNOSTIC_LOG + // Capture the incoming trafo for diagnostic logging. // This is the slicer-frame transform AFTER belt_xform but BEFORE z_shift. const Transform3d trafo_pre_shift = trafo; auto log_mat = [](const Matrix3d &m) { @@ -63,29 +66,41 @@ void BeltSliceStrategy::apply_to_trafo(Transform3d &trafo, << " trafo.linear=" << log_mat(trafo_pre_shift.linear()) << " trafo.translation=" << log_vec3(trafo_pre_shift.translation()) << " volumes=" << model_volumes.size(); +#endif double min_z = std::numeric_limits::max(); +#ifdef SLIC3R_BELT_DIAGNOSTIC_LOG int vol_idx = 0; +#endif for (const ModelVolume *mv : model_volumes) { +#ifdef SLIC3R_BELT_DIAGNOSTIC_LOG if (!mv->is_model_part()) { ++vol_idx; continue; } +#else + if (!mv->is_model_part()) continue; +#endif Transform3d vol_trafo = trafo * mv->get_matrix(); - // [BELT-DEBUG] Per-volume bbox in mesh-frame and post-trafo slicer-frame. const auto &its = mv->mesh().its; +#ifdef SLIC3R_BELT_DIAGNOSTIC_LOG + // Per-volume bbox in mesh-frame and post-trafo slicer-frame. Vec3d mesh_min(std::numeric_limits::max(), std::numeric_limits::max(), std::numeric_limits::max()); Vec3d mesh_max(std::numeric_limits::lowest(), std::numeric_limits::lowest(), std::numeric_limits::lowest()); Vec3d slicer_min(std::numeric_limits::max(), std::numeric_limits::max(), std::numeric_limits::max()); Vec3d slicer_max(std::numeric_limits::lowest(), std::numeric_limits::lowest(), std::numeric_limits::lowest()); double vol_min_z = std::numeric_limits::max(); +#endif for (const stl_vertex &v : its.vertices) { Vec3d vm = v.cast(); + Vec3d pt = vol_trafo * vm; + min_z = std::min(min_z, pt.z()); +#ifdef SLIC3R_BELT_DIAGNOSTIC_LOG mesh_min = mesh_min.cwiseMin(vm); mesh_max = mesh_max.cwiseMax(vm); - Vec3d pt = vol_trafo * vm; slicer_min = slicer_min.cwiseMin(pt); slicer_max = slicer_max.cwiseMax(pt); vol_min_z = std::min(vol_min_z, pt.z()); - min_z = std::min(min_z, pt.z()); +#endif } +#ifdef SLIC3R_BELT_DIAGNOSTIC_LOG BOOST_LOG_TRIVIAL(trace) << "[BELT-DEBUG] vol[" << vol_idx << "] id=" << mv->id().id << " name='" << mv->name << "'" << " mesh_bbox_min=" << log_vec3(mesh_min) << " mesh_bbox_max=" << log_vec3(mesh_max) @@ -93,10 +108,13 @@ void BeltSliceStrategy::apply_to_trafo(Transform3d &trafo, << " slicer_bbox_min=" << log_vec3(slicer_min) << " slicer_bbox_max=" << log_vec3(slicer_max) << " vol_min_z=" << vol_min_z; ++vol_idx; +#endif } double belt_z_shift_val = (min_z < 0. && min_z != std::numeric_limits::max()) ? -min_z : 0.; +#ifdef SLIC3R_BELT_DIAGNOSTIC_LOG BOOST_LOG_TRIVIAL(trace) << "[BELT-DEBUG] combined min_z=" << min_z << " z_shift_val=" << belt_z_shift_val; +#endif if (belt_z_shift_val > 0.) { Transform3d z_shift = Transform3d::Identity(); z_shift.matrix()(2, 3) = belt_z_shift_val; @@ -104,13 +122,17 @@ void BeltSliceStrategy::apply_to_trafo(Transform3d &trafo, } if (out_belt_min_z) { double new_val = (min_z != std::numeric_limits::max()) ? min_z : 0.; +#ifdef SLIC3R_BELT_DIAGNOSTIC_LOG BOOST_LOG_TRIVIAL(trace) << "[BELT-DEBUG] write m_belt_min_z tid=" << std::this_thread::get_id() << " target=" << out_belt_min_z << " old=" << *out_belt_min_z << " new=" << new_val; +#endif *out_belt_min_z = new_val; } +#ifdef SLIC3R_BELT_DIAGNOSTIC_LOG BOOST_LOG_TRIVIAL(trace) << "[BELT-DEBUG] apply_to_trafo exit" << " final_trafo.linear=" << log_mat(trafo.linear()) << " final_trafo.translation=" << log_vec3(trafo.translation()); +#endif } } diff --git a/src/libslic3r/BuildVolume.cpp b/src/libslic3r/BuildVolume.cpp index d6fae8f0a8..2e194bdd7e 100644 --- a/src/libslic3r/BuildVolume.cpp +++ b/src/libslic3r/BuildVolume.cpp @@ -182,6 +182,12 @@ void BuildVolume::set_belt_printer(bool enabled, double angle_deg, bool infinite m_belt_angle = angle_deg; m_belt_infinite_y = infinite_y; + // Restart from the unmodified bbox each call. Without this, toggling + // belt mode off (or switching infinite_y true→false) would leave the + // extents inflated and break collision / object_state checks. + BoundingBoxf bboxf = get_extents(m_bed_shape); + m_bboxf = BoundingBoxf3{ to_3d(bboxf.min, 0.), to_3d(bboxf.max, m_max_print_height) }; + if (enabled) { if (infinite_y) { // Extend the Y bound to a very large value for infinite belt. diff --git a/src/libslic3r/GCode/CoolingBuffer.cpp b/src/libslic3r/GCode/CoolingBuffer.cpp index a9991f6f62..a92bb7e4cf 100644 --- a/src/libslic3r/GCode/CoolingBuffer.cpp +++ b/src/libslic3r/GCode/CoolingBuffer.cpp @@ -1091,6 +1091,10 @@ std::string CoolingBuffer::apply_first_layer_plane_fan_eval( std::string out; out.reserve(gcode.size() + 256); + // Match the PWM floor applied at every other set_fan call in this file so + // band-crossing M106 emissions start the fan reliably at low speeds. + const unsigned int part_cooling_fan_min_pwm = static_cast(std::max(0, m_config.part_cooling_fan_min_pwm.value)); + // Track position in slicing-frame mm. Seed from m_current_pos which the // CoolingBuffer keeps up-to-date across layers. Vec3d cur_pos_mm(m_current_pos[0], m_current_pos[1], m_current_pos[2]); @@ -1221,7 +1225,7 @@ std::string CoolingBuffer::apply_first_layer_plane_fan_eval( target_fan = pre_band_main_fan; } if (target_fan != current_main_fan) { - out += GCodeWriter::set_fan(m_config.gcode_flavor, target_fan); + out += GCodeWriter::set_fan(m_config.gcode_flavor, target_fan, part_cooling_fan_min_pwm); current_main_fan = target_fan; m_fan_speed = target_fan; m_current_fan_speed = target_fan; diff --git a/src/libslic3r/PrintObject.cpp b/src/libslic3r/PrintObject.cpp index 31565b9a6b..54a3f7bdb9 100644 --- a/src/libslic3r/PrintObject.cpp +++ b/src/libslic3r/PrintObject.cpp @@ -427,15 +427,15 @@ std::vector> PrintObject::detect_extruder_geometric_unprintables() // 3) Generates perimeters, gap fills and fill regions (fill regions of type stInternal). void PrintObject::make_perimeters() { - BOOST_LOG_TRIVIAL(warning) << "[BELTRACE] make_perimeters request tid=" << std::this_thread::get_id() << " obj=" << this; + BOOST_LOG_TRIVIAL(trace) << "[BELTRACE] make_perimeters request tid=" << std::this_thread::get_id() << " obj=" << this; // prerequisites this->slice(); if (! this->set_started(posPerimeters)) { - BOOST_LOG_TRIVIAL(warning) << "[BELTRACE] make_perimeters SKIP tid=" << std::this_thread::get_id() << " obj=" << this << " (already started/done)"; + BOOST_LOG_TRIVIAL(trace) << "[BELTRACE] make_perimeters SKIP tid=" << std::this_thread::get_id() << " obj=" << this << " (already started/done)"; return; } - BOOST_LOG_TRIVIAL(warning) << "[BELTRACE] make_perimeters ENTER tid=" << std::this_thread::get_id() << " obj=" << this; + BOOST_LOG_TRIVIAL(trace) << "[BELTRACE] make_perimeters ENTER tid=" << std::this_thread::get_id() << " obj=" << this; m_print->set_status(15, L("Generating walls")); BOOST_LOG_TRIVIAL(info) << "Generating walls..." << log_memory_info(); @@ -533,7 +533,7 @@ void PrintObject::make_perimeters() m_print->throw_if_canceled(); BOOST_LOG_TRIVIAL(debug) << "Generating perimeters in parallel - end"; - BOOST_LOG_TRIVIAL(warning) << "[BELTRACE] make_perimeters EXIT tid=" << std::this_thread::get_id() << " obj=" << this; + BOOST_LOG_TRIVIAL(trace) << "[BELTRACE] make_perimeters EXIT tid=" << std::this_thread::get_id() << " obj=" << this; this->set_done(posPerimeters); } @@ -829,9 +829,9 @@ void PrintObject::detect_overhangs_for_lift() void PrintObject::generate_support_material() { - BOOST_LOG_TRIVIAL(warning) << "[BELTRACE] generate_support_material request tid=" << std::this_thread::get_id() << " obj=" << this; + BOOST_LOG_TRIVIAL(trace) << "[BELTRACE] generate_support_material request tid=" << std::this_thread::get_id() << " obj=" << this; if (this->set_started(posSupportMaterial)) { - BOOST_LOG_TRIVIAL(warning) << "[BELTRACE] generate_support_material ENTER tid=" << std::this_thread::get_id() << " obj=" << this; + BOOST_LOG_TRIVIAL(trace) << "[BELTRACE] generate_support_material ENTER tid=" << std::this_thread::get_id() << " obj=" << this; this->clear_support_layers(); if(!has_support() && !m_print->get_no_check_flag()) { @@ -872,10 +872,10 @@ void PrintObject::generate_support_material() this->_generate_support_material(); m_print->throw_if_canceled(); } - BOOST_LOG_TRIVIAL(warning) << "[BELTRACE] generate_support_material EXIT tid=" << std::this_thread::get_id() << " obj=" << this; + BOOST_LOG_TRIVIAL(trace) << "[BELTRACE] generate_support_material EXIT tid=" << std::this_thread::get_id() << " obj=" << this; this->set_done(posSupportMaterial); } else { - BOOST_LOG_TRIVIAL(warning) << "[BELTRACE] generate_support_material SKIP tid=" << std::this_thread::get_id() << " obj=" << this << " (already started/done)"; + BOOST_LOG_TRIVIAL(trace) << "[BELTRACE] generate_support_material SKIP tid=" << std::this_thread::get_id() << " obj=" << this << " (already started/done)"; } }