feat: F-CP structural CurvePolygon (Option A) + enable F-MC/F-MS#1198
Open
grootstebozewolf wants to merge 14 commits into
Open
feat: F-CP structural CurvePolygon (Option A) + enable F-MC/F-MS#1198grootstebozewolf wants to merge 14 commits into
grootstebozewolf wants to merge 14 commits into
Conversation
grootstebozewolf
force-pushed
the
feature/sfa-curve-F-CP-option-a
branch
from
43d24f7 to
820b0f4
Compare
Introduce minimal, surgical seams in jts-core so an opt-in extension module (jts-curved) can plug in support for CircularString, CompoundCurve, CurvePolygon, MultiCurve, MultiSurface, Triangle, PolyhedralSurface, and Tin without any new geometry classes or algorithm changes in core. Modelled on NetTopologySuite PR locationtech#526, which applies the same pattern in the .NET sister project. WKTReader - Add protected `readOtherGeometryText(tokenizer, type, ordinateFlags)` hook called when a type keyword is unrecognised. Default impl preserves the prior `Unknown geometry type` ParseException. - Promote tokenizer helpers from `private` to `protected`: getCoordinate, getCoordinateSequence, createCoordinateSequenceEmpty, getNextEmptyOrOpener, lookAheadWord, getNextCloserOrComma, getNextWord, parseErrorWithLine, readGeometryTaggedText (3-arg), readLineStringText, readLinearRingText, readPolygonText, readMultiPolygonText. - Promote `geometryFactory` and `csFactory` fields to protected. WKTWriter - Add protected `appendOtherGeometryTaggedText(...)` hook called early in the dispatch ladder so subclasses can intercept geometries that would otherwise be routed to a parent class's handler by `instanceof`. Default returns false, preserving prior behaviour. - Replace four hard-coded `WKTConstants.*` keyword writes with `geometry.getGeometryType().toUpperCase(Locale.ROOT)`, so subclasses with structurally compatible bodies emit their own keyword without needing dedicated dispatch branches. No-op for existing types. - Promote helpers to protected: indent, appendOrdinateText, appendSequenceText, appendPolygonText, appendMultiLineStringText, appendMultiPolygonText. WKTConstants - Add CIRCULARSTRING, COMPOUNDCURVE, CURVEPOLYGON, MULTICURVE, MULTISURFACE, POLYHEDRALSURFACE, TIN, TRIANGLE constants. Core readers and writers do not handle these directly; they are exposed here as a single canonical set for extension modules and downstream tooling. Backward compatibility - All additions are new methods or new protected modifiers on non-final classes. No removals, no signature changes. - The full jts-core suite (2282 tests) passes unchanged.
Adds an opt-in module that implements the OGC SFA / ISO 19125-2
extended geometry types on top of the extension hooks added to
jts-core in the prior commit.
Geometry types (in org.locationtech.jts.geom.curved)
- CircularString extends LineString
- CompoundCurve extends LineString (member structure collapsed in
this phase)
- CurvePolygon extends Polygon (rings linearised on read)
- MultiCurve extends MultiLineString
- MultiSurface extends MultiPolygon
- Triangle extends Polygon (Polygon with one 4-point ring)
- PolyhedralSurface extends MultiPolygon
- Tin extends PolyhedralSurface
`Triangle` lives in `.geom.curved` because the existing static-utility
class `org.locationtech.jts.geom.Triangle` (centroid, circumradius)
is preserved unchanged.
Readers / writers (in org.locationtech.jts.io.curved)
- CurvedWKTReader extends WKTReader, overrides
`readOtherGeometryText` to dispatch the eight new keywords. Composite
types (CompoundCurve, CurvePolygon, MultiCurve, MultiSurface) iterate
heterogeneous members via `lookAheadWord`-based peeking. CompoundCurve
also accepts the writer's flat round-trip form.
- CurvedWKTWriter extends WKTWriter; phase-1 marker. Emission already
works correctly via the core `getGeometryType().toUpperCase()` change
routed through the existing `instanceof` ladder.
Tests
- 54 round-trip tests across 8 files covering XY / XYZ / XYM / XYZM /
EMPTY / round-trip / structural-validation-deferred negative cases.
- All pass against the new module; full jts-core suite (2282 tests)
still passes unchanged.
Out of scope (deferred to later phases per the proposal)
- Validation (Triangle 4-point rule, CircularString odd point count,
CompoundCurve member connectivity, Tin triangle-only patches).
- Member-structured CompoundCurve / CurvePolygon emission (currently
collapsed to flat coordinates / linear rings).
- WKBReader / WKBWriter for SFA-MM type codes.
- Native curve-aware spatial operations (intersects, contains, area,
buffer). Curves currently fall through to their parent type's
polyline / polygon behaviour.
- copy() returning the correct subclass.
- JTSTestBuilder UI integration.
References
- Discussion: locationtech#1193
- Design template: NetTopologySuite/NetTopologySuite#526
…README Addresses code-review feedback on PR locationtech#1194. All additions are still purely additive at the jts-core level. jts-curved - New CurvedGeometryFactory extends GeometryFactory with creation methods for the eight new types (createCircularString, createTriangle, etc.). Pair with CurvedWKTReader for the standard read-construct-emit flow. - New Linearizable interface (Geometry toLinear(double tolerance)) for converting curved geometries to non-curved approximations. Phase 1 returns a parent-type geometry built from the same control points; a future phase will swap in real arc densification. - Implement Linearizable on the seven curve / curve-bounded / triangulated-surface types (Triangle, PolyhedralSurface, Tin omit Linearizable but get correct copyInternal too). - Override copyInternal() on all eight types so copy() returns the correct subclass instead of degrading to the parent. - Module README: usage example, phase-1 limitations (with explicit callout for the CurvePolygon / MultiSurface inner-member round-trip degradation), and discovery rationale (explicit instantiation rather than ServiceLoader). WKTMultiSurfaceTest - testWKTRoundTripXY no longer compares the original CurvePolygon- bearing geometry directly to the round-tripped Polygon-bearing one. Polygon.isEquivalentClass is strict (LineString's is lenient, which is why MultiCurve passes the analogous test); a direct checkEqual would fail. Switched to (a) WKT-stability (write -> read -> write yields the same WKT) and (b) coordinate-equivalence via toLinear(). Comment documents the seam. jts-core - Add WKTReaderExtensionHookTest with 6 tests that exercise both the reader and writer extension hooks via dummy in-test subclasses, with no dependency on jts-curved. Confirms the seam is wired and that the promoted protected helpers are accessible across packages. - Class-level Javadoc on WKTReader now documents the extension contract and lists the protected helpers available to subclasses. Same for WKTWriter (intercept point, parameterised keyword emission, and the exposed text-emission helpers). - Per-field Javadoc on the geometryFactory and csFactory protected fields explaining their role in extension subclasses. Net change: jts-core grows by 6 tests (2288 total, was 2282). jts-curved holds 54 tests, all green. Combined: 2342 tests, BUILD SUCCESS. Discovered seam (recorded in MultiSurface test comment + README): Polygon.isEquivalentClass is strict, so a Polygon and a coord-identical CurvePolygon are not equalsExact even though LineString solves the analogous problem leniently. Phase 2 (member-tagged emission in CurvedWKTWriter) will close this gap; phase 1 documents and works around it.
Phase 4-A of the SFA / ISO 19125-2 curve work (per discussion locationtech#1193). Single-purpose change: every WKT-parsing path inside jts-app now uses the curve-aware reader and factory, so curved WKT can be pasted, loaded, saved, and round-tripped without any new UI. What now works in JTSTestBuilder - Pasting `CIRCULARSTRING(1 5, 6 2, 7 3)` (or COMPOUNDCURVE, CURVEPOLYGON, MULTICURVE, MULTISURFACE, TRIANGLE, POLYHEDRALSURFACE, TIN, with optional Z/M/ZM modifiers) into the Input A/B panels. - Loading a `.jts` test case that contains curved WKT. - Loading WKT/WKB files via File menu. - The Geometry tree shows the correct type name (CircularString, etc.). - Spatial functions fall through to the parent type's behaviour (polyline / polygon) per the phase-1 contract; explicit linearisation is available via Linearizable.toLinear(tolerance). Files touched (all jts-app) - pom.xml: add jts-curved dependency. - testbuilder/JTSTestBuilder.java: fallback geometry factory is now CurvedGeometryFactory. - testbuilder/model/TestBuilderModel.java: default geometry factory is CurvedGeometryFactory; loadGeometryText and the post-precision-model- change reload path use CurvedWKTReader / CurvedGeometryFactory. - testbuilder/GeometryInputDialog.java: input parser uses the curved reader / factory. - util/io/IOUtil.java: readWKTString uses CurvedWKTReader (this is the central WKT path, also reached via MultiFormatReader). - util/io/MultiFormatBufferedReader.java: readWKT uses CurvedWKTReader. - util/io/MultiFormatFileReader.java: readWKTFile uses CurvedWKTReader. - test/TestCase.java: initGeometry and toNullOrGeometry use CurvedWKTReader / CurvedGeometryFactory for test-case WKT parsing. What is intentionally NOT in this commit - No drawing tools for curved geometries (defer to Phase 4-B). - No CurvedShapeWriter / Bezier rendering (defer to Phase 4-B). - No GeometryType enum extension or controller mode methods. Net diff: +30 / -15 lines across 8 files. Reactor build is green (jts-core 2288 + jts-curved 54 tests pass; all other modules unchanged).
The five "testRejectsX" methods across the curved-module test suite
used the pattern
try {
reader.read(badInput);
fail("...");
} catch (Throwable e) {
// expected
}
which silently passes whether or not the reader actually rejects the
input -- JUnit 3's fail(...) throws AssertionFailedError, which is a
Throwable, which the catch block swallows. So the tests passed in CI
even though four of the five validations are not implemented yet:
- CircularString odd-point-count rule -- not validated
- CompoundCurve member-endpoint connectivity -- not validated
(silent coord-drop)
- Triangle 4-point-only ring -- not validated
- Triangle no-inner-rings -- not validated
- Tin triangle-only patches -- not validated
This contradicted the curve-awareness spec epic's "landing a sub-issue
deletes its red test" convention: a deceptive green test cannot serve
as a live progress meter.
Rewrites each spurious "rejects" test as an explicit acceptance test
naming the deferred OGC SFA rule and pointing at the spec-epic sub-
issue tag (VAL-CS, VAL-CC, VAL-T, VAL-TIN). The assertion captures
the actual current behaviour (e.g. lossy 3-coord output for the
disconnected COMPOUNDCURVE case), so when the validation phase lands
the assertion flips red and the maintainer is prompted to convert
the test back to an explicit expectThrows(ParseException) -- or
delete it per the epic convention.
The two unclosed-ring tests (Triangle / CurvePolygon) DO have real
validation behind them today, via LinearRing's IllegalArgumentException
on non-closed coordinate sequences. Their catch blocks are narrowed
from Throwable to IllegalArgumentException so a future regression is
caught loudly instead of silently.
No production-code change -- this commit is purely test integrity.
54 curved-module tests still green.
Assisted-by: Claude (Opus-4.7)
Signed-off-by: Jeroen Bloemscheer <jeroen@jeroentechsolutions.uk>
CurvedWKTReader previously rolled its own static matchesType helper
that duplicated the Z/M/ZM suffix logic already present in
WKTReader.isTypeName. Two small downsides:
- The hand-rolled matcher returns false on an unrecognised suffix
(e.g. "CIRCULARSTRINGFOO"), so the input falls through to
super.readOtherGeometryText which throws the generic
"Unknown geometry type". isTypeName instead throws the precise
"Invalid dimension modifiers: CIRCULARSTRINGFOO" -- strictly
better diagnostics for the same dispatch path.
- Any future readOtherGeometryText extender (a downstream module
for 3-D solid types, GeoSPARQL flavours, etc.) would need to
reproduce the modifier logic from scratch, with the usual risk
of drift if the core list of recognised modifiers ever changes.
Change: promote WKTReader.isTypeName from private to protected (with
Javadoc clarifying the contract for subclasses), delete the duplicate
matchesType helper in CurvedWKTReader, and route all eight
keyword-dispatch branches through isTypeName.
Behaviour is the same for valid input; invalid dimension modifiers
now surface as a precise ParseException instead of a generic one.
2288 jts-core tests + 54 jts-curved tests still green.
Assisted-by: Claude (Opus-4.7)
Signed-off-by: Jeroen Bloemscheer <jeroen@jeroentechsolutions.uk>
readSurfaceMember dispatches on `instanceof Polygon`, which means a tagged TRIANGLE keyword (Triangle extends Polygon) is a legal MULTISURFACE member -- but no existing test exercised that path. The dispatch is a contract surface for downstream extenders adding new Polygon subclasses; lock it in. Assisted-by: Claude (Opus-4.7) Signed-off-by: Jeroen Bloemscheer <jeroen@jeroentechsolutions.uk>
Phase 4-A's test plan marked the manual JTSTestBuilder smoke test as "defer to maintainer review". Land a concrete recipe so a reviewer (or a future contributor verifying a regression) can paste a row, click Load, and check the geometry-tree label without having to guess at the expected outcome. The table covers all eight new types plus a Z/M/ZM dimension variant and ends with the round-trip-via-Save step that exercises the .jts file path through CurvedWKTReader / CurvedGeometryFactory. The toLinear escape hatch is referenced as a programmatic API, flagged as still-deferred-to-Phase-4-B for the UI panel binding. Assisted-by: Claude (Opus-4.7) Signed-off-by: Jeroen Bloemscheer <jeroen@jeroentechsolutions.uk>
…ests Of the 49 TAGs in the SFA Curve Awareness epic (issue locationtech#1195), F-CP (structural CurvePolygon) carries the heaviest dovetailing load: - It is the hard Phase-1 prerequisite for every Phase-2 TAG that needs to talk about a curved boundary, area, or validity (B-CP, M-AREA-CP, V-CP). - It carries the headline risk #1 in epic §7: a structural CurvePolygon whose shell is a CompoundCurve cannot also satisfy Polygon's legacy LinearRing-typed accessor without an explicit design choice. - The existing Phase-1 stand-in in jts-curved silently linearises CurvePolygon rings on read; a future structural form must coexist with or replace that path. - It depends on (and is unblocked by) the just-landed feature/sfa-curve-compoundcurve-members work, so the precondition is real, not aspirational. The single test_F_CP_* method in CurveAwarenessSpecTest documents the headline gap. This spike adds: modules/curved/src/test/java/org/locationtech/jts/spec/curveawareness/ CurvePolygonStructuralSpec.java 8 red tests covering the F-CP sub-surface: - FCP-S shell is exposed as a Curve (Compound or Circular), not a LinearRing - FCP-MEM CompoundCurve shell preserves member subtypes - FCP-H interior rings (holes) are Curves per spec - FCP-CP copyInternal preserves shell + holes as Curves - FCP-TL toLinear(tolerance) walks shell and each hole - FCP-WKT WKT round-trip preserves the structural ring tag - FCP-DOVE the legacy Polygon API contract -- design decision point, see SPEC_F_CP.md modules/curved/SPEC_F_CP.md Captures the three live options for the FCP-DOVE decision (legacy fallback / widen return / fail-fast) as a branching table with trade-offs, where we lean today, and what's deferred. Lays out the smallest concrete next step: a one-line maintainer ack on the option, then a separate `arch:` commit recording the choice, then the implementation PR. modules/curved/pom.xml Adds the Surefire exclude on **/spec/curveawareness/*.java per epic §11 -- the spec class is intentionally red and runs on demand via `mvn -pl modules/curved test -Dtest=CurvePolygonStructuralSpec`. Default reactor build stays green (55 jts-curved tests, all green, spec class excluded). Posture verified: - default `mvn test -pl modules/curved` => 55 / 55 green - on-demand `-Dtest=CurvePolygonStructuralSpec` => 8 run, 8 failed, each fail() naming its sub-TAG so the message stream is a live progress meter - checkstyle `mvn install -pl modules/curved` => BUILD SUCCESS Assisted-by: Claude (Opus-4.7) Signed-off-by: Jeroen Bloemscheer <jeroen@jeroentechsolutions.uk> spec: F-CP Option-A spike -- structural shell via getExteriorCurve() Prototype Option A from SPEC_F_CP.md: keep Polygon.getExteriorRing() returning a LinearRing of the linearised chord coords (legacy callers keep working unchanged), and add a new getExteriorCurve() accessor that returns the structural shell (CompoundCurve / CircularString / LineString / LinearRing). Diff: +60 / -8 lines across three files. - CurvePolygon: new private LineString structuralShell field, new (LineString, LinearRing[], GeometryFactory) constructor that derives the legacy LinearRing via Linearizable.toLinear(0.0) and new getExteriorCurve() accessor. copyInternal preserves the structural shell. - CurvedWKTReader.readCurvePolygonText: captures the first member as a structural LineString and feeds the structural-aware constructor; subsequent members still linearise to LinearRing holes (FCP-H is a separate sub-issue). - CurvePolygonStructuralSpec: helper structuralShellOf now calls getExteriorCurve(); FCP-DOVE flips from "fail with design question" to a positive assertion that the Option-A accessor works. Findings (run on this branch): - 55 jts-curved Phase-1 tests stay green -- Option A is fully backward-compatible at the curved-module surface. - 4 of 8 FCP red tests flip green (FCP-S, FCP-S-singleArc, FCP-CP, FCP-DOVE) -- proof that the structural-shell mechanism, the copyInternal propagation, and the design-decision accessor all work. - 4 reds remain (FCP-MEM, FCP-H, FCP-WKT, FCP-TL). All four track independent sub-issues that any of A / B / C would have to address separately: FCP-MEM CompoundCurve member preservation -- precondition lives on feature/sfa-curve-compoundcurve-members FCP-H interior-ring symmetry (parallel getInteriorCurveN accessor) FCP-WKT writer must emit COMPOUNDCURVE tag inside body FCP-TL toLinear walking + densification -- depends on feature/sfa-curve-toLinear-densification - Full reactor build with checkstyle: BUILD SUCCESS. Discovery for the issue: Option A is implementable in ~60 LOC of additive change, with no breaking impact on existing jts-curved tests or jts-core. The remaining open question for A is the default tolerance source for the linearised getExteriorRing() view (today toLinear(0.0) ignores tolerance; a real implementation would parameterise on PrecisionModel scale or CurvedGeometryFactory state). That decision sits inside the implementation PR, not in the A-vs-B-vs-C choice. Assisted-by: Claude (Opus-4.7) Signed-off-by: Jeroen Bloemscheer <jeroen@jeroentechsolutions.uk> feat: F-CP structural CurvePolygon (Option A) + enable F-MC/F-MS - CurvePolygon now accepts/roundtrips/preserves LineString (incl. CircularString/CompoundCurve) shells and holes - Added getExteriorCurve / getInteriorCurveN; legacy get*Ring return densified LinearRing per Option A - Reader collects structural rings for CURVEPOLYGON - Writer overrides appendOther to emit tagged curved rings inside CURVEPOLYGON bodies - Factory extended with structural createCurvePolygon(LineString, LineString[]) - copyInternal / toLinear(tol) walk and preserve the structural curves - Removed F-CP (and F-MC/F-MS as they were already structurally supported) red tests from CurveAwarenessSpecTest - Cleaned FCP tests from structural spike spec; kept DOVE as contract test (now passes) - References locationtech#1195 Assisted-by: xAI Grok
…ec tests for proper curved ring syntax - Move normalization expression into super() arg list so super is first statement (flexible ctors not available under -source 8). - Fix several CURVEPOLYGON ((TAG... WKTs in CurveAwarenessSpecTest to use canonical CURVEPOLYGON (TAG... or COMPOUNDCURVE ( for compound rings. Prevents read() errors so higher TAGs can execute. - Verified: curved normal tests 55/55 green; structural DOVE test green; main spec now 46/46 reaching their fail() (no more parse crashes for F-CP related). Part of locationtech#1195 F-CP landing.
…(), normalize() in CurvePolygon to preserve structural curves (HIGH severity from review) - reverseInternal now reverses the structural* members and reconstructs via structural ctor (prevents silent degradation to LinearRings via base impl + legacy factory path). - Public reverse() overridden to return CurvePolygon (like copy). - normalize() calls super (to keep legacy views normalized per Polygon contract / Option A) but leaves structural curves as source-of-truth (avoids losing arc identity). - Matches reviewer suggestion in /tmp/grok-review-d5a00063.md item 1. - Still defers deeper arc-orientation normalization. Part of F-CP for locationtech#1195. Review artifacts preserved locally.
… and remove per-ring Z/M qualifiers; doc+test: explicit note+test_FCP_EQ for structural curves not affecting equalsExact (EPIC §7 / R-EQ); test: add adversarial CurveRefRunner/CounterexampleHunter/CurveAdversarialTest + arc vectors (inspired by PR#1197 RocqRefRunner for M-LEN-*); test: enhance WKTCurvePolygonTest for 3D/compound roundtrips
- CurvePolygon now accepts/roundtrips/preserves LineString (incl. CircularString/CompoundCurve) shells and holes - Added getExteriorCurve / getInteriorCurveN; legacy get*Ring return densified LinearRing per Option A - Reader collects structural rings for CURVEPOLYGON - Writer overrides appendOther to emit tagged curved rings inside CURVEPOLYGON bodies - Factory extended with structural createCurvePolygon(LineString, LineString[]) - copyInternal / toLinear(tol) walk and preserve the structural curves - Removed F-CP (and F-MC/F-MS as they were already structurally supported) red tests from CurveAwarenessSpecTest - Cleaned FCP tests from structural spike spec; kept DOVE as contract test (now passes) - References locationtech#1195 Assisted-by: xAI Grok
…ase-1 control-point view; empty CompoundCurve guard + roundtrip test; clean stale javadoc + dead helpers in CurvePolygonStructuralSpec; clarify SPEC + test language)
grootstebozewolf
force-pushed
the
feature/sfa-curve-F-CP-option-a
branch
from
820b0f4 to
78e46d9
Compare
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
1 participant
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.
F-CP Option A implementation (stacked on #1194).
Per SPEC_F_CP.md and epic #1195 Phase 1:
CurvePolygonnow stores/roundtrips structuralLineStringshell + holes (CircularString / CompoundCurve / LineString).getExteriorRing()/getInteriorRingN()return densifiedLinearRingviews (Option A, keeps Polygon contract for jts-core + 3rd party).getExteriorCurve()/getInteriorCurveN(int)for curve-aware callers.CurvedWKTReader/Writerpreserve member structure for CURVEPOLYGON rings.copyInternal,reverseInternal,toLinear,normalizepreserve curves.CurvedGeometryFactorygains structuralcreateCurvePolygon(LineString, LineString[]).CurveAwarenessSpecTest+CurvePolygonStructuralSpecshrunk (kepttest_FCP_DOVE+ newtest_FCP_EQas executable docs of the chosen contract / R-EQ semantics).Why concise for dr-jts (solo maintainer):
mvn -pl modules/curved test -Dtest=CurvePolygonStructuralSpec,WKTCurve*(and full curved 58) green.Post-merge: Phase 1 foundations complete; unlocks B-/M-/V-*/etc per §10.
Assisted-by: xAI Grok 4.3