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

image: Rectangle.Union() doesn't handle empty rectangles correctly #9895

Closed
robzan8 opened this issue Feb 16, 2015 · 7 comments
Closed

image: Rectangle.Union() doesn't handle empty rectangles correctly #9895

robzan8 opened this issue Feb 16, 2015 · 7 comments
Assignees

Comments

@robzan8
Copy link

@robzan8 robzan8 commented Feb 16, 2015

The function for calculating the union of two rectangles returns wrong values when one or both input rectangles are empty. The code in image/geom.go is currently:

func (r Rectangle) Union(s Rectangle) Rectangle {
    if r.Min.X > s.Min.X {
        r.Min.X = s.Min.X
    }
    if r.Min.Y > s.Min.Y {
        r.Min.Y = s.Min.Y
    }
    if r.Max.X < s.Max.X {
        r.Max.X = s.Max.X
    }
    if r.Max.Y < s.Max.Y {
        r.Max.Y = s.Max.Y
    }
    return r
}

To be correct a couple of lines should be added at the beginning of the function:

    if r.Empty() {
        return s
    }
    if s.Empty() {
        return r
    }

Is this a bug, or is the case ignored for performance reasons? If it's the latter, I think a warning should be added in the function documentation.

@mikioh mikioh changed the title image.Rectangle.Union() doesn't handle empty rectangles correctly image: Rectangle.Union() doesn't handle empty rectangles correctly Feb 16, 2015
@jnjackins
Copy link
Contributor

@jnjackins jnjackins commented Feb 17, 2015

I think it's correct as is. An empty rectangle is still a rectangle, and the function documentation says the resulting rectangle will contain both. With your addition, that's not necessarily true.

Loading

@robzan8
Copy link
Author

@robzan8 robzan8 commented Feb 17, 2015

Yeah, I was thinking about it, I'm not sure either. But, for an empty rectangle the position does not matter, it contains no pixels ( as explained here http://blog.golang.org/go-image-package ). If we define the union of two rectangles as the minimum rectangle that contains all the pixels of both, then the union of two empty rectangles should be empty, mathematically speaking it makes more sense. Also practically speaking maybe, if you imagine that you want to calculate the union of a slice of rectangles, you should be able to initialize an empty rectangle as accumulator and then simply iterate over the slice.

Loading

@nigeltao
Copy link
Contributor

@nigeltao nigeltao commented Feb 17, 2015

I think it is indeed a bug. Please see https://go-review.googlesource.com/#/c/4990/ for a fix (and test).

Loading

@robzan8
Copy link
Author

@robzan8 robzan8 commented Feb 17, 2015

Looks good to me.

Loading

@robzan8
Copy link
Author

@robzan8 robzan8 commented Feb 17, 2015

Do you think that Rectangle.Eq() should be true for two empty but "different" rectangles or am I pushing it too far? :)

Loading

@nigeltao nigeltao closed this in 5c8f9e3 Feb 17, 2015
@nigeltao
Copy link
Contributor

@nigeltao nigeltao commented Feb 17, 2015

Changing Rectangle.Eq is https://go-review.googlesource.com/5006

Loading

@robzan8
Copy link
Author

@robzan8 robzan8 commented Feb 17, 2015

Great.

Loading

@golang golang locked and limited conversation to collaborators Jun 25, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants