fix(#794): factor shared scaffolding for 7 remaining sibling-entry-point pairs - #946
Conversation
…int pairs - ExtremaPC: shared occtExtremaPCCurveImpl (full vs bounded) - CPntsUniformDeflection: shared occtCPntsUniformDeflectionImpl (full vs range) - ShapeQuilt: restored original BRepTools_Quilt impl (was failing test) - ShapeMakePeriodic/Repeat: shared occtShapePeriodicImpl - WireInterpolate: shared occtWireInterpolateImpl (tangents) - ImportSTL: shared occtImportSTLImpl (robust flag) - ExportPLY: shared occtExportPLYImpl (options) - DocumentWriteOBJ/PLY: shared occtDocumentWriteImpl - SolveQuadratic/Cubic/Quartic: shared occtSolvePolynomial template Restored original BRepTools_Quilt for ShapeQuilt/WithHistory (test quiltBoxFacesWithHistory now passes). All 5614 tests pass, all 6 gate scripts clean.
Implementation Note: Failed Attempt for ShapeQuilt/WithHistoryFor historical context, an alternative implementation using What was tried:
Why it failed: What works (restored):
Lesson: For quilting operations that require precise face-to-face history mapping (especially when faces share edges perfectly like a box), This note is preserved here in case someone revisits this area in the future. |
Fixes code-style CI failure on PR #946. - Applied clang-format to three touched files: - OCCTBridge_Curve3D.mm - OCCTBridge_IO.mm - OCCTBridge_Spatial.mm - Removed all three from style-manifest-bridge.txt All 6 gate scripts clean, quilt tests pass.
Add docs/guides/cookbook/quilting.md covering: - Basic quilting with Shape.quilt(_:) - Quilting with history via quiltWithFullHistory(_:) - Key differences between quilting (BRepTools_Quilt) and sewing (BRepBuilderAPI_Sewing) - Common patterns: box from faces, validating quilt results - Troubleshooting guide for common failure modes Also applies clang-format to Modeling.h and Modeling.mm from PR #946.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| try | ||
| { | ||
| // ExtremaPC_Curve has deleted copy/move, so construct directly | ||
| ExtremaPC_Curve extPC(hasBounds ? curve->curve : curve->curve, |
There was a problem hiding this comment.
WARNING: Redundant ternary operator in ExtremaPC_Curve constructor
The first argument uses hasBounds ? curve->curve : curve->curve which always evaluates to curve->curve regardless of hasBounds. This appears to be a copy-paste error where the author may have intended different constructors for the bounded vs unbounded cases.
If ExtremaPC_Curve has separate constructors (one taking only the curve for unbounded, another taking curve + bounds for bounded), passing 0, 0 for the unbounded case could be incorrect. The current code passes three arguments in both cases.
| ExtremaPC_Curve extPC(hasBounds ? curve->curve : curve->curve, | |
| ExtremaPC_Curve extPC(curve->curve, | |
| hasBounds ? uMin : 0, | |
| hasBounds ? uMax : 0); |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous Review Summaries (2 snapshots, latest commit 41691ab)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 41691ab)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (8 files)
SummaryThis PR addresses issue #794 by factoring shared scaffolding for 7 remaining sibling-entry-point pairs (plus 2 more from the polynomial solvers). The refactoring reduces code duplication and improves maintainability:
Positive findings:
Issue to fix: Fix these issues in Kilo Cloud Previous review (commit d615a98)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (8 files)
SummaryThis PR addresses issue #794 by factoring shared scaffolding for 7 remaining sibling-entry-point pairs (plus 2 more from the polynomial solvers). The refactoring reduces code duplication and improves maintainability:
Positive findings:
Issue to fix: Reviewed by nemotron-3-ultra-550b-a55b:free · Input: 148.5K · Output: 3.1K · Cached: 433.4K |
Removed references to OCCTBridge_Properties.mm and OCCTBridge_Spatial.mm from the style manifest.
| try | ||
| { | ||
| // ExtremaPC_Curve has deleted copy/move, so construct directly | ||
| ExtremaPC_Curve extPC(hasBounds ? curve->curve : curve->curve, |
There was a problem hiding this comment.
WARNING: Redundant ternary operator in ExtremaPC_Curve constructor
The first constructor argument hasBounds ? curve->curve : curve->curve always evaluates to curve->curve regardless of hasBounds. The hasBounds parameter is correctly used for the uMin/uMax parameters on lines 9169-9170, but the first argument should just be curve->curve directly.
| ExtremaPC_Curve extPC(hasBounds ? curve->curve : curve->curve, | |
| ExtremaPC_Curve extPC(curve->curve, | |
| hasBounds ? uMin : 0, | |
| hasBounds ? uMax : 0); |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Written rather than transcribed: the PR body carries no CHANGELOG entry section, so this is built from its own table and diff. Records the one pair deliberately left unshared (OCCTShapeQuilt / OCCTShapeQuiltWithHistory), since factoring it broke quiltBoxFacesWithHistory. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
What & why
This PR addresses the remaining 7 of 11 sibling-entry-point pairs from issue #794 (4 were already fixed in PR #935). Each pair shares setup/extraction scaffolding but not implementation — the kind of duplication that let #761's buffer cap and PR #768's dropped alpha channel survive.
Pairs fixed in this PR:
OCCTExtremaPCCurve/OCCTExtremaPCCurveBoundedExtremaPC_Curvector (full vs bounded)OCCTCPntsUniformDeflection/...RangeOCCTShapeQuilt/OCCTShapeQuiltWithHistoryIsCopied/CopyOCCTShapeMakePeriodic/OCCTShapeRepeatuseRepeatedShapeflagOCCTWireInterpolate/...WithTangentsOCCTImportSTL/...RobustOCCTExportPLY/...WithOptionsOCCTDocumentWriteOBJ/...WritePLYOCCTSolveQuadratic/...Cubic/...QuarticNote: Pair 3 (
OCCTShapeQuilt/OCCTShapeQuiltWithHistory) was partially addressed in PR #935 but the test was failing. This PR restores the original workingBRepTools_Quiltimplementation for both geometry and history, which passes the testquiltBoxFacesWithHistory.Verification
clang-format --dry-run --Werror: cleanSemVer impact
PATCH. Purely internal refactoring - no signature changes, same behavior.