Adding useful methods to points and rectangles

[stanim]

I was missing basic operations with points and rectangles. I have implemented these and like to contribute these back to your wonderful project. All added code has 100% test coverage by testable examples: https://blog.golang.org/examples

So all added methods are tested and well documented at the same time.

[dmitshur]

Why doesn't this example have "Output: " prefix?

[stanim]

Good catch. I've fixed it. Also in my branch I had fixed several golint issues already, so you'll get these as well. (https://splice.com/blog/going-extra-mile-golint-go-vet/)

[akavel]

Soooo; there are quite some aspects to this; please read below:

First of all: to tell the truth, after some time I realized I'm not really happy that I left any public methods on Point and Rectangle. The point is, I'd really like the API of this package to be minimal, and focused on what's important. Also to make it as easy as possible to use the package, for a newcomer who's just looking at the godocs. I've seen quite a lot of packages with dozens of methods, where I totally have no idea how to use them and what they do. But please don't worry yet, stay with me for a moment longer.

Now; I think that it could still be possible to keep your work compatible with this package. Specifically, I think that maybe it could be possible to put those functions either in a subpackage (e.g. .../polyclip-go/geom, or something), or you could maybe put them in a separate package on your account (github.com/stanim/geom, or something, and I could mention it in polyclip's docs). Making it work with Point could be done without actually linking those packages; but for compatibility with polyclip's Rectangle, I think this pkg geom would have to import polyclip-go (and have something like type Rectangle polyclip.Rectangle). So - I could agree to any of those two solutions; but I'm not very happy to include it straight in polyclip. What would you think?

Now, I still have some smaller stuff to comment on:

1. I'd very much prefer if you could possibly rewrite the commits in the pull request (with rebase -i, splitting, etc; or by hand committing appropriate parts) in a way kinda like listed below. It should make it much easier for me to grasp and try reviewing.
   1. First, a commit with just the golint/govet changes to the existing codebase (this could probably be even a totally separate pull rq).
   2. A commit with only Point methods
   3. A commit with only tests for Point
   4. A commit with only Rectangle methods
   5. A commit with only tests for Rectangle
   6. Cosmetic changes (in comments, etc.) could go with the golint/govet commit/PR.
2. I don't really see the point of ZP & ZR. One can just use Point{}, Rectangle{}, and that'd be easier to understand. Keeping ZP&ZR also looks to me like an invitation for users to change it.
3. More stuff may come after you make the split.

That said, there's one more thing to it: some of those operations are actually already used in "inlined form" in various algorithms in the code. It could be interesting to try changing them to actually use the funcs. But it's probably much work, and also I'm not sure how to avoid code duplication then, and keep no import cycles, if I preferred to leave those funcs as private ones. Maybe some subpackage named "internal", but still not sure how to do that for Rect then, unfortunately, without import cycles.

Sorry for a somewhat chaotic response, but I wanted to reply quickly, so that you had some feedback (alternative would be for me to reply "at undefined time in future, possibly never", unfortunately :/)

Thanks.

[stanim]

Fair enough. This was an honest answer, which is what I prefer. I can completely understand to keep polyclip-go minimal and focused.

About the big picture ...

To keep my work compatible with this package is not straightforward. If it moves to either a subpackage or separate package, all private methods are inaccessible. Something like "type Rectangle polyclip.Rectangle" doesn't help either as the methods of this Rectangle still require e.g. a polyclip.Point instead of for example "type Point polyclip.Point".

So the challenge is put simply:

• A minimal package with private methods. Only core functionality is public. In this case for example Point or Rectangle would not have public methods and even attributes (if wished)
• A maximal package with public methods, which I name stupidly "max" for clarity.

Maybe the only way to keep packages compatible without needing a fork is by using interface arguments instead of concrete types, like gonum does for Matrix:
http://godoc.org/github.com/gonum/matrix/mat64#Matrix
http://godoc.org/github.com/gonum/matrix/mat64#Det

The following structure would be a solution:

• polyclip (root package with minimal api, Point or Rectangle would not have any public method or even attributes)
• polyclip/max (subpackage with public methods and more added functionality)
• polyclip/interfaces (subpackage with only interfaces, not types or code)
• polyclip/internal (only functions which are needed both by polyclip and polyclip/max, similarly to stdlib cmd/nm and cmd/objdump which both import cmd/internal/objfile)

There is no import cycle as:

• polyclip imports polyclip/interfaces and polyclip/internal
• polyclip/max imports polyclip/interfaces and polyclip/internal

Also if somebody imports polyclip, polyclip/max is not imported.

Let's take Equals() for Point as an example.

In polyclip/interfaces:

type Point interface {
  X() float64
  Y() float64
}

In polyclip:

type Point struct {
    x, y float64
}

func (p Point) X() float64 {
  return p.x
}

func (p Point) Y() float64 {
  return p.y
}

In polyclip/internal:

func Equals(p, q interfaces.Point) bool {
    return p.X() == q.X() && p.Y() == q.Y()
}

In polyclip/max:

type Point struct {
    x, y float64
}

func (p Point) X() float64 {
  return p.x
}

func (p Point) Y() float64 {
  return p.y
}

func (p Point) Equals(q Point) bool {
    return internal.Equals(p, q)
}

So public methods would expose the internal functions. Anything you don't want to expose stays hidden.

If you have another working solution without a fork, I am very interested. Otherwise maybe the price of this too high and we could agree to disagree (minimal api with private methods versus expansive api with public methods). Maybe a gentle fork is than the best solution, where we sync manually the common parts and refer to each other. I expect this is feasible as polyclip-go is an implementation of an algorithm, which seems fixed on a certain paper. So except from fine-tuning some bugs, not a lot of changes will happen.

About the smaller stuff:

1. No problem to do a separate pull request for golint/govet. The other commits have little sense if you prefer not to include this code as public methods into your package.
2. ZP and ZR are useful as singletons. For example if the intersection method of two non-overlapping rectangles returns a ZR, it is more efficient to check the result with "rect == ZR" instead of "rect.Equals(Rectangle{})". This is a common pattern also used in the standard image library: https://golang.org/src/image/geom.go?s=3350:3401#L136

[dmitshur]

@stanim, in the polyclip/max code you posted above, you'd get an type Point has both field and method named X error. You're also missing () after methods that take 0 parameters, I assume that's a typo.

[stanim]

@shurcooL Thanks again.

[akavel]

Big picture

Actually, my "v2" API idea was like below - even getting rid of the Rectangle entirely:

type Point struct { X, Y float64 }
type Contour []Point
type Poly []Contour
func Union(p1, p2 Poly) Poly
func Intersection(p1, p2 Poly) Poly
func Xor(p1, p2 Poly) Poly
func Difference(p1, p2 Poly) Poly

Then, a 100% separate package could exist, which would have a duplicated Point type. They're then assignable (I tested on Playground), so I believe it should work (hopefully without copying?...), however clumsily, via e.g.:

p := Point{11,22}
func cast(p Point) geom.Point { return geom.Point(p) }
q := cast(p).Add(cast(Point{3,4}))

This separate pkg could then have all kinds of fancy methods on the Points, and polyclip could use them internally. Also it could have Rect, and again polyclip could use it internally. This would thus, I believe, solve our problems; at a cost of forking the API to a "v2" (probably in a separate repo). What would you think about going this way?

Actually, I do also have some fondness towards adding basic drawing functions, like:

type DrawHline func(x0, y, x1 int)
func DrawFilled(p Poly, hline DrawHline)

type DrawPixel func(x, y int)
func DrawLine(x0, y0, x1, y1 int, brush DrawPixel)
func DrawOutline(p Poly, brush DrawPixel)

but then again, I'd very much prefer to keep them in a separate repo. With all of those taken together, I'm wondering how it would be best to structure them:

polyclip-v2
polyclip-v2/geom
polyclip-v2/draw

or maybe:

geom
geom/clip
geom/draw

or:

poly/geom
poly/clip
poly/draw

?

I expect this is feasible as polyclip-go is an implementation of an algorithm, which seems fixed on a certain paper. So except from fine-tuning some bugs, not a lot of changes will happen.

Yes, that's the plan.

Also, I'm not very happy going the way of func (p Point) X() float64 - first of all, it changes the API already; if we want to change it, then I'd prefer to go the way I described above; simplifying the API this way looks much more "idiomatic Go" to me, while func X() smells more Java to me, and also would impose a performance cost, unnecessary in my opinion. If left only with choice between this and a "gentle fork" as you kindly described it, the fork way would look much more appealling to me.

Smaller stuff

Wow, I'm surprised, ZR really is there in pkg image; I wonder why? Actually, the "singleton" explanation isn't correct AFAIK; in Go, just rect1 == rect2 should work properly, see: http://play.golang.org/p/sRKh-wQ0M2. But even given pkg image, personally I'd prefer to avoid adding it; if just because it can always be added later without breaking the API, but can't be removed.

[stanim]

Then, a 100% separate package could exist, which would have a duplicated
Point type. They're then assignable (I tested on Playground)
Can you share this assignable test on playground?

[stanim]

I agree completely that converting points everywhere feels clumsy. Let's not 'go' that way. Than I would prefer that in Union, Intersection, ... the Poly arguments are converted to geom.Poly's, so you are sure every point is only converted once. So the geom package would also have the same types (Contour&Poly) and the clipping functions (Union, Intersection, ...). Only the geom package has all the public methods. In that case the poly clip package imports the geom package, but not vice versa.

I am still wondering if a fork is not better. To keep everything as one package might give tensions about its scope. Many public methods I proposed for Point and Rectangle won't be used by the clipping functions and only add bloat. I want to use polyclip-go as a base for a geom package which can do much more than clipping, for which I need these extra methods. For example my geom package also needs to do:

• curve fitting
• converting curves to polygons to do clipping operations and convert them back to curves
• a Circle type with methods (e.g. to find intersection points)
• ...

(For an example of my work, see http://pythonide.blogspot.nl/2008/10/how-to-make-money-with-free-software.html)

So in the geom package I want to have the freedom to add whatever I need, which might conflict with your focus.

However ... even in case of a fork your api v2 would be much better starting point to fork from, although I would slightly change it to:

type Point struct { X, Y float64 }
type Contour []Point
type Poly []Contour
func (p Poly) Union(q ...Poly) Poly
func (p Poly) Intersection(q ...Poly) Poly
func (p Poly) Xor(q ...Poly) Poly
func (p Poly) Difference(q ...Poly) Poly

In this package all methods are private (e.g.Point.length) except from the above Poly methods. After this api is implemented, I do a fork where the relevant private methods become public and where I can add other functionality (curve fitting, circles, ...). I also think it might be better to use pointer arguments as the Poly data can be quite huge (like world map in the README) to avoid unnecessary copying which slows down the execution and increases memory consumption.

I am not happy to integrate a drawing subpackage. For that I prefer to use an external package, such as the draw2d package: https://github.com/llgcode/draw2d I just wrote a pdf backend for it, which will be merged soon. So draw2d supports images, pdf and opengl. Let's concentrate first on the v2 api and we can evaluate drawing afterwards. I could bridge draw2d and polyclip in a separate package.

About ZR ... you are surprised because you focus on the singleton aspect, rather than on the efficiency argument. Maybe I was expressing myself not clear enough about the singleton. (One way of implementing singletons in Go is through global variables, such as ZR. See also https://groups.google.com/d/msg/golang-nuts/PcuRWl36zdc/iCJpnUH-7eQJ. But that is not important). If you compare the two processes. With Rectangle{} instead of ZR, you have first to create a struct which has later to be garbage collected. So using ZR is just a way to avoid these unnecessary steps and (micro) optimize your code. If you prefer to be convinced by numbers, not using ZR is 20% slower: https://gist.github.com/stanim/7abe87be623c08f2d1a2. ZP and ZR would not be exposed in the v2 api, only in the geom package. It is not the end of the world. I am not religious about it.

[akavel]

Forking

I am still wondering if a fork is not better. (...) I want to use polyclip-go as a base for a geom package which can do much more than clipping, for which I need these extra methods. For example my geom package also needs to do:

• curve fitting
• converting curves to polygons to do clipping operations and convert them back to curves
• a Circle type with methods (e.g. to find intersection points)
• ...

So in the geom package I want to have the freedom to add whatever I need, which might conflict with your focus.

Ahhhhhh, good that you wrote about that. Then, I totally believe that forking by you is the best course of action to take here, please go on with that! It's great that you have interesting ideas and motivation to grow the package, so it will be great if you make it useful for you, as this may also make it more useful to other people in future. I'll be happy to link to your package from my one, as a continuation and extension of the work.

Just one thing I'd wish to ask of you: please do clearly mention issue #3 in the docs for the clipping function(s). As far as I understand, and from some additional emails from the original author of the algorithm: the algorithm currently does not support polygons with self-overlapping edges (i.e. if some edge covers another one; just crossing is OK). This probably includes all "variants" of that:

• edge wholly or partially covers another edge in the same Contour;
• edge wholly or partially covers another edge in a different Contour of the same Polygon.

In such a case, _the resulting Polygon may be incorrect_ (see #3 for an example).

That's the only thing I'd like to fix in this package at some point in future, but it's not easy for me, and I can't find enough time and motivation for that for now.

Uhmh, and I'd sure be grateful for some credit in the readme or somewhere :)

(For an example of my work, see http://pythonide.blogspot.nl/2008/10/how-to-make-money-with-free-software.html)

Wow, congrats on the coin! :)

ZR

I've commented in the gist.

That said, if you're going to fork, that's your toy now, so do as you wish with the ZR or whatever :)

Good luck!

Please let me know when your package will be ready enought that I can link to it from my readme. If you're OK with all of the above, I'll close this PR.

[stanim]

You can close this pull request. It was my pleasure. I'll keep you updated.