-
Notifications
You must be signed in to change notification settings - Fork 200
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
Implement Offset Algorithm #935
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @thehappycheese, welcome and thanks for the contribution! I've left a load of review comments – please don't feel that you need to address them all at once. In particular, it looks like you're having some trouble using the Kernel
trait, and this might be a good place to start. I and other contributors are happy to help here, and we can do it inline in the PR, and / or on Discord: https://discord.gg/Fp2aape
// TODO: I'm still confused about how to use Kernel / RobustKernel; | ||
// the following did not work. I need to read more code | ||
// from the rest of this repo to understand. | ||
// if Kernel::orient2d(*a, *b, *d) == Orientation::Collinear { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Orientation
is an enum, so you have to use a match
for control flow.
something like
match Kernel::orient2d(*a, *b, *c) {
Orientation::Collinear => { // keep going }
_ => { // not collinear, bail out or test different combination?? }
}
As you note, the use of arbitrary "magic" threshold values without considerable justification and accompanying comments explaining the reasoning behind it for future maintainers is unlikely to be accepted (I've just finished removing an algorithm containing a lot of these that I had the misfortune of writing myself…), so if we can use existing mechanisms that would be much better.
@@ -0,0 +1,85 @@ | |||
use crate::CoordFloat; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused about this. Would the dot product sign, i.e. the Orientation suffice? It looks like you're just checking whether the result is -1
, 1
, or 0
…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, looks like my inept git-fu has disconnected all your comments from the code 😕 I think maybe re-basing the branch on my fork was the wrong thing to do.
cross_product
simplifies line_segment_intersection_with_parameters()
: While it is used for a collinear check at the start of that function (which could be replaced with Kernel::dot_product_sign
or Kernel::orient2d
) it is used twice more further along to calculate the parameters t_ab
and t_cd
so I do not believe it can be substituted for an orientation check.
let t_ab = cross_product_2d(ac, cd) / ab_cross_cd;
let t_cd = -cross_product_2d(ab, ac) / ab_cross_cd;
line_segment_intersection_with_parameters()
is currently only used via a wrapper function line_segment_intersection_with_relationships()
; in that function it checks each t_ab
and t_cd
to find which of three ranges they lie in:
- less than zero
- between zero and one
- greater than one
It's possible there is some tricky way to express this check in terms of orient2d()
, I'll have a think about it.
It does feel weird to have cross_product()
out in it's own module. In the past I have worked with libraries that have a Vector2
type where this kind of function would be implemented. It is simple enough to be inline perhaps, but I wrote up all the tests and documentation because in the past I have made many mistakes with putting negatives in the wrong place and if you make this kind of mistake an even number of times you might not realize since it cancels out 😋
geo/CHANGES.md
Outdated
@@ -11,6 +11,7 @@ | |||
* <https://github.com/georust/geo/pull/928> | |||
* Added `RemoveRepeatedPoints` trait allowing the removal of (consecutive) | |||
repeated points. | |||
* Added very basic `Offset` trait and implemented it for `Line` and `LineString` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A minor nit: there's no need to qualify this as "very basic", since the docs will make the limitations clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I will try to reword it to be more specific, or be more clear about any limitations in the rest of the docs
/// | ||
/// Signed offset of Geometry assuming cartesian coordinate system. | ||
/// | ||
/// This is a cheap offset algorithm that is suitable for flat coordinate systems |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no need to specify this as geo
operates only on coordinates in the real plane (with the exception of some explicitly documented convenience algorithms for spherical coordinates). There is an explicit assumption that only projected / cartesian coordinates are used. That this algorithm may be suitable for coordinates near the equator is a coincidence as far as the library is concerned.
/// some may be removed during development, | ||
/// others are very hard to fix. | ||
/// | ||
/// - No checking for zero length input. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a specific difficulty you've encountered while trying to implement a check against this?
/// others are very hard to fix. | ||
/// | ||
/// - No checking for zero length input. | ||
/// Invalid results may be caused by division by zero. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where could this occur?
/// | ||
/// - No checking for zero length input. | ||
/// Invalid results may be caused by division by zero. | ||
/// - No check is implemented to prevent execution if the specified offset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will need a check for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have added this check to LineString but it probably isn't worth it for Line. I have added comments to the relevant code in my next commit.
/// distance is zero. | ||
/// - Only local cropping where the output is self-intersecting. | ||
/// Non-adjacent line segments in the output may be self-intersecting. | ||
/// - There is no mitre-limit; A LineString which |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a fairly significant limitation, but if the algorithm can't handle it, then we should consider a check for self-crossing LineStrings (I believe we already have one) and bail out early if this is encountered.
where | ||
T: CoordFloat, | ||
{ | ||
fn offset(&self, distance: T) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My expectation is that a positive (or outward) offset on a LineString produces a Polygon
(as per JTS, GEOS, and derived libraries). Could you explain the reasoning here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no context on how JTS/GEOS/etc does things, but offsetting a LineString and getting a LineString has many use cases. If you start with something representing the center of a road and want the left or right edge, or the center of a lane, for example. To buffer around a LineString and get a polygon, you could offset inwards (the hole) and outwards (the exterior), right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dabreegster I'm struggling to visualise that. Do you have an example? The standard implementations of offset (in plain language, buffer) operations always produce a polygon from a Line or LineString as far as I know. The Shapely docs are representative here: https://shapely.readthedocs.io/en/stable/manual.html#object.buffer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disclaimer that I haven't looked at this implementation, but just a note on terminology...
JTS has an "offset curve" which is a line string to line string transformation.
a line lying on one side of another line at a given offset distance
http://lin-ear-th-inking.blogspot.com/2022/01/jts-offset-curves.html?m=1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @urschrei thank you so much for all your feedback I really appreciate it, I will do my best to address each one :)
Regarding the sign of the specified offset, I agree that it would be strange if a negative offset resulted in an inset for polygons. I am certainly open to changing this, I will add it to the TODO list in my PR.
On this @dabreegster has already explained my reasoning really well; To me a buffer operation different to an offset; I am interested in offsetting road center lines, not producing enclosed polygons.
I recognize that a buffer operation is more generally useful, but I also just need this signed offset function. This offset trait can be extended by a a few clipping and stitching steps to a full buffer operation, this is the end goal of the paper I am following.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm struggling to visualise that. Do you have an example?
https://a-b-street.github.io/docs/tech/map/geometry/index.html#projecting-a-polyline, and for the inspiration behind it, https://wwwtyro.net/2019/11/18/instanced-lines.html. The JTS offset curve looks like the same thing. I don't know what this linestring -> linestring operation should be called, but I have plenty of use cases for it, as well as buffering (to get a polygon)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks all, it looks like it's just confusion about terminology, mostly on my part. It might be worth discussing whether the current trait name conveys the functionality clearly enough, though maybe it's just a question of ensuring that the module-level docstring does this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thehappycheese By the way, your screenshot link doesn't appear to work: https://github.com/thehappycheese/powerbi-visual-geojson-map-1/raw/main/git_docs/hero_image.jpg (404)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keen to align terminology with suggestions / other libraries :)
offset_curve
is nice because it makes it more obvious why this type of signed offset cannot be implemented for a Point
object. Point
objects can only have a "buffer" or "outset" applied, not an 'offset_curve'.
Ah apologies for the broken link I picked it from one of my public repos but didn't realize the image link is to a private repo; this one should work?
This is from a custom PowerBI visual I created. I will eventually make that code public when I get a chance to clean up /republish repo. I want the PowerBI visual to dynamically connect to a rust server for a specialized kind of chainage-based geo-coding.
052d65f
to
c0c1e48
Compare
I have deployed an interactive tester thing here https://geo-tester.thehappycheese.com/ Source code here: https://github.com/thehappycheese/geo-offset-test-server Hoping it will help me find edge cases as I start to implement the miter limit an clipping step of the algorithm. To be honest though after playing with it for a bit, I am a little worried that some edge cases are going to be pretty challenging. |
As an aside, Wow, that's a really nice demo! We've talked in the past about having interactive tools like this as a kind of documentation for some of our algorithms, but I think think this is the first actual working example. 👏 👏👏 |
Hi @thehappycheese - this is an exciting new feature that I think a lot of people would like to use. Are you still interested in working on it? Is there anything I can do to help move it along? |
Hi @michaelkirk 😃 I have to admit I stalled out on this project; There are a few contributing factors
I am still keen to get back into this but it might take me a few more weeks to get going again. In the meantime, if anyone is keen to jump in I would be happy to collaborate in any way possible :) |
7ca3f66
to
d95420f
Compare
My current draft PR is stalled because it got a bit too messy with too many new features at once. In this comment I am trying to figure out if I can break off smaller bite-sized contributions and have a go at those first. Required for Simple Offset
Required for Clipped Offset
|
1025: Vector2DOps Trait - Proposal r=michaelkirk a=thehappycheese - [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/main/CODE_OF_CONDUCT.md). - [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users. --- Proposal to define `Vector2DOps` trait and implement it for the `Coord` struct. Intention is to facilitate general purpose linear algebra operations such as 'dot product' etc all in one place. Most of these functions are are already implemented in various places throughout the crate or sometimes hard-coded in place. This would bring them all together. For example - `wedge_product`( aka cross/perp/exterior product) `geotypes::Line::determinant`, - `geotypes::Point::cross_prod` (which takes three arguments for some reason) etc. - Also used in the default implementation of Kernel orient2d, although this serves a different purpose as it returns an enum - `dot_product` `geotypes::Point::dot`, - `magnitude` in `Line::line_euclidean_length` - `magnitude_squared` hard coded ( [here](https://github.com/thehappycheese/geo/blob/9b477fc7b24fcb8731bd5e01d559b613e79ada0c/geo/src/algorithm/line_locate_point.rs#L61C9-L61C29)) - `normalize` ... interestingly it is hard to find an example of this operation in the existing crate. Possibly because it is generally best avoided due to floating point instability. For more discussion about motivation for this proposal, see [this comment](#935 (comment)) on PR #935 . Possibly the timing of this proposal is weird, I am sure that the `geotraits::CoordTrait` will affect how this should be implemented. I suspect it may allow the `Vector2DOps` to define default implementations? Note: - Currently `Vector2DOps` isn't used anywhere so it will trigger dead code warnings. - For an example that illustrates how I imagine it would be used in the OffsetCurve trait, see [this snippet](https://github.com/thehappycheese/geo/blob/d95420f1f8d6b0d2dd1e6d803fe3d3e2a2b3dd13/geo/src/algorithm/offset_curve/offset_line_raw.rs#L46C1-L51C1) from this PR #935 - Note that PR #935 is a separate, older branch, in that branch I was calling it `VectorExtensions`. In this PR I renamed it to `Vector2DOps` so that it is more similar to the existing `AffineOps` Many thanks for your time and any feedback you can provide Co-authored-by: thehappycheese <[email protected]>
1025: Vector2DOps Trait - Proposal r=michaelkirk a=thehappycheese - [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/main/CODE_OF_CONDUCT.md). - [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users. --- Proposal to define `Vector2DOps` trait and implement it for the `Coord` struct. Intention is to facilitate general purpose linear algebra operations such as 'dot product' etc all in one place. Most of these functions are are already implemented in various places throughout the crate or sometimes hard-coded in place. This would bring them all together. For example - `wedge_product`( aka cross/perp/exterior product) `geotypes::Line::determinant`, - `geotypes::Point::cross_prod` (which takes three arguments for some reason) etc. - Also used in the default implementation of Kernel orient2d, although this serves a different purpose as it returns an enum - `dot_product` `geotypes::Point::dot`, - `magnitude` in `Line::line_euclidean_length` - `magnitude_squared` hard coded ( [here](https://github.com/thehappycheese/geo/blob/9b477fc7b24fcb8731bd5e01d559b613e79ada0c/geo/src/algorithm/line_locate_point.rs#L61C9-L61C29)) - `normalize` ... interestingly it is hard to find an example of this operation in the existing crate. Possibly because it is generally best avoided due to floating point instability. For more discussion about motivation for this proposal, see [this comment](#935 (comment)) on PR #935 . Possibly the timing of this proposal is weird, I am sure that the `geotraits::CoordTrait` will affect how this should be implemented. I suspect it may allow the `Vector2DOps` to define default implementations? Note: - Currently `Vector2DOps` isn't used anywhere so it will trigger dead code warnings. - For an example that illustrates how I imagine it would be used in the OffsetCurve trait, see [this snippet](https://github.com/thehappycheese/geo/blob/d95420f1f8d6b0d2dd1e6d803fe3d3e2a2b3dd13/geo/src/algorithm/offset_curve/offset_line_raw.rs#L46C1-L51C1) from this PR #935 - Note that PR #935 is a separate, older branch, in that branch I was calling it `VectorExtensions`. In this PR I renamed it to `Vector2DOps` so that it is more similar to the existing `AffineOps` Many thanks for your time and any feedback you can provide Co-authored-by: thehappycheese <[email protected]>
1025: Vector2DOps Trait - Proposal r=michaelkirk a=thehappycheese - [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/main/CODE_OF_CONDUCT.md). - [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users. --- Proposal to define `Vector2DOps` trait and implement it for the `Coord` struct. Intention is to facilitate general purpose linear algebra operations such as 'dot product' etc all in one place. Most of these functions are are already implemented in various places throughout the crate or sometimes hard-coded in place. This would bring them all together. For example - `wedge_product`( aka cross/perp/exterior product) `geotypes::Line::determinant`, - `geotypes::Point::cross_prod` (which takes three arguments for some reason) etc. - Also used in the default implementation of Kernel orient2d, although this serves a different purpose as it returns an enum - `dot_product` `geotypes::Point::dot`, - `magnitude` in `Line::line_euclidean_length` - `magnitude_squared` hard coded ( [here](https://github.com/thehappycheese/geo/blob/9b477fc7b24fcb8731bd5e01d559b613e79ada0c/geo/src/algorithm/line_locate_point.rs#L61C9-L61C29)) - `normalize` ... interestingly it is hard to find an example of this operation in the existing crate. Possibly because it is generally best avoided due to floating point instability. For more discussion about motivation for this proposal, see [this comment](#935 (comment)) on PR #935 . Possibly the timing of this proposal is weird, I am sure that the `geotraits::CoordTrait` will affect how this should be implemented. I suspect it may allow the `Vector2DOps` to define default implementations? Note: - Currently `Vector2DOps` isn't used anywhere so it will trigger dead code warnings. - For an example that illustrates how I imagine it would be used in the OffsetCurve trait, see [this snippet](https://github.com/thehappycheese/geo/blob/d95420f1f8d6b0d2dd1e6d803fe3d3e2a2b3dd13/geo/src/algorithm/offset_curve/offset_line_raw.rs#L46C1-L51C1) from this PR #935 - Note that PR #935 is a separate, older branch, in that branch I was calling it `VectorExtensions`. In this PR I renamed it to `Vector2DOps` so that it is more similar to the existing `AffineOps` Many thanks for your time and any feedback you can provide Co-authored-by: thehappycheese <[email protected]>
CHANGES.md
if knowledge of this change could be valuable to users.Defines an
OffsetCurve
trait and implements the associated function.offset_curve(distance)
for some geometry types. Theoffset_curve
function offsets the edges of a geometry by a specifieddistance
, where the sign ofdistance
determines if the edges of the geometry are offset to the 'left' or 'right'.'Offset curve' is like a 'one-sided buffer' and, unlike buffer, it is only meaningful for some geometry types. The table below summarises the different outputs.
A 'clipped offset curve' has had parts of the output removed to ensure that all edges in the output are at least
distance
from the input, even if the input is self-overlapping. So far, this clipping step has not been implemented.The following table is a work in progress:
Point
Polygon
)Line
Line
Line
Polygon
)LineString
LineString
(possibly self overlapping)
MultiLineString
Polygon
)Polygon
Option<Polygon>
(always closed, but possibly malformed)
Option<Polygon>
(possibly unclosed, possibly malformed)
Polygon
GEOS and JTS
Choice of Algorithm
I am not sure if the algorithm I am trying to implement here is any better or worse than those, but it is one I have got working in the past in python so I figure I have the best chance of success with it. It loosely follows the algorithm described by
Xu-Zheng Liu, Jun-Hai Yong, Guo-Qin Zheng, Jia-Guang Sun. An offset algorithm for polyline curves. Computers in Industry, Elsevier, 2007, 15p. inria-00518005
Trait Implementations
Line<T>
LineString<T>
MultiLineString<T>
Triangle<T>
Rectangle<T>
Polygon<T>
MultiPolygon<T>
Point<T>
MultiPoint<T>
Geometry<T>
GeometryCollection<T>
For coordinate types
where T:CoordFloat
where T:CoordNum
(??? seems tricky)Issues to Fix:
Offset
toOffsetCurve
to match terminology inGEOS
and
jts
SimpleKernel
/RobustKernel
LineString
which doubles-back onitself will not produce an elbow at infinity
are not self-intersecting.
Hopefully I have started off my contribution in the right direction? :)
Many thanks for your time