juli/OrcaSlicer
1
1
Fork 0
mirror of https://github.com/OrcaSlicer/OrcaSlicer.git synced 2026-10-10 17:21:10 +00:00
Code Issues Packages Projects Releases Wiki Activity

Bound the parallel pressure-advance lookup to the filament count

Browse Source 
Review findings on the preceding commit, plus one defect it should have
caught.

- The IDEX/IQEX pressure-advance loop fed resolve_filament_for_head()'s
  result straight into enable_pressure_advance and pressure_advance. That
  result is bounded by physical_extruder_map, which holds one entry per
  NOZZLE, while both options are indexed per filament SLOT. On a printer
  with more nozzles than the project has filaments the two spaces diverge
  and get_at() clamped the overflow onto filament 0, emitting its pressure
  advance on a secondary carriage. The second-layer temperature loop bounds the
  same lookup, but against nozzle_temperature, which is variant-expanded and so
  is not the slot count either -- it is not the precedent it looks like.
  IMEXHelpers.hpp states the rule
  once, and a test pins the contract that makes the bound necessary:
  resolve_filament_for_head() answers in nozzle space, so a non-negative
  result is not by itself safe to use as a filament id.

- The header claimed every caller renders a -1 tool qualifier as "emit
  none". RepRapFirmware substitutes the historical D0 instead, deliberately
  and with its own comment in GCodeWriter. Say so, rather than leaving a
  contract a future author would code against.

- A cross-reference pointed at a hard-coded line number that the preceding
  commit had itself shifted by nine lines. Name the function instead.

- The multi-color rejection reasons reach the user through Print::validate()
  as raw English, while the returns on either side of them use L(). Wrap
  them and register IMEXHelpers.cpp for extraction. They also still said
  "IMEX", the internal name, so they move to IDEX/IQEX with the rest of the
  user-facing strings rather than shipping the internal one to translators.

- Trim the preceding commit's comments. One block explained the same
  clamping hazard six times; the canonical explanation now lives in
  IMEXHelpers.hpp and the call sites point at it. The mode grid carried
  twelve lines of commentary and no code, most of it archaeology already in
  the commit message, and one claim about the modes editor that was not
  true. The ArrangeJob threading note stays: it documents an invariant that
  cannot be recovered from the code.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Clifford Garwood
2026-09-18 01:32:26 -04:00
co-authored by Claude Opus 5
parent e309029b28
commit 98544fd6bc
7 changed files with 79 additions and 62 deletions
Download Patch File Download Diff File Expand all files Collapse all files
localization/i18n/list.txt
+1
View File
@@ -214,6 +214,7 @@ src/libslic3r/ExtrusionEntity.cpp
src/libslic3r/Flow.cpp
src/libslic3r/Format/AMF.cpp
src/libslic3r/miniz_extension.cpp
src/libslic3r/IMEXHelpers.cpp
src/libslic3r/Preset.cpp
src/libslic3r/Print.cpp
src/libslic3r/PrintBase.cpp
src/libslic3r/GCode.cpp
+18 -15
View File
@@ -3408,7 +3408,7 @@ void GCode::_do_export(Print& print, GCodeOutputStream &file, ThumbnailsGenerato
// pollute is_extruder_used with the *first* slot routed to the primary's
// physical extruder, which is generally not the slot the user assigned to
// the printing object. Same skip-primary pattern as the IMEX PA emission
// path (GCode.cpp:3917).
// path in _do_export().
//
// For secondaries: translate physical -> logical via the per-plate
// imex_head_filament_map (set by the IMEX ghost picker), with
@@ -3418,9 +3418,8 @@ void GCode::_do_export(Print& print, GCodeOutputStream &file, ThumbnailsGenerato
const auto plate_head_map = parse_imex_head_filament_map(
print.objects().front()->config().imex_head_filament_map.value);
const ConfigOptionInts& pem = print.config().physical_extruder_map;
// Bounds-check rather than get_at(), which clamps out-of-range to values.front(). The
// clamped value is used as a skip-primary sentinel below, so a filament id past the end
// of the map would suppress whichever head sits at pem[0]. -1 matches no head.
// Bounds-checked, not get_at() -- see IMEXHelpers.hpp. The value is a skip-primary
// sentinel below, so a clamp would suppress whichever head sits at pem[0].
const int primary_physical =
((int) initial_extruder_id >= 0 &&
(int) initial_extruder_id < (int) pem.values.size())
@@ -3915,22 +3914,29 @@ void GCode::_do_export(Print& print, GCodeOutputStream &file, ThumbnailsGenerato
// loop can skip the primary head (which emitted PA via the normal path).
// Then pem-invert each active physical head back to its first routed filament
// for the PA setting lookup. Guarded on non-empty pem above.
// Bounds-check rather than get_at(), which clamps out-of-range to values.front():
// a clamped initial_physical would make this loop skip whichever active head equals
// pem[0], leaving that head with no PA at all. Same reasoning as the second-layer
// temperature loop below. -1 matches no head, so every active head is emitted.
// Bounds-checked, not get_at() -- see the note on imex_pem_tool_for in
// IMEXHelpers.hpp. A clamped initial_physical would skip whichever active head
// equals pem[0], leaving it with no PA at all.
const int initial_physical =
((int) initial_extruder_id >= 0 &&
(int) initial_extruder_id < (int) m_config.physical_extruder_map.values.size())
? m_config.physical_extruder_map.values[(int) initial_extruder_id]
: -1;
const int num_pa_filament_columns = std::min(
(int) print.config().enable_pressure_advance.values.size(),
(int) print.config().pressure_advance.values.size());
for (int tool_idx : get_imex_active_tools(print)) {
// Unlike the second-layer temperature loop, the primary is skipped here:
// set_extruder() above already emitted its PA with the pem tool qualifier.
if (tool_idx == initial_physical) continue;
const int logical = resolve_filament_for_head(
m_imex_head_filament_map, m_config.physical_extruder_map, tool_idx);
if (logical < 0) continue;
// resolve_filament_for_head() answers in pem's index space -- one entry per
// NOZZLE -- while these two options are indexed per FILAMENT SLOT. The spaces
// diverge when the printer has more nozzles than the project has filaments, and
// get_at() would clamp a past-the-end index to filament 0 and pin its PA onto a
// secondary carriage. Same guard the second-layer temperature loop uses.
if (logical < 0 || logical >= num_pa_filament_columns) continue;
if (!print.config().enable_pressure_advance.get_at(logical)) continue;
file.write(m_writer.set_pressure_advance(
print.config().pressure_advance.get_at(logical),
@@ -5950,12 +5956,9 @@ LayerResult GCode::process_layer(
// transition at all. `tool_idx` is physical; the printing head uses this layer's
// own filament, the parallel carriages resolve through the head map.
const int num_filament_columns = (int)print.config().nozzle_temperature.values.size();
// Bounds-check rather than get_at(): get_at() clamps to values.front(), which would
// make initial_physical the primary's head for any first_extruder_id past the end of
// the map. A secondary that happens to sit on that head would then take the
// "initial" branch below and be given the wrong filament's transition temperature,
// while never receiving its own. -1 matches no tool, so every head takes the
// resolved path instead.
// Bounds-checked, not get_at() -- see IMEXHelpers.hpp. A clamp would hand the
// "initial" branch below to whichever secondary sits on pem[0], giving it the wrong
// filament's transition temperature and never its own.
const int initial_physical =
((int) first_extruder_id >= 0 &&
(int) first_extruder_id < (int) m_config.physical_extruder_map.values.size())
src/libslic3r/IMEXHelpers.cpp
+27 -28
View File
@@ -11,9 +11,13 @@
#include <boost/log/trivial.hpp>
#include "libslic3r/ClipperUtils.hpp"
#include "libslic3r/I18N.hpp"
#include "libslic3r/PresetBundle.hpp"
#include "libslic3r/PrintConfig.hpp"
// Mark string for localization and translate.
#define L(s) Slic3r::I18N::translate(s)
namespace Slic3r {
namespace {
@@ -60,14 +64,9 @@ int imex_pem_tool_for(int filament_id, const std::string& parallel_mode, const C
const bool imex_parallel = !parallel_mode.empty() && parallel_mode != kImexPrimaryMode;
if (!imex_parallel || pem.values.empty())
return -1;
// Bounds-check rather than get_at(), for the same reason imex_physical_heater_for() below
// does: get_at() CLAMPS to values.front(), so a filament id past the end of the map would
// silently address physical head pem[0] instead of reporting "no mapping". The map is one
// entry per NOZZLE (effective_physical_extruder_map), while filament ids index filament
// SLOTS, and nothing caps the slot count at the nozzle count -- set_num_filaments() takes
// its size from the project, so opening a project authored with more filaments than this
// printer has extruders leaves ids past the end. Emitting no tool qualifier is correct
// there; pinning PA onto the primary's carriage is not.
// Bounds-checked, not get_at() -- see the note in IMEXHelpers.hpp for why the two index
// spaces diverge. Emitting no tool qualifier is correct for a miss; pinning PA onto the
// primary's carriage is not.
if (filament_id < 0 || filament_id >= (int) pem.values.size())
return -1;
return pem.values[filament_id];
@@ -109,11 +108,11 @@ std::string imex_multicolor_block_reason(const std::string& parallel_mode,
if (filament < 0 || filament >= (int)pem.values.size()) continue;
const int phys = pem.values[filament]; // bounds-checked above; get_at would clamp
if (!seen_physicals.insert(phys).second) {
return "Multi-color prints in IMEX parallel modes require each filament "
"to have its own dedicated physical extruder. Two or more of the active "
"filaments are routed to the same physical head via the printer's "
"physical extruder map (an MMU/AFC manifold), which the slaved gantry "
"cannot follow.";
return L("Multi-color prints in IDEX/IQEX parallel modes require each filament "
"to have its own dedicated physical extruder. Two or more of the active "
"filaments are routed to the same physical head via the printer's "
"physical extruder map (an MMU/AFC manifold), which the slaved gantry "
"cannot follow.");
}
}
}
@@ -121,9 +120,9 @@ std::string imex_multicolor_block_reason(const std::string& parallel_mode,
// Determine which physical head is the primary in this mode.
const int primary_physical = imex_primary_tool_for_mode(active_tools_str);
if (primary_physical < 0) {
return "The active IMEX mode does not define a primary tool, so multi-color "
"printing cannot be scheduled. Open the printer settings IMEX Modes "
"editor and assign a Primary role to one tool.";
return L("The active IDEX/IQEX mode does not define a primary tool, so multi-color "
"printing cannot be scheduled. Open the printer settings IDEX/IQEX Modes "
"editor and assign a Primary role to one tool.");
}
// Walk the active tools once: collect the set of distinct gantries spanned by
@@ -149,11 +148,11 @@ std::string imex_multicolor_block_reason(const std::string& parallel_mode,
// (use Primary mode for that), and the user's mode_gcode would still emit
// parallel-print firmware setup that doesn't apply here.
if (active_gantries.size() < 2) {
return "Multi-color in this mode isn't a parallel-print scenario — all of the "
"active tools sit on a single gantry, so there's no second gantry being "
"copied or mirrored to. Switch the plate to Primary mode for multi-color "
"printing on a single gantry, or define an IMEX mode that includes "
"tools on a second gantry.";
return L("Multi-color in this mode isn't a parallel-print scenario — all of the "
"active tools sit on a single gantry, so there's no second gantry being "
"copied or mirrored to. Switch the plate to Primary mode for multi-color "
"printing on a single gantry, or define an IDEX/IQEX mode that includes "
"tools on a second gantry.");
}
// Dual-gantry mode but no Span tool on the primary's gantry — the slicer would
@@ -162,11 +161,11 @@ std::string imex_multicolor_block_reason(const std::string& parallel_mode,
// copies (each gantry tool prints its own object) which is incompatible with
// mid-print multicolor in a parallel mode.
if (!span_on_primary_gantry) {
return "Multi-color prints in IMEX parallel modes require a Span tool on the "
"primary's gantry — without one, the mode doesn't declare a within-gantry "
"multicolor partner. Either reduce the print to a single filament, switch to "
"Primary mode, or open the printer settings IMEX Modes editor and mark a "
"tool on the primary's gantry as Span.";
return L("Multi-color prints in IDEX/IQEX parallel modes require a Span tool on the "
"primary's gantry — without one, the mode doesn't declare a within-gantry "
"multicolor partner. Either reduce the print to a single filament, switch to "
"Primary mode, or open the printer settings IDEX/IQEX Modes editor and mark a "
"tool on the primary's gantry as Span.");
}
return {};
}
@@ -507,8 +506,8 @@ int resolve_filament_for_head(const std::map<int,int>& plate_map,
auto it = plate_map.find(physical);
if (it != plate_map.end()) {
const int zero_based = it->second - 1;
// pem has one entry per logical filament slot, so its size IS the slot count and
// an override outside it names a filament that does not exist. Bounding here rather
// pem is indexed by filament slot here, so an override outside it names a filament
// this printer cannot route. Bounding here rather
// than at the parse site is deliberate: the parser is handed a raw string with no
// notion of how many filaments the project has, while every consumer of this
// function's result indexes a per-filament array. The picker only ever offers
src/libslic3r/IMEXHelpers.hpp
+10 -1
View File
@@ -118,7 +118,16 @@ std::vector<ImexMode> imex_mode_table(const ConfigBase& cfg);
// answering "physical head pem[0]" for a logical id that has no mapping at all. The map is one
// entry per nozzle while logical ids index filament slots, and nothing caps the slot count at
// the nozzle count, so out-of-range is reachable. Bounds-check and treat the miss as "no
// mapping" (-1 for a tool qualifier, which every caller renders as "emit none").
// mapping" (-1).
//
// The same nozzle-vs-slot divergence bites in the other direction: anything derived from pem
// -- `resolve_filament_for_head`, `first_filament_for_physical_head` -- answers in NOZZLE index
// space, so bound it against the filament array you are about to index before using it as a
// filament id.
//
// -1 as a tool qualifier reaches GCodeWriter::set_pressure_advance, which omits the qualifier
// on Klipper, Marlin and BBL but substitutes the historical `D0` on RepRapFirmware
// (GCodeWriter.cpp, set_pressure_advance) -- deliberate, not a miss.
//
// Past bugs in this class:
// - GCode PA emission used the inline `pem.get_at(filament_id)` form at two
src/slic3r/GUI/IMEXFilamentPickerPopover.hpp
+3 -6
View File
@@ -26,12 +26,9 @@ public:
void popup_at_cursor();
private:
// wxPopupTransientWindow::Dismiss() only hides the window; it does not destroy it, and it
// does not call OnDismiss() either -- only DismissAndNotify() does. The ghost-click handler
// creates one popover per click and keeps no reference, so without destroying here every
// click would leak a live top-level window (with its BitmapComboBox, bitmaps and event
// bindings) parented to the GL canvas. The filament-selected handler therefore calls
// DismissAndNotify(), not Dismiss(), or it would bypass this entirely.
// Destroys the popover: the ghost-click handler creates one per click and keeps no
// reference, so every click would otherwise leak a live top-level window parented to the
// GL canvas. Only DismissAndNotify() reaches this -- plain Dismiss() just hides.
void OnDismiss() override;
void build_row();
void on_filament_selected(int slot_1_based);
src/slic3r/GUI/IMEXModesCtrl.cpp
+4 -12
View File
@@ -408,18 +408,10 @@ void IMEXModesCtrl::add_row(const std::string& name,
// raw_row = flip_y ? (n_rows-1-row) : row
// raw_col = flip_x ? (n_cols-1-col) : col
//
// The displayed window is always the whole grid: m_n_rows is the gantry count, so valid tool
// indices are 0 .. m_n_rows*m_n_cols-1 and rows 0 .. m_n_rows-1. A window m_n_rows tall can
// only lie entirely inside the grid when it starts at row 0.
//
// This used to anchor the window's first row to the Primary's gantry row, to "keep the
// meaningful row visible" when the gantry count shrank. That cannot work: shifting a
// full-height window forward walks it off the end, so it drew tiles for tools that do not
// exist and hid real ones, and clicking a phantom tile wrote a tool index that
// compute_imex_zone_layout() (IMEXZones.cpp:76) then discards. A Primary sitting outside the
// current grid is a data problem -- only reachable from a hand-authored mode string, since
// the editor will not move Primary off tool 0 -- and the zone layout already answers it by
// producing no zones at all. Showing the real grid is the honest rendering of that state.
// Render the whole grid: m_n_rows is the gantry count, so the valid tool indices are
// 0 .. m_n_rows*m_n_cols-1 and a window this tall only fits inside the grid at row 0.
// A Primary outside the grid is a data problem the zone layout already reports by
// producing no zones (compute_imex_zone_layout, IMEXZones.cpp).
bool flip_x = (m_layout == 1 || m_layout == 3);
bool flip_y = (m_layout == 2 || m_layout == 3);
tests/libslic3r/test_imex_helpers.cpp
+16
View File
@@ -1520,3 +1520,19 @@ TEST_CASE("imex_pem_tool_for - a filament id past the end of the map has no tool
REQUIRE(imex_pem_tool_for(9, "copy_mode", pem) == -1);
REQUIRE(imex_pem_tool_for(-1, "copy_mode", pem) == -1);
}
TEST_CASE("resolve_filament_for_head answers in nozzle index space, not filament slots", "[IMEX]") {
// The contract that callers get wrong: the returned index is bounded by pem's length --
// one entry per NOZZLE -- and NOT by the number of filaments the project has. A caller that
// feeds this straight into a per-filament option must bound it first, or get_at() clamps
// the overflow onto filament 0 (see GCode.cpp's IMEX pressure-advance loop, which does).
const auto pem = make_pem({0, 1, 2, 3}); // 4 nozzles, identity routing
// Head 3 resolves to index 3 even for a 2-filament project: nothing here knows the
// filament count, so the result can legitimately exceed it.
REQUIRE(resolve_filament_for_head({}, pem, 3) == 3);
REQUIRE(resolve_filament_for_head({}, pem, 2) == 2);
// Only a head with no routing at all yields -1, so "-1 means safe to index" is false.
REQUIRE(resolve_filament_for_head({}, pem, 9) == -1);
}