Opened 10 years ago

Closed 10 years ago

Last modified 10 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: master
Severity: Unassigned Keywords:
Cc:

Attachments (2)

Screenshot.png (5.1 KB) - added by strk 10 years ago.
bug398bis.xml (2.2 KB) - added by strk 10 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 Changed 10 years ago by strk

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

r3269 adds an XML version of this test.

comment:2 Changed 10 years ago by strk

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 Changed 10 years ago by strk

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

comment:4 Changed 10 years ago by strk

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.

Changed 10 years ago by strk

Attachment: Screenshot.png added

comment:5 Changed 10 years ago by strk

The collapsed geometry according to GEOS, in HEXWKB form:

0102000000040000000000000000000000841D588465963540F56BFB214F0341408F26B714B2971B40F66BFB214F0341408C26B714B2971B400000000000000000841D588465963540

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

comment:6 Changed 10 years ago by strk

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 Changed 10 years ago by strk

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 Changed 10 years ago by strk

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 Changed 10 years ago by strk

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.

Changed 10 years ago by strk

Attachment: bug398bis.xml added

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

comment:10 Changed 10 years ago by strk

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 Changed 10 years ago by strk

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 Changed 10 years ago by warmerdam

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.