Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GEOSOffsetCurve_r returns multipart geometries for simple single line input curve #477

Closed
nyalldawson opened this issue Aug 30, 2021 · 24 comments
Assignees
Labels
Milestone

Comments

@nyalldawson
Copy link

Given the input linestring:

LINESTRING (435916.931276825 5317307.89296271,435918.66095346 5317306.55315136,435918.74215247 5317306.49645725,435919.681002279 5317305.90786128,435921.21792646 5317304.70052204,435922.432700873 5317303.23810423,435923.312958213 5317301.55302567,435923.819370855 5317299.72057131,435923.929313594 5317297.82261048,435923.629705377 5317295.89127962,435921.31706914 5317287.19038841,435921.221701166 5317286.85631381,435915.908874538 5317269.4572725,435914.7633747 5317267.01641003,435903.842360859 5317250.39158356,435899.758588329 5317244.17647954,435899.455360435 5317243.7419094,435893.628526679 5317235.86859603,435893.224487222 5317235.35998225,435872.782446377 5317211.35986935,435872.372693834 5317210.90893047,435869.15930026 5317207.59274011,435867.883192455 5317206.49082854,435864.610150099 5317204.13727291,435863.353861139 5317203.37399972,435860.256010043 5317201.80619905,435857.848390088 5317200.9709003,435853.922745693 5317200.17766004,435851.682719877 5317199.99627125,435846.607217165 5317200.18812387,435845.378374182 5317200.31495892,435839.281241994 5317201.34852148,435837.464642851 5317201.8458565,435832.430833488 5317203.77821505,435831.484909335 5317204.20189893,435825.880474749 5317207.08899965,435825.12124478 5317207.52552405,435821.328166785 5317209.94561359,435821.149319795 5317210.05972295,435820.050621375 5317210.76072319,435814.860158069 5317214.07238352,435814.838370797 5317214.08589461,435814.618838194 5317214.21816895,435814.596711727 5317214.23111709,435808.966860142 5317217.4294731,435808.951285284 5317217.43813861,435808.794789304 5317217.52339208,435808.779062482 5317217.53177862,435803.01073606 5317220.54210429,435802.963122261 5317220.56536872,435802.481495028 5317220.78508349,435802.432715656 5317220.80579291,435796.863819116 5317222.99780748,435796.782502639 5317223.02583973,435795.960323675 5317223.27025114,435795.876899097 5317223.29119192,435780.347449045 5317226.48630237,435780.324337614 5317226.49077457,435780.092761038 5317226.53276446,435780.069550738 5317226.5366914,435772.817567975 5317227.67602179,435772.690430921 5317227.68774518,435771.421984674 5317227.72335853,435771.294390119 5317227.71878707,435767.88348365 5317227.37760801,435767.768182103 5317227.35922466,435766.633702871 5317227.10967695,435766.521329982 5317227.0779795,435764.078125903 5317226.23198537,435763.991190552 5317226.19724517,435763.141855147 5317225.81080517,435763.058549573 5317225.76808728,435758.411152463 5317223.1031489,435758.339817555 5317223.0581432,435757.64793541 5317222.57968342,435757.580646902 5317222.52882637,435753.413664903 5317219.09392388,435753.336942071 5317219.02366753,435752.610260899 5317218.28516032,435752.541252137 5317218.20731343,435749.175172161 5317213.98682501,435749.13533911 5317213.93331258,435748.755714259 5317213.38611485,435748.71953768 5317213.3300657,435745.16310407 5317207.39440303,435745.13520682 5317207.34468047,435744.87084485 5317206.84032003,435744.845826669 5317206.7890884,435743.664101745 5317204.18330231,435743.65119553 5317204.15357146,435743.527017347 5317203.85432842,435743.515081399 5317203.82419493,435741.053643157 5317197.30333302,435741.045265524 5317197.28028762,435740.964339211 5317197.04887603,435740.956529148 5317197.02563215,435739.842565796 5317193.57057633,435739.814521057 5317193.46369936,435739.594561963 5317192.38603107,435739.578479782 5317192.27671247,435739.189692389 5317188.00618857,435739.186229563 5317187.9517313,435739.16649418 5317187.40702564,435739.166006987 5317187.35246056,435739.348268731 5317177.42570189,435739.348688227 5317177.40977135,435739.354151883 5317177.25051952,435739.354825188 5317177.23459768,435739.623028294 5317171.89970192,435739.629281655 5317171.82754814,435739.717728252 5317171.11016272,435739.729187468 5317171.03865078,435742.114519534 5317158.96868311,435742.206127242 5317158.4216151,435745.170399421 5317136.87464647,435745.255376311 5317135.45819151,435745.12265794 5317134.04541002,435744.780030139 5317132.05489557)

Attempt to offset this curve using GEOSOffsetCurve_r, with the parameters:
width: 1
quadsegs: 8
joinStyle: 1
mitreLimit: 5

The result is a multi line string, with a segment of the line missing:

MULTILINESTRING((435917.5436520002 5317308.683530003,435919.2537435948 5317307.358889269,435919.29431819037 5317307.330559606,435920.2121784477 5317306.755122679,435920.29874662287 5317306.694240277,435921.8356708039 5317305.486901036,435921.98715750204 5317305.339492778,435923.201931915 5317303.877074968,435923.3190513573 5317303.701119153,435924.19930869725 5317302.016040593,435924.2768282938 5317301.819398467,435924.7832409358 5317299.986944107,435924.81769731024 5317299.778401136,435924.92764004925 5317297.880440306,435924.91749378055 5317297.669313647,435924.6178855635 5317295.737982787,435924.59615019965 5317295.634405322,435922.28351396264 5317286.933514113,435922.27865534223 5317286.915885267,435922.1832873683 5317286.581810666,435922.1781072104 5317286.56427361,435916.8652805824 5317269.1652323,435916.8141415969 5317269.032429527,435915.66864175885 5317266.591567057,435915.5991695112 5317266.467368107,435904.6781556702 5317249.842541637,435900.5943208807 5317243.627342852,435900.5786803694 5317243.604247903,435900.2754524754 5317243.169677763,435900.2591739285 5317243.14702801,435894.4323401725 5317235.273714641,435894.41153234337 5317235.246581457,435894.00749288633 5317234.7379676765,435893.98576959316 5317234.711561745,435873.54372874816 5317210.7114488445,435873.5225431885 5317210.687369008,435873.1127906455 5317210.236430128,435873.0908437737 5317210.213042071,435869.8774501997 5317206.896851711,435869.81285845663 5317206.835863843,435868.5367506516 5317205.733952273,435868.46700127976 5317205.67893738,435865.19395892374 5317203.32538175,435865.1293898194 5317203.28264427,435863.8731008594 5317202.51937108,435863.8054185729 5317202.481757666,435860.70756747696 5317200.913956995,435860.5837834563 5317200.861442684,435858.1761635013 5317200.026143935,435858.0464532601 5317199.990711043,435854.1208088651 5317199.197470782,435854.0034577102 5317199.180922576,435851.7634318942 5317198.999533786,435851.644947125 5317198.996984895,435846.569444413 5317199.188837515,435846.50454758556 5317199.193408354,435845.27570460254 5317199.320243404,435845.21124232246 5317199.329024367,435839.1141101345 5317200.362586928,435839.01718631224 5317200.384014043,435837.2005871692 5317200.881349063,435837.1062651744 5317200.912279807,435832.07245581143 5317202.844638357,435832.02205965493 5317202.865579332,435831.0761355019 5317203.289263212,435831.0269567877 5317203.3129222905,435825.4225222017 5317206.200023011,435825.3820317996 5317206.222077147,435824.6228018306 5317206.658601547,435824.5833710538 5317206.682498632),(435820.79029305896 5317209.102588172,435820.7902930588 5317209.102588172,435820.611446094 5317209.216697516,435819.512747644 5317209.917697776,435814.32768750045 5317213.225910737,435814.3167943096 5317213.232666006,435814.10824161425 5317213.358324654,435814.0971788271 5317213.364798464,435808.47677434137 5317216.557787513,435808.4689871327 5317216.562120146,435808.3203172044 5317216.64311026,435808.3124540114 5317216.647303414,435802.559824916 5317219.649437095,435802.53602223133 5317219.661067251,435802.07850604353 5317219.869782741,435802.0541206275 5317219.880135639,435796.51757214876 5317222.059417449,435796.4769327464 5317222.07342708,435795.69599625503 5317222.305578227,435795.65430326265 5317222.316043773,435780.15168936376 5317225.505632812,435780.1401340544 5317225.507868834,435779.9201393391 5317225.54775868,435779.9085345768 5317225.549722084,435772.69393014227 5317226.683180112,435772.63042652345 5317226.689035822,435771.4258584975 5317226.722855704,435771.36212634994 5317226.720572308,435768.01208220376 5317226.385481057,435767.95448062586 5317226.376297226,435766.87706593185 5317226.139301816,435766.8209274416 5317226.123466618,435764.427458262 5317225.294693903,435764.3840144472 5317225.277333339,435763.5773093254 5317224.910289735,435763.5356793817 5317224.888942503,435758.92689756135 5317222.246147135,435758.89124597627 5317222.223654297,435758.2340660156 5317221.769192245,435758.20043672953 5317221.743775032,435754.069814874 5317218.338844726,435754.0314794479 5317218.3037403505,435753.34130526334 5317217.602334243,435753.3068242559 5317217.563437166,435749.967373643 5317213.376337411,435749.9474626497 5317213.349588628,435749.5868560778 5317212.82980417,435749.5687728219 5317212.801787394,435746.028240652 5317206.892664115,435746.0142948573 5317206.86780788,435745.76316981716 5317206.388701389,435745.7506632674 5317206.363090777,435744.57817324356 5317203.777668287,435744.57172055385 5317203.762803825,435744.4537541374 5317203.478529822,435744.44778655807 5317203.463464074,435741.99137498427 5317196.955918853,435741.98718633066 5317196.944396601,435741.91030739015 5317196.724558613,435741.90640250174 5317196.712937099,435740.80280347314 5317193.290027144,435740.7887918194 5317193.236629497,435740.57988977234 5317192.213134138,435740.57185482763 5317192.158516614,435740.1868109183 5317187.92911194,435740.18507982744 5317187.90188837,435740.1663325009 5317187.384453526,435740.1660889498 5317187.3571760645,435740.34802707855 5317177.448042873,435740.34823682316 5317177.440077731,435740.35342726635 5317177.288789366,435740.3537639131 5317177.280828582,435740.6208575767 5317171.968001022,435740.62398323184 5317171.931935963,435740.70799732377 5317171.250502345,435740.7137250526 5317171.214758102,435743.09554564254 5317159.162558776,435743.10078754515 5317159.133835797,435743.19239525317 5317158.586767787,435743.19679637544 5317158.557904013,435746.16106855444 5317137.010935384,435746.16860470496 5317136.934531452,435746.25358159497 5317135.5180764925,435746.2509928294 5317135.364662112,435746.1182744584 5317133.951880622,435746.10816480155 5317133.875774452,435745.76553700055 5317131.885260002))

Shown below, where green = original line, blue = offset result showing the gap in the middle of the line:

image

(tested on GEOS 3.9)

@dr-jts
Copy link
Contributor

dr-jts commented Sep 10, 2021

Do you know if this works on earlier versions of GEOS?
How sensitive to the buffer distance is this error?

joinStyle = 1 is ROUND, which is the default, and most robust to compute, so that probably removes one possible cause.

@dr-jts
Copy link
Contributor

dr-jts commented Sep 10, 2021

The offset curve implementation is quite complex, so this could be a challenge to debug. The code uses overlay (SnapOverlayOp), so my first suspicion is that the change to OverlayNG in 3.9 has something to do with this. But not sure why or how SnapOverlayOp would have been affected.

It will be useful to add offsetCurve to geosop to allow testing across versions.

@nyalldawson
Copy link
Author

Actually, on further investigation it doesn't appear to be a regression at all, and can be reproduced back in 3.7.0!

@dr-jts
Copy link
Contributor

dr-jts commented Sep 10, 2021

Actually, on further investigation it doesn't appear to be a regression at all, and can be reproduced back in 3.7.0!

Well, that's good news and bad news! Perhaps an improved offset curve algorithm is called for.

@pramsey
Copy link
Member

pramsey commented Sep 10, 2021

Indeed. It's one of the places where GEOS functionality is > JTS functionality still, right? Not many of those now that JTS has GeometryFixer.

@dr-jts
Copy link
Contributor

dr-jts commented Sep 10, 2021 via email

@pramsey
Copy link
Member

pramsey commented Sep 10, 2021

Worth reading through https://github.com/libgeos/geos/blob/main/src/operation/buffer/BufferBuilder.cpp#L131 to see the current hack. It's amazing it works! Anyways, retrofitting it into the new snaprounder might make it magically work on this case.

@dr-jts
Copy link
Contributor

dr-jts commented Sep 10, 2021

Worth reading through https://github.com/libgeos/geos/blob/main/src/operation/buffer/BufferBuilder.cpp#L131 to see the current hack. It's amazing it works! Anyways, retrofitting it into the new snaprounder might make it magically work on this case.

Yes, have read through the code. Agreed it is amazing that it works - except when it doesn't!

The code actually uses snapping overlay, not snap-rounding (the distinction being that snapping snaps to the existing vertices, whereas snap-rounding snaps to a precision grid, and hence changes all vertices.). It's possible that using OverlayNG with the SnappingNoder will work better. But it's also possible that the approach of using a snapping intersection to identify the linework of an offset curve just ain't a perfect solution.

@pramsey
Copy link
Member

pramsey commented Sep 10, 2021

I was wondering whether snap-rounding might not be better precisely because it's a bigger hammer.

@dr-jts
Copy link
Contributor

dr-jts commented Sep 10, 2021

Well, some research required.

Here's an alternative. My understanding of the current algorithm is:

  1. compute the buffer of the input line
  2. compute the noded raw offset curve line of the input line (which will likely have unwanted loops and other artifacts)
  3. extract the desired offseet curve as the snapped intersection the buffer and the raw offset curve

Now, in step 3 the intersection is really being used as a way of selecting line segments from the raw offset curve. Using intersection is likely to run up against all kinds of internal optimization/robustness mitigation issues. Potentially instead this could be done by a query using a distance tolerance (possible followed by a shortest-path pruning). This might be more robust and amenable to tuning.

@dr-jts
Copy link
Contributor

dr-jts commented Oct 20, 2021

Some further investigation on this. I tried out the idea mentioned above of simply checking if the raw offset curve has any self-intersections and if not return it. This works for the case and distance (= 1) on this issue. However, it doesn't work on that case for distance = 2! So not sure if it's really worth adding that to the code.

I'm still keen on the idea of extracting portions of the full buffer curve based on a distance test (and some other logic). That will be the next prototype, so stay tuned.

@nyalldawson
Copy link
Author

Thank you @dr-jts , it's great to hear that you're investigating this! (I'd love to assist, but I don't have any useful input to make here 😆 )

@dr-jts
Copy link
Contributor

dr-jts commented Oct 21, 2021

Happy to get involved, @nyalldawson . Offset curves have been on the JTS roadmap for far too long, so it's time to get them sorted out.

One way you can perhaps help is by providing some ideas / feedback on the semantics of offset curve generation in more complicated cases. (Do you have requirements from projects you work on? Or do you have any examples from other GIS systems which produce offset curves? )

In particular, I need to decide on the following questions:

  • How should offset curves behave for input lines which self-cross? (As an example, my prototype implementation does this:
    image

  • Should the offset curve result include additional loops that result from the input line curving back on itself? For example, the current GEOS implementation produces this result, which contains extra rings formed where the input line loops back close to itself:

image

@nyalldawson
Copy link
Author

Happy to get involved, @nyalldawson . Offset curves have been on the JTS roadmap for far too long, so it's time to get them sorted out.

One way you can perhaps help is by providing some ideas / feedback on the semantics of offset curve generation in more complicated cases. (Do you have requirements from projects you work on? Or do you have any examples from other GIS systems which produce offset curves? )

In particular, I need to decide on the following questions:

  • How should offset curves behave for input lines which self-cross? (As an example, my prototype implementation does this:
    image

I guess I would expect this:

image

but honestly in the case of a self-intersecting line I'd say that any semi-reasonable output is ok.

  • Should the offset curve result include additional loops that result from the input line curving back on itself? For example, the current GEOS implementation produces this result, which contains extra rings formed where the input line loops back close to itself:

image

This is a really tricky one. I don't think a single part input should ever result in a multi-part output for this operation.

So I think the output in this case should be:

image

@dr-jts
Copy link
Contributor

dr-jts commented Oct 21, 2021

Thanks for the feedback, @nyalldawson.

I can see the argument for having the offset curve "loop back on itself" if the input does. However, that has some tricky technical challenges that I'm not sure can be overcome at the moment. But it's worth thinking about. It's also the behaviour implemented in various papers and other systems I've seen, which makes me wonder if there's reasons for that.

As for not including extra loops, that seems totally reasonable to me. It might be worth making this an option, with the single-line value being the default.

@nyalldawson
Copy link
Author

@dr-jts

I can see the argument for having the offset curve "loop back on itself" if the input does. However, that has some tricky technical challenges that I'm not sure can be overcome at the moment.

Fair enough! Like I said, I think it's entirely reasonable to output any semi-plausible curve in this circumstance. I guess another reasonable option could just be this:

image

(i.e. drop all loops first, then do the offset)

@dr-jts
Copy link
Contributor

dr-jts commented Dec 5, 2021

This issue will be fixed when locationtech/jts#810 is ported to GEOS.

@nyalldawson
Copy link
Author

Thanks @dr-jts -- that's fantastic news!

@dr-jts
Copy link
Contributor

dr-jts commented Dec 17, 2021

This should be fixed by #530. @nyalldawson Are you able to verify?

@dr-jts dr-jts added Bug JTS Issue also appears in JTS and removed JTS Issue also appears in JTS labels Dec 17, 2021
@pramsey
Copy link
Member

pramsey commented Jan 10, 2022

This still true as of 0a32f15, @nyalldawson?

@nyalldawson
Copy link
Author

I'll give this a test ASAP -- just back from leave this week and got a bit of catching up to do!

@dr-jts
Copy link
Contributor

dr-jts commented Feb 18, 2022

@nyalldawson Any chance to test this?

@dr-jts
Copy link
Contributor

dr-jts commented May 24, 2022

@nyalldawson ping?

@dr-jts
Copy link
Contributor

dr-jts commented Jun 10, 2022

Closed by #530

@dr-jts dr-jts closed this as completed Jun 10, 2022
ptitjano added a commit to ptitjano/QGIS that referenced this issue Aug 2, 2023
GEOSOffsetCurve_r used to return a wrong result for some simple single
lines (described in libgeos/geos#477). This
has been fixed in GEOS 3.11.

Update the test to check with the correct result and use the previous
result as fallback for the previous GEOS versions.
ptitjano added a commit to ptitjano/QGIS that referenced this issue Aug 2, 2023
GEOSOffsetCurve_r used to return a wrong result for some simple single
lines (described in libgeos/geos#477). This
has been fixed in GEOS 3.11.

Update the test to check with the correct result and use the previous
result as fallback for the previous GEOS versions.
ptitjano added a commit to ptitjano/QGIS that referenced this issue Aug 2, 2023
GEOSOffsetCurve_r used to return a wrong result for some simple single
lines (described in libgeos/geos#477). This
has been fixed in GEOS 3.11.

Update the test to check with the correct result and use the previous
result as fallback for the previous GEOS versions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants