Summary
Round-2 /slicer-review of PR #393 surfaced 4 minor non-blocking items that were carried as a follow-up rather than holding up the merge. All are small, single-purpose patches. See #393 (comment) for the synthesis comment.
Items
🟡 (1) ReadXMLAttributes bypasses ValidateKnotsClampedMonotonic
File: LiverResections/MRML/vtkMRMLNurbsSurfaceNode.cxx:694-721.
ReadXMLAttributes directly assigns this->KnotsU = values / this->KnotsV = values on a length match without invoking ValidateKnotsClampedMonotonic. The validator covers SetKnotsU / SetKnotsV / ReadJsonNurbs but the XML path has a back-door. Non-blocking because the XML path is legacy (.lrp.json is the v2.1 documented storage format), but worth a small patch:
- Call the validator on the deserialised values.
- On failure:
vtkWarningMacro + fall back to ResetKnotsToClampedUniform().
🟡 (2) Class-level docstring lumps shape and degree setters
File: LiverResections/MRML/vtkMRMLNurbsSurfaceNode.h — \par Shape-change side effects block.
The docstring says all 6 setters (SetRows, SetCols, SetSize, SetDegreeU, SetDegreeV, SetDegree) regenerate Weights + ControlGrid + Knots. In code, SetDegreeU / SetDegreeV / SetDegree only touch knots — Rows*Cols is unchanged so Weights and ControlGrid lengths stay valid + their values are preserved.
Fix: split the paragraph into shape-setter (regen all three) vs degree-setter (regen knots only) groups.
🟢 (3) Missing positive hand-rolled-JSON testJsonReadV3NurbsValid
File: LiverResections/MRML/Testing/Cxx/vtkMRMLBezierSurfaceStorageNodeTest1.cxx.
The existing testJsonRoundTripNurbs is write-then-read; it cannot catch a writer that silently produces a malformed schema (e.g., wrong field name, swapped axis-order semantics). Add a sibling testJsonReadV3NurbsValid that writes a known-good v3 JSON fixture by hand + reads it through ReadJson + asserts the resulting IVars. Twin of the existing testJsonReadV3BezierExplicit at line 824.
🟢 (4) Unclamped-end branch of ValidateKnotsClampedMonotonic is uncovered
File: LiverResections/MRML/Testing/Cxx/vtkMRMLNurbsSurfaceNodeTest1.cxx.
testSetKnotsRejectsNonMonotonic covers the unclamped-start path (first clamping loop in the helper) but the unclamped-end path (second clamping loop, knots size-degree-1 .. size-1 not all equal) is untested. Add one fixture unclampedEnd — a knot vector that is clamped at start, non-decreasing, but with the last degree+1 values not all equal. Assert ValidateKnotsClampedMonotonic returns false + populates error.
Priority
Low. Items 1-2 are small in-PR-style polish; 3-4 are test-coverage strengthening. All carry-able as a single cleanup PR (~50 LOC total).
References
Filed autonomously after the round-2 fix-review cycle on PR #393.
Summary
Round-2
/slicer-reviewof PR #393 surfaced 4 minor non-blocking items that were carried as a follow-up rather than holding up the merge. All are small, single-purpose patches. See #393 (comment) for the synthesis comment.Items
🟡 (1)
ReadXMLAttributesbypassesValidateKnotsClampedMonotonicFile:
LiverResections/MRML/vtkMRMLNurbsSurfaceNode.cxx:694-721.ReadXMLAttributesdirectly assignsthis->KnotsU = values/this->KnotsV = valueson a length match without invokingValidateKnotsClampedMonotonic. The validator coversSetKnotsU/SetKnotsV/ReadJsonNurbsbut the XML path has a back-door. Non-blocking because the XML path is legacy (.lrp.jsonis the v2.1 documented storage format), but worth a small patch:vtkWarningMacro+ fall back toResetKnotsToClampedUniform().🟡 (2) Class-level docstring lumps shape and degree setters
File:
LiverResections/MRML/vtkMRMLNurbsSurfaceNode.h—\par Shape-change side effectsblock.The docstring says all 6 setters (
SetRows,SetCols,SetSize,SetDegreeU,SetDegreeV,SetDegree) regenerate Weights + ControlGrid + Knots. In code,SetDegreeU/SetDegreeV/SetDegreeonly touch knots —Rows*Colsis unchanged so Weights and ControlGrid lengths stay valid + their values are preserved.Fix: split the paragraph into shape-setter (regen all three) vs degree-setter (regen knots only) groups.
🟢 (3) Missing positive hand-rolled-JSON
testJsonReadV3NurbsValidFile:
LiverResections/MRML/Testing/Cxx/vtkMRMLBezierSurfaceStorageNodeTest1.cxx.The existing
testJsonRoundTripNurbsis write-then-read; it cannot catch a writer that silently produces a malformed schema (e.g., wrong field name, swapped axis-order semantics). Add a siblingtestJsonReadV3NurbsValidthat writes a known-good v3 JSON fixture by hand + reads it throughReadJson+ asserts the resulting IVars. Twin of the existingtestJsonReadV3BezierExplicitat line 824.🟢 (4) Unclamped-end branch of
ValidateKnotsClampedMonotonicis uncoveredFile:
LiverResections/MRML/Testing/Cxx/vtkMRMLNurbsSurfaceNodeTest1.cxx.testSetKnotsRejectsNonMonotoniccovers the unclamped-start path (first clamping loop in the helper) but the unclamped-end path (second clamping loop, knotssize-degree-1 .. size-1not all equal) is untested. Add one fixtureunclampedEnd— a knot vector that is clamped at start, non-decreasing, but with the lastdegree+1values not all equal. AssertValidateKnotsClampedMonotonicreturns false + populateserror.Priority
Low. Items 1-2 are small in-PR-style polish; 3-4 are test-coverage strengthening. All carry-able as a single cleanup PR (~50 LOC total).
References
/slicer-reviewsynthesis: ENH: Add vtkMRMLNurbsSurfaceNode + .lrp.json schema v3 (NURBS-1) #393 (comment)Filed autonomously after the round-2 fix-review cycle on PR #393.