mirror of
https://github.com/OrcaSlicer/OrcaSlicer.git
synced 2026-09-18 14:32:36 +00:00
CAD: Axis PlaneIntersection read its plane refs in the wrong index space
axis_plane_a/b are filled by the GUI from populate_plane_choices(), whose rows
are XY / XZ / YZ followed by the datum planes, and the row is stored verbatim.
The kernel's base_plane() indexed datum_planes[ref] directly, so the two spaces
were off by three: picking XY resolved to datum plane 0, and picking the first
datum ran past the end and failed with "plane ref not found". The
PlaneIntersection axis type could not work from the GUI at all.
base_plane() now uses the encoding CadFeature::plane_base already uses — 0/1/2
are the base planes through the modeling origin, >=3 indexes datum_planes[ref-3]
— so there is one convention for plane references instead of two. That also
makes two base planes usable, which the previous code rejected as out of scope
even though XY x XZ is an ordinary way to define the X axis.
Removed the dead find_plane lambda directly above it. It was never called and
half-anticipated this exact offset ("if (ref >= 3) // base plane offset"),
which is presumably where the confusion started.
Tests: the existing parallel-planes case encoded the OLD convention, passing
axis_plane_a = 0 to mean "datum 0" — values the GUI cannot produce — so it is
re-based onto rows 3 and 4. Two new cases cover what the GUI actually emits:
base x base (XY x XZ -> X) and base x datum, the latter pinning the +3 offset.
Both verified to FAIL against the previous indexing, at test_caddocument.cpp
:2325 and :2346.
Found by auditing the remaining tools for the index-space defect class that had
already produced three bugs in the GUI; this is the first instance of it
crossing the GUI/kernel boundary.
Suite 143 cases / 1980 assertions (was 141/1972). GUI compiles clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
0455b0bf96
commit
4d331099de
@@ -1589,26 +1589,27 @@ std::vector<CadDocument::DatumAxis> CadDocument::resolve_datum_axes() const
|
||||
break;
|
||||
}
|
||||
case AxisType::PlaneIntersection: {
|
||||
auto find_plane = [&](int ref) -> const SketchPlane* {
|
||||
if (ref >= 0 && ref < int(datum_planes.size()))
|
||||
return &datum_planes[ref].second;
|
||||
if (ref >= 3) { // base plane offset: 0=XY,1=XZ,2=YZ
|
||||
da.error = "plane ref index out of range (datum planes not found)";
|
||||
return nullptr;
|
||||
}
|
||||
return nullptr;
|
||||
};
|
||||
// For base planes we handle directly.
|
||||
// Same reference encoding as CadFeature::plane_base, because the GUI fills these
|
||||
// two fields from populate_plane_choices() — whose rows are XY/XZ/YZ followed by
|
||||
// the datum planes — and stores the row verbatim. Indexing datum_planes[ref]
|
||||
// directly, as this did, is off by three: picking XY resolved to datum plane 0 and
|
||||
// picking the first datum ran off the end with "plane ref not found", so the
|
||||
// PlaneIntersection axis type could not work at all. Two base planes are also a
|
||||
// perfectly ordinary way to define an axis (XY x XZ = the X axis), so they must
|
||||
// resolve rather than be rejected as out of scope.
|
||||
auto base_plane = [&](int ref, Vec3d& origin, Vec3d& normal) -> bool {
|
||||
if (ref >= 0 && ref < int(datum_planes.size())) {
|
||||
origin = datum_planes[ref].second.origin;
|
||||
normal = datum_planes[ref].second.normal;
|
||||
return true;
|
||||
}
|
||||
return false;
|
||||
SketchPlane p;
|
||||
if (ref == 0) { p = SketchPlane::XY(); p.origin += modeling_origin; }
|
||||
else if (ref == 1) { p = SketchPlane::XZ(); p.origin += modeling_origin; }
|
||||
else if (ref == 2) { p = SketchPlane::YZ(); p.origin += modeling_origin; }
|
||||
else if (ref >= 3 && ref - 3 < int(datum_planes.size()))
|
||||
p = datum_planes[ref - 3].second; // datums are already world-space
|
||||
else
|
||||
return false;
|
||||
origin = p.origin;
|
||||
normal = p.normal;
|
||||
return true;
|
||||
};
|
||||
// Both refs reference datum plane indices in the resolved list.
|
||||
// Supporting cross-base-plane where ref < 0 isn't in scope.
|
||||
bool ok0 = base_plane(f.axis_plane_a, da.origin, da.direction); // direction reused as normal0
|
||||
Vec3d origin1, normal1;
|
||||
bool ok1 = base_plane(f.axis_plane_b, origin1, normal1);
|
||||
|
||||
@@ -2396,20 +2396,63 @@ TEST_CASE("datum axis: degenerate two identical points fails cleanly", "[CadDocu
|
||||
TEST_CASE("datum axis: two parallel planes fail with error", "[CadDocument]")
|
||||
{
|
||||
CadDocument doc;
|
||||
// Two offset XY planes are parallel -> no intersection
|
||||
// Two offset XY planes are parallel -> no intersection.
|
||||
// 3 and 4, not 0 and 1: axis_plane_a/b use the same encoding as plane_base — 0/1/2 are the
|
||||
// XY/XZ/YZ base planes and datums start at 3 — because the GUI fills these from
|
||||
// populate_plane_choices() and stores the row verbatim.
|
||||
doc.add_plane(0 /*XY*/, 10.0, 0.0, 0, "PlaneA");
|
||||
doc.add_plane(0 /*XY*/, 30.0, 0.0, 0, "PlaneB");
|
||||
|
||||
int ax = doc.add_axis(AxisType::TwoPoints, "Parallel");
|
||||
doc.features[ax].axis_type = AxisType::PlaneIntersection;
|
||||
doc.features[ax].axis_plane_a = 0;
|
||||
doc.features[ax].axis_plane_b = 1;
|
||||
doc.features[ax].axis_plane_a = 3;
|
||||
doc.features[ax].axis_plane_b = 4;
|
||||
|
||||
auto axes = doc.resolve_datum_axes();
|
||||
REQUIRE(axes.size() == 1);
|
||||
REQUIRE_FALSE(axes[0].error.empty());
|
||||
}
|
||||
|
||||
// The case the GUI actually produces most often, and which could not work before: the two refs
|
||||
// are rows of the plane picker, whose first three entries are the base planes. XY x XZ is the
|
||||
// X axis. Previously ref 0 was read as "datum plane 0", so this silently resolved to the wrong
|
||||
// plane or failed with "plane ref not found".
|
||||
TEST_CASE("datum axis: intersection of two BASE planes gives the expected axis", "[CadDocument]")
|
||||
{
|
||||
using Catch::Matchers::WithinAbs;
|
||||
|
||||
CadDocument doc;
|
||||
int ax = doc.add_axis(AxisType::PlaneIntersection, "XAxis");
|
||||
doc.features[ax].axis_plane_a = 0; // XY
|
||||
doc.features[ax].axis_plane_b = 1; // XZ
|
||||
|
||||
auto axes = doc.resolve_datum_axes();
|
||||
REQUIRE(axes.size() == 1);
|
||||
REQUIRE(axes[0].error.empty());
|
||||
// XY normal is Z, XZ normal is Y; Z x Y is +/-X.
|
||||
CHECK_THAT(std::abs(axes[0].direction.x()), WithinAbs(1.0, 1e-12));
|
||||
CHECK_THAT(axes[0].direction.y(), WithinAbs(0.0, 1e-12));
|
||||
CHECK_THAT(axes[0].direction.z(), WithinAbs(0.0, 1e-12));
|
||||
}
|
||||
|
||||
// A datum plane crossed with a base plane — the mixed case, which pins the +3 offset.
|
||||
TEST_CASE("datum axis: base plane crossed with a datum plane", "[CadDocument]")
|
||||
{
|
||||
using Catch::Matchers::WithinAbs;
|
||||
|
||||
CadDocument doc;
|
||||
doc.add_plane(1 /*XZ*/, 5.0, 0.0, 0, "OffsetXZ"); // datum 0 -> row 3
|
||||
|
||||
int ax = doc.add_axis(AxisType::PlaneIntersection, "MixedAxis");
|
||||
doc.features[ax].axis_plane_a = 0; // XY base
|
||||
doc.features[ax].axis_plane_b = 3; // the datum above
|
||||
|
||||
auto axes = doc.resolve_datum_axes();
|
||||
REQUIRE(axes.size() == 1);
|
||||
REQUIRE(axes[0].error.empty());
|
||||
CHECK_THAT(std::abs(axes[0].direction.x()), WithinAbs(1.0, 1e-12));
|
||||
}
|
||||
|
||||
TEST_CASE("datum axis: cylinder centreline from extruded circle", "[CadDocument]")
|
||||
{
|
||||
using Catch::Matchers::WithinAbs;
|
||||
|
||||
Reference in New Issue
Block a user