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.

Sign up for GitHub

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

Jump to bottom

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
Labels
FrozenDueToAge

Comments

@robzan8
Copy link

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

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.

@robzan8
Copy link
Author

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.

@nigeltao
Copy link
Contributor

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

@robzan8
Copy link
Author

robzan8 commented Feb 17, 2015

Looks good to me.

@robzan8
Copy link
Author

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? :)

@nigeltao
Copy link
Contributor

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

@robzan8
Copy link
Author

robzan8 commented Feb 17, 2015

Great.

@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.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@jnjackins @nigeltao @gopherbot @robzan8