Opened 13 years ago

Closed 13 years ago

Last modified 13 years ago

#398 closed defect (fixed)

PolygonBuilder::findShell assertion 'shellcount <= 1' failed [JTS fails!]

Reported by: strk Owned by: strk
Priority: major Milestone: 3.3.0
Component: Core Version: main
Severity: Unassigned Keywords:
Cc:

Description

Attachments (2)

Screenshot.png (5.1 KB ) - added by strk 13 years ago.
bug398bis.xml (2.2 KB ) - added by strk 13 years ago.
the new test, same input and op as old, different expected result, failing JTS

Download all attachments as: .zip

Change History (14)

comment:1 by strk, 13 years ago

Summary: PolygonBuilder::findShell assertion 'shellcount <= 1' failedPolygonBuilder::findShell assertion 'shellcount <= 1' failed [JTS works!]

r3269 adds an XML version of this test.

comment:2 by strk, 13 years ago

Owner: changed from geos-devel@… to strk
Status: newassigned

Found the difference: JTS is doing this:

    PlanarGraph.linkResultDirectedEdges(nodes);

right in the PolygonBuilder::add function. GEOS has that call commented out. WHen commenting it in JTS we get the same assertion failure...

comment:3 by strk, 13 years ago

Uhm, false alarm, the GEOS code is indeed inlining the equivalent of that function

comment:4 by strk, 13 years ago

The first real difference between GEOS and JTS is at the point where a minimalEdgeRing infact dimensionally collapsed is considered as an hole by JTS and not by GEOS.

I attach a snapshot showing the two MinimalEdgeRing geometries. You can see the blue one collapsed.

by strk, 13 years ago

Attachment: Screenshot.png added

comment:5 by strk, 13 years ago

The collapsed geometry according to GEOS, in HEXWKB form:

0102000000040000000000000000000000841D588465963540F56BFB214F0341408F26B714B2971B40F66BFB214F0341408C26B714B2971B400000000000000000841D588465963540

Haven't tested if JTS considers coordinates of this CW or CCW

comment:6 by strk, 13 years ago

I've checked that JTS gets to the _exact_ same HEXWKB, so obviously JTS considers that thing to be CCW while GEOS does not.

comment:7 by strk, 13 years ago

Ok, JTS's version is _not_ exactly the same. Thanks to robe for finding out, as I was getting mad:

GEOS:
0102000000040000000000000000000000841D588465963540F56BFB214F0341408F26B714B2971B40F66BFB214F0341408C26B714B2971B400000000000000000841D588465963540
JTS:
0102000000040000000000000000000000841D588465963540F56BFB214F0341408F26B714B2971B40F66BFB214F0341408E26B714B2971B400000000000000000841D588465963540
---------------------------------------------------------------------------------------------------^ difference here

Now I'm even more curious about whether JTS considers the former clockwise too...

comment:8 by strk, 13 years ago

in any case, turning the assertion failure in a robustness exception triggers engaging of snapping policy which results in an output with the collapsed "ring" removed, which passes the expectances of the tester and seems better for me too.

comment:9 by strk, 13 years ago

Now this is funny. I've modified the testcase to expect GEOS output (the one with the collapsed ring removed) and JTS reacts to that new testcase with the shellcount <= 1 assertion.

It's funny because the input did not change, only the expected output changed.

I attach the new testcase here, in case Martin wants to take a look.

Will commit the new testcase and the exception solution, which I belive makes sense, being this clearly a robusness issue.

by strk, 13 years ago

Attachment: bug398bis.xml added

the new test, same input and op as old, different expected result, failing JTS

comment:10 by strk, 13 years ago

Milestone: 3.3.0
Resolution: fixed
Status: assignedclosed
Summary: PolygonBuilder::findShell assertion 'shellcount <= 1' failed [JTS works!]PolygonBuilder::findShell assertion 'shellcount <= 1' failed [JTS fails!]
Version: svn-trunk

Fixed in r3286

comment:11 by strk, 13 years ago

For the record: JTS agrees with GEOS about the ring computed by GEOS being CW: http://sourceforge.net/mailarchive/message.php?msg_id=27379769

Our r3288 puts both rings under regression testing.

comment:12 by warmerdam, 13 years ago

Component: DefaultCore

With the error report now being an exception it is necessary for the related logic to no longer be run only when NDEBUG is not defined. Fixed in trunk (r3373).

Note: See TracTickets for help on using tickets.