fix: all three PA calibration tests fail to print correctly - #231
Open
bugparty wants to merge 2 commits into
Open
fix: all three PA calibration tests fail to print correctly#231bugparty wants to merge 2 commits into
bugparty wants to merge 2 commits into
Conversation
The PA Line, PA Pattern and PA Tower calibrations all produced a wrong
EXCLUDE_OBJECT_DEFINE for the calibration area, and aborted outright on
Debug builds.
In GCode::set_object_info(), `first_v` holds the polygon's first vertex so
the ring can be closed at the end, exactly as polygon_to_string() does at
GCode.cpp:8155. Inside the loop it was then "assigned" with:
first_v(v.x() + ext_x, v.y());
On an already-constructed Vec2d that is not assignment -- it resolves to
Eigen's two-dimensional coefficient accessor
Scalar& DenseCoeffsBase<Derived, WriteAccessors>::operator()(Index, Index)
with both doubles truncated to indices. For a 2x1 vector only (0,0) and
(1,0) are valid, so real bed coordinates blow the bounds assertion:
Assertion `row >= 0 && row < rows() && col >= 0 && col < cols()' failed.
Under NDEBUG the assertion is compiled out and the expression forms an
out-of-bounds reference that is immediately discarded, so `first_v` was
never actually written. The statement is dead code in every release build.
Remove it and make `first_v` const, matching polygon_to_string(). The
closing vertex must be vertex 0; had the statement been a real assignment
it would have left `first_v` at vertex 3, emitting a duplicated final point
and an unclosed polygon. Behaviour is therefore unchanged for release
builds, and Debug builds no longer abort.
Verified on Fedora against all three PA calibration modes.
Claude-Session: https://claude.ai/code/session_01GX1HqJrRdEWvoiwXkfrLQH
bugparty
force-pushed
the
fix/pa-calib-eigen-oob-crash
branch
from
September 6, 2026 06:38
ca36783 to
8e28fc3
Compare
Docstrings generation was requested by @bugparty. The following files were modified: * `src/libslic3r/GCode.cpp`
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
All three Pressure Advance calibrations — PA Line, PA Pattern and PA Tower — currently fail to print correctly. Debug builds abort outright:
Cause
GCode::set_object_info()emits anEXCLUDE_OBJECT_DEFINEdescribing the area actually covered by the calibration pattern.first_vholds the polygon's first vertex so the ring can be closed at the end — the same role it plays inpolygon_to_string()atGCode.cpp:8155, where it is correctly declaredconst auto.Inside the loop it was then "assigned" with:
first_v(v.x() + ext_x, v.y());On an already-constructed
Vec2dthis is not assignment. It resolves to Eigen's two-dimensional coefficient accessorScalar& DenseCoeffsBase<Derived, WriteAccessors>::operator()(Index row, Index col)with both
doubles truncated to indices.Vec2disEigen::Matrix<double, 2, 1, DontAlign>— matchingMatrix<double, 2, 1, 2>in the assertion — so only(0,0)and(1,0)are in range. Real bed coordinates are not, and the bounds assertion fires.Under
NDEBUGthe assertion is compiled out and the expression merely forms an out-of-bounds reference that is immediately discarded, sofirst_vwas never actually written. The statement is dead code in every release build.Fix
Remove the statement and declare
first_vconst, matchingpolygon_to_string().The closing vertex must be vertex 0. Had the statement been a real assignment it would have left
first_vat vertex 3 (the branch runs fori == 0 || i == 3), emitting a duplicated final point and an unclosed polygon — strictly worse than the current release behaviour. Making itconstalso lets the compiler reject this class of misuse at compile time.Release behaviour is therefore unchanged, and Debug builds no longer abort.
Verification
Built on Fedora and exercised all three PA calibration modes; all now print correctly, where previously all three were broken.
https://claude.ai/code/session_01GX1HqJrRdEWvoiwXkfrLQH