Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#633 closed defect (fixed)

Mismatch segment sides in OffsetCurveBuilder

Reported by: mloskot Owned by: mloskot
Priority: major Milestone: 3.4.0
Component: Core Version: master
Severity: Significant Keywords: history
Cc:

Description (last modified by mloskot)

This ticket follows bug fix committed in r3817.

There are several more places that need to be reviewed in OffsetCurveBuilder.cpp file, Position::LEFT in

  • line 221 in OffsetCurveBuilder::computeLineBufferCurve
  • line 275 in OffsetCurveBuilder::computeSingleSidedBufferCurve

should likely read Position::RIGHT.

As shown in the test case, the problem is that for the given LINESTRING:

LINESTRING (665.7317504882812500 133.0762634277343700, 1774.4752197265625000 19.9391822814941410, 756.2413940429687500 466.8306579589843700, 626.1337890625000000 1898.0147705078125000, 433.8007202148437500 404.6052856445312500)

right-side curve has 0 vertices:

BufferParameters params;
params.setJoinStyle(BufferParameters::JOIN_MITRE);
params.setMitreLimit(5.57F); // somewhere between 5.5 and 5.6
params.setSingleSided(true);
BufferBuilder builder(params);
builder.bufferLineSingleSided(input, distance, true);  // 5-vertex output
builder.bufferLineSingleSided(input, distance, false); // 0-vertex output

However, attempt to fix those causes unit tests failures for the buffer operation.

Attachments (2)

BufferBuilderTest.cpp (4.0 KB) - added by mloskot 5 years ago.
Test used to reproduce the problem reported.
BufferBuilderTestDistNegative.cpp (4.3 KB) - added by mloskot 5 years ago.
Followign strk's tests, C++ test case updated to use NEGATIVE distance to request right-sided curve

Download all attachments as: .zip

Change History (23)

comment:1 Changed 5 years ago by strk

I guess the user classes of OffsetCurveBuilder? (BufferBuilder?, most likely) is in agreement about the swap, so that if you swap flags in OffsetCurveBuilder? you should also swap them outside.

Order of API importance is (most important to least important): C-API, BufferOp?, BufferBuilder?, OffsetCurveBuilder?. I don't care if OffsetCurveBuilder? changes semantic of LEFT:RIGHT (if previous was wrong) as long as BufferOp? doesn't show any difference outside.

comment:2 Changed 5 years ago by strk

Please revert the change if you don't have time to carefully fix all occurrences. In its current incarnation the file only generate the curve on the left, so normal buffer should be broken. I'm surprised it works at all... (not tested)

comment:3 Changed 5 years ago by mloskot

I see that some significant comments are missing from the GEOS code in comparison to JTS code, for example in computeOffsetCurve method:

    if (isRightSide) {
      //---------- compute points for right side of line
...
      // since we are traversing line in opposite order, offset position is still LEFT
else {
      //--------- compute points for left side of line

Changed 5 years ago by mloskot

Attachment: BufferBuilderTest.cpp added

Test used to reproduce the problem reported.

comment:4 Changed 5 years ago by mloskot

Description: modified (diff)

comment:5 Changed 5 years ago by strk

I tried this case from Postgis, using ST_OffsetCurve and it works fine with a distance of 57.164000837203 (negative for right side, positive for left side). Can you confirm your test also works fine if you omit the mitre limit and use round styles ?

comment:6 Changed 5 years ago by strk

Even with mitre join and mitre limit the result is fine from postgis:

select ST_AsTExt(ST_OffsetCurve('LINESTRING (665.7317504882812500 133.0762634277343700,
 1774.4752197265625000 19.9391822814941410, 756.2413940429687500
 466.8306579589843700, 626.1337890625000000 1898.0147705078125000,
 433.8007202148437500 404.6052856445312500)', 
 -57.164000837203, 
 'join=mitre mitre_limit=5.57'));
LINESTRING(377.104972695962 411.907011809687,609.68600615508 2216.7818017113,654.496491514261 2215.94323235809,810.115438893684 505.613201784055,2084.33508634209 -53.729796722186,2080.40513353176 -68.6449203870038,659.928822325032 76.2075636320831)

This is with GEOS="3.4.0dev-CAPI-1.8.0 r3800"

Changed 5 years ago by mloskot

Followign strk's tests, C++ test case updated to use NEGATIVE distance to request right-sided curve

comment:7 Changed 5 years ago by mloskot

In the test case I added to GEOSOffsetCurveTest.cpp@3820 based on C API, all works as you are reporting, indeed.

However, using negative distance, see the new BufferBuilderTestDistNegative?.cpp, and reproducing other parameters according to C API, does not make the test pass. The 2nd test case still produces curve with Zero vertices.

comment:8 Changed 5 years ago by strk

Looking at the C-API implementation, the BufferParameter? used to initialize the BufferBuilder? used for bufferLineSingleSided isn't configured to use single-sided buffer. Don't know if it matters, but anyway I suggest you look at the C-API implementation as a reference. setSingleSided may be for areal return. "bufferLineSingleSided" is a GEOS-only function IIRC.

comment:9 Changed 5 years ago by mloskot

Certainly, passing negative distance as it happens in C API/PostGIS does nto make any sense. The sign used in those only controls leftSide flag. So, the test case BufferBuilderTestDistNegative?.cpp is incorrect, of course.

In order to get the same results as in C API/PostGIS, I have to NOT to set the C++ BufferBuilder? with:

params.setSingleSided(true); // makes tests failing

and then for the test LINESTRING, I'm getting 7 (or more) vertices of right-side offset curve.

comment:10 Changed 5 years ago by mloskot

Apparently, the BufferParameters::setSingleSided is something that Martin added in JTS to implement areal single-sided buffers. Whereas BufferBuilder::bufferLineSingleSided is for lines only, and misnamed (should be OffsetCurve?, which is how it's called in C-API). This suggests, that the setSingleSided(true) setting is invalid for non-areal input geometry.

Perhaps we could add something like this to the bufferLineSingleSided:

BufferBuilder::bufferLineSingleSided( const Geometry* g, double distance,
                                      bool leftSide )
{
    // Alternatively reset-to-ignore: bufParams.setSingleSided(false)
    if (bufParams.isSingleSided())
        throw util::IllegalArgumentException("BufferBuilder::bufferLineSingleSided: setSingleSided(true) parameter must not be specified for  non-areal input");

   // Returns the line used to create a single-sided buffer.
   // Input requirement: Must be a LineString.
   const LineString* l = dynamic_cast< const LineString* >( g );
   if ( !l ) throw util::IllegalArgumentException("BufferBuilder::bufferLineSingleSided only accept linestrings");

comment:11 Changed 5 years ago by strk

I guess reset-to-ignore would be the most useful thing to do here, if it doesn't have drawbacks. IllegalArgumentException? would be the safest. I'd go with reset-to-ignore for 3.4, as there's enough time for people to test it (and you're on it, right ?)

comment:12 Changed 5 years ago by mloskot

Owner: changed from strk to mloskot
Status: newassigned

comment:13 Changed 5 years ago by mloskot

Milestone: 3.4.0

comment:14 Changed 5 years ago by mloskot

I've added updated BufferBuilderTest? in r3821.

In r3822, I have added check for uniform coordinates order in both, intput and output geometry. As it turns out, right-side offset curve is generated with coordinates in reverse order. IMHO, we should decided if BufferBuilder::bufferLineSingleSided should take care of reversing coordinates, if necessary.

comment:15 Changed 5 years ago by strk

I was aware of the reversed outcome from the right-side offset curve. That's useful for the areal result. Indeed it may be more useful if bufferLineSingleSided reversed the line. Can't remember if PostGIS does this itself, should be checked. It'd be also a good test of a client having to deal with different behavior depending on different version (3.3 vs. 3.4?). Can you try reversing and see if any test fails, including postgis ones ?

comment:16 Changed 5 years ago by mloskot

I will try to run tests as you request, but I think it's better to decide on the actual behaviour of this operation, on paper. So, we can document and promise to users one or the other:

  • vertex order of input is always preserved in the output
  • vertex order is may or may not change

Does JTS promise anything here?

comment:17 Changed 5 years ago by strk

It turns out we're already promising reverse direction at the postgis side: http://postgis.net/docs/ST_OffsetCurve.html

For positive distance the offset will be at the left side of the input line and retain the same direction. For a negative distance it'll be at the right side and in the opposite direction.

comment:18 Changed 5 years ago by strk

Better to document what we have, for now.

comment:19 Changed 5 years ago by mloskot

Final clarification of BufferBuilder::bufferLineSingleSided behaviour in r3823

  • Ignore BufferParameters::setSingleSided() parameter as it is specific to areal geometries.
  • Document semantic of input parameters.
  • Document order of coordinates in generated output (conforms to PostGIS behaviour).
  • Add test for coordinates order assumptions.

I think we can assume the behaviour regarding coordinates order is fixed and should not change in future.

comment:20 Changed 5 years ago by mloskot

Resolution: fixed
Status: assignedclosed

comment:21 Changed 5 years ago by robe

Keywords: history added
Note: See TracTickets for help on using tickets.