code review 1918047: image: replace Width and Height by Bounds, and introduc...

image: replace Width and Height by Bounds, and introduce the Point and
Rect types.

The actual image representation is unchanged. A future change will
replace the {[][]color} with {[]color, stride int, r Rectangle} and
possibly a clip region.

The draw.Color, draw.Point and draw.Rect types will be removed in a
future change. Trying to do it in this one polluted the diff with
trivia.

[nigeltao, 2010-08-09 12:08:05]

Hello r, rsc (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[nigeltao, 2010-08-09 12:09:34]

http://codereview.appspot.com/1918047/diff/2001/3011
File src/pkg/image/names.go (right):

http://codereview.appspot.com/1918047/diff/2001/3011#newcode28
src/pkg/image/names.go:28: func (c ColorImage) Bounds() Rectangle { return
Rectangle{Point{0, 0}, Point{1e9, 1e9}} }
Should the "0"s be "-1e9"s?

[r, 2010-08-09 23:32:56]

http://codereview.appspot.com/1918047/diff/11001/12006
File src/pkg/exp/nacl/av/image.go (right):

http://codereview.appspot.com/1918047/diff/11001/12006#newcode29
src/pkg/exp/nacl/av/image.go:29: return image.Rectangle{image.Point{0, 0},
image.Point{0, 0}}
ZR

http://codereview.appspot.com/1918047/diff/11001/12006#newcode31
src/pkg/exp/nacl/av/image.go:31: return image.Rectangle{image.Point{0, 0},
image.Point{len(m.Pixel[0]), len(m.Pixel)}}
use ZP

http://codereview.appspot.com/1918047/diff/11001/12009
File src/pkg/image/geom.go (right):

http://codereview.appspot.com/1918047/diff/11001/12009#newcode11
src/pkg/image/geom.go:11: // A Point is an X, Y coordinate pair.
increases right and down

http://codereview.appspot.com/1918047/diff/11001/12009#newcode16
src/pkg/image/geom.go:16: // String returns a string representation of p.
comment could include example: "(3,4)"

http://codereview.appspot.com/1918047/diff/11001/12009#newcode75
src/pkg/image/geom.go:75: // Inset returns the rectangle r inset by n.
, which can be negative.

http://codereview.appspot.com/1918047/diff/11001/12009#newcode106
src/pkg/image/geom.go:106: }
you need a canonicalizer that reorders the coordinates to make the rectangle
have bounds.
if max.x < min.x, flip x; similarly for y

http://codereview.appspot.com/1918047/diff/11001/12010
File src/pkg/image/image.go (right):

http://codereview.appspot.com/1918047/diff/11001/12010#newcode16
src/pkg/image/image.go:16: // It is illegal to pass an (x, y) that is not in an
Image's Bounds.
what does illegal mean?

i'd prefer you define the image as zero outside the bounds.

http://codereview.appspot.com/1918047/diff/11001/12010#newcode32
src/pkg/image/image.go:32: return Rectangle{Point{0, 0}, Point{0, 0}}
ZR

http://codereview.appspot.com/1918047/diff/11001/12010#newcode34
src/pkg/image/image.go:34: return Rectangle{Point{0, 0}, Point{len(p.Pixel[0]),
len(p.Pixel)}}
ZP

http://codereview.appspot.com/1918047/diff/11001/12010#newcode78
src/pkg/image/image.go:78: return Rectangle{Point{0, 0}, Point{0, 0}}
ZR

http://codereview.appspot.com/1918047/diff/11001/12010#newcode80
src/pkg/image/image.go:80: return Rectangle{Point{0, 0}, Point{len(p.Pixel[0]),
len(p.Pixel)}}
ZP

http://codereview.appspot.com/1918047/diff/11001/12010#newcode342
src/pkg/image/image.go:342: return Rectangle{Point{0, 0}, Point{0, 0}}
ZR

http://codereview.appspot.com/1918047/diff/11001/12010#newcode344
src/pkg/image/image.go:344: return Rectangle{Point{0, 0}, Point{len(p.Pixel[0]),
len(p.Pixel)}}
ZP

http://codereview.appspot.com/1918047/diff/11001/12010#newcode414
src/pkg/image/image.go:414: return Rectangle{Point{0, 0}, Point{0, 0}}
ZR

http://codereview.appspot.com/1918047/diff/11001/12010#newcode416
src/pkg/image/image.go:416: return Rectangle{Point{0, 0}, Point{len(p.Pixel[0]),
len(p.Pixel)}}
ZP

[nigeltao, 2010-08-10 00:43:52]

http://codereview.appspot.com/1918047/diff/11001/12006
File src/pkg/exp/nacl/av/image.go (right):

http://codereview.appspot.com/1918047/diff/11001/12006#newcode29
src/pkg/exp/nacl/av/image.go:29: return image.Rectangle{image.Point{0, 0},
image.Point{0, 0}}
On 2010/08/09 23:32:56, r wrote:
> ZR

Done.

http://codereview.appspot.com/1918047/diff/11001/12006#newcode31
src/pkg/exp/nacl/av/image.go:31: return image.Rectangle{image.Point{0, 0},
image.Point{len(m.Pixel[0]), len(m.Pixel)}}
On 2010/08/09 23:32:56, r wrote:
> use ZP

Done.

http://codereview.appspot.com/1918047/diff/11001/12009
File src/pkg/image/geom.go (right):

http://codereview.appspot.com/1918047/diff/11001/12009#newcode11
src/pkg/image/geom.go:11: // A Point is an X, Y coordinate pair.
On 2010/08/09 23:32:56, r wrote:
> increases right and down

Done.

http://codereview.appspot.com/1918047/diff/11001/12009#newcode16
src/pkg/image/geom.go:16: // String returns a string representation of p.
On 2010/08/09 23:32:56, r wrote:
> comment could include example: "(3,4)"

Done.

http://codereview.appspot.com/1918047/diff/11001/12009#newcode75
src/pkg/image/geom.go:75: // Inset returns the rectangle r inset by n.
On 2010/08/09 23:32:56, r wrote:
> , which can be negative.

Done.

http://codereview.appspot.com/1918047/diff/11001/12009#newcode106
src/pkg/image/geom.go:106: }
On 2010/08/09 23:32:56, r wrote:
> you need a canonicalizer that reorders the coordinates to make the rectangle
> have bounds.
> if max.x < min.x, flip x; similarly for y

Done, although exp/draw's Rectangle has a Canon method that is not used
anywhere.

http://codereview.appspot.com/1918047/diff/11001/12010
File src/pkg/image/image.go (right):

http://codereview.appspot.com/1918047/diff/11001/12010#newcode16
src/pkg/image/image.go:16: // It is illegal to pass an (x, y) that is not in an
Image's Bounds.
On 2010/08/09 23:32:56, r wrote:
> what does illegal mean?
> 
> i'd prefer you define the image as zero outside the bounds.

I've changed the semantics in the comment, but I'll leave the implementation as
a TODO. I have a pending change to add a Rectangle to the concrete image types
and it'd be easier to implement the "zero, not illegal" semantics when that's
in.

http://codereview.appspot.com/1918047/diff/11001/12010#newcode32
src/pkg/image/image.go:32: return Rectangle{Point{0, 0}, Point{0, 0}}
On 2010/08/09 23:32:56, r wrote:
> ZR

Done (as are similar occurrences below).

http://codereview.appspot.com/1918047/diff/11001/12010#newcode34
src/pkg/image/image.go:34: return Rectangle{Point{0, 0}, Point{len(p.Pixel[0]),
len(p.Pixel)}}
On 2010/08/09 23:32:56, r wrote:
> ZP

Done (as are similar occurrences below).

[r, 2010-08-10 01:55:48]

LGTM

http://codereview.appspot.com/1918047/diff/21001/22009
File src/pkg/image/geom.go (right):

http://codereview.appspot.com/1918047/diff/21001/22009#newcode101
src/pkg/image/geom.go:101: // Min.X <= Max.X and Min.Y <= Max.Y.
The returned value has mininum and maximum coordinates swapped if necessart so
that...

[nigeltao, 2010-08-10 02:09:02]

*** Submitted as http://code.google.com/p/go/source/detail?r=7dcf7ae083c1 ***

image: replace Width and Height by Bounds, and introduce the Point and
Rect types.

The actual image representation is unchanged. A future change will
replace the {[][]color} with {[]color, stride int, r Rectangle} and
possibly a clip region.

The draw.Color, draw.Point and draw.Rect types will be removed in a
future change. Trying to do it in this one polluted the diff with
trivia.

R=r, rsc
CC=golang-dev
http://codereview.appspot.com/1918047

[rog, 2010-08-10 07:41:02]

http://codereview.appspot.com/1918047/diff/2001/3011
File src/pkg/image/names.go (right):

http://codereview.appspot.com/1918047/diff/2001/3011#newcode28
src/pkg/image/names.go:28: func (c ColorImage) Bounds() Rectangle { return
Rectangle{Point{0, 0}, Point{1e9, 1e9}} }
On 2010/08/09 12:09:34, nigeltao_golang wrote:
> Should the "0"s be "-1e9"s?

yes.

http://codereview.appspot.com/1918047/diff/11001/12009
File src/pkg/image/geom.go (right):

http://codereview.appspot.com/1918047/diff/11001/12009#newcode106
src/pkg/image/geom.go:106: }
On 2010/08/10 00:43:52, nigeltao_golang wrote:
> On 2010/08/09 23:32:56, r wrote:
> > you need a canonicalizer that reorders the coordinates to make the rectangle
> > have bounds.

if the points are canonicalised here, then the comment is not true,
because Rectangle{p0, p1} does not canonicalise.

it's not necessarily always the right thing to do either, because
sometimes it's useful to construct a non-canonical rectange, for
instance when writing test cases or finding bugs.

FWIW plan9's Rect constructor doesn't canonicalise.

[nigeltao_gnome, 2010-08-11 04:38:51]

On 10 August 2010 17:41,  <rogpeppe@gmail.com> wrote:
> http://codereview.appspot.com/1918047/diff/2001/3011
> File src/pkg/image/names.go (right):
>
> http://codereview.appspot.com/1918047/diff/2001/3011#newcode28
> src/pkg/image/names.go:28: func (c ColorImage) Bounds() Rectangle {
> return Rectangle{Point{0, 0}, Point{1e9, 1e9}} }
> On 2010/08/09 12:09:34, nigeltao_golang wrote:
>>
>> Should the "0"s be "-1e9"s?
>
> yes.

It's not a no-brainer. On the one hand, symmetry is nice. If I want a
circular mask image of radius 50, a Bounds of (-50,-50)-(50,50) might
be more elegant than a Bounds of (0,0)-(100,100). On the other hand,
whilst (-50,-50)-(50,50) would be a valid Rectangle, it might be
useful to restrict an image's bounds to be in the bottom right
quadrant.


> if the points are canonicalised here, then the comment is not true,
> because Rectangle{p0, p1} does not canonicalise.
>
> it's not necessarily always the right thing to do either, because
> sometimes it's useful to construct a non-canonical rectange, for
> instance when writing test cases or finding bugs.
>
> FWIW plan9's Rect constructor doesn't canonicalise.

I'm still thinking about what extra conditions (if any) need to be
placed on an image's bounds.

A Rectangle's definition is: "A Rectangle contains the points with
Min.X <= X < Max.X, Min.Y <= Y < Max.Y". It seems perfectly valid to
me to have a Rectangle that has Min.X >= Max.X, it just happens to be
an empty rectangle. However, an image's bounds Rectangle with negative
Dx is problematic when naively slicing the pixel buffer. The question
is whether dealing with negative Dx should be the responsibility of
code that writes image bounds or code that reads them.

[r, 2010-08-11 04:56:51]

On Wed, Aug 11, 2010 at 2:38 PM, Nigel Tao <nigel.tao.gnome@gmail.com> wrote:
> On 10 August 2010 17:41,  <rogpeppe@gmail.com> wrote:
>> http://codereview.appspot.com/1918047/diff/2001/3011
>> File src/pkg/image/names.go (right):
>>
>> http://codereview.appspot.com/1918047/diff/2001/3011#newcode28
>> src/pkg/image/names.go:28: func (c ColorImage) Bounds() Rectangle {
>> return Rectangle{Point{0, 0}, Point{1e9, 1e9}} }
>> On 2010/08/09 12:09:34, nigeltao_golang wrote:
>>>
>>> Should the "0"s be "-1e9"s?
>>
>> yes.
>
> It's not a no-brainer. On the one hand, symmetry is nice. If I want a
> circular mask image of radius 50, a Bounds of (-50,-50)-(50,50) might
> be more elegant than a Bounds of (0,0)-(100,100). On the other hand,
> whilst (-50,-50)-(50,50) would be a valid Rectangle, it might be
> useful to restrict an image's bounds to be in the bottom right
> quadrant.

that would prevent a window being moved left or up off the screen.

>
>> if the points are canonicalised here, then the comment is not true,
>> because Rectangle{p0, p1} does not canonicalise.
>>
>> it's not necessarily always the right thing to do either, because
>> sometimes it's useful to construct a non-canonical rectange, for
>> instance when writing test cases or finding bugs.
>>
>> FWIW plan9's Rect constructor doesn't canonicalise.
>
> I'm still thinking about what extra conditions (if any) need to be
> placed on an image's bounds.
>
> A Rectangle's definition is: "A Rectangle contains the points with
> Min.X <= X < Max.X, Min.Y <= Y < Max.Y". It seems perfectly valid to
> me to have a Rectangle that has Min.X >= Max.X, it just happens to be
> an empty rectangle. However, an image's bounds Rectangle with negative
> Dx is problematic when naively slicing the pixel buffer. The question
> is whether dealing with negative Dx should be the responsibility of
> code that writes image bounds or code that reads them.
>

[rsc, 2010-08-11 06:09:31]

You really don't want -50 to mean Dx-50 (or Dy-50).
I know it's the X convention, but, like when using uint math,
it inserts a giant discontinuity in the middle of a range
of useful arithmetic values, so that p+(-1,-1) is not the
point up and to the left one pixel for all typical p.
Like the loop for i = n; i >= 0; i-- with i uint, it's going
to be a source of bugs more often than it will be
convenient.  (Or, for another analogy, consider the
fact that given var x []int, Go does not define x[-1]
to refer to the last element of x.)

Russ

[nigeltao_gnome, 2010-08-11 08:33:28]

On 11 August 2010 16:09, Russ Cox <rsc@golang.org> wrote:
> You really don't want -50 to mean Dx-50 (or Dy-50).

Uh, that wasn't what I meant. I just meant that calling img.At(-5, -5)
could be a valid thing to do. Not that it meant the pixel at (width-5,
height-5).

[nigeltao_gnome, 2010-08-11 12:18:32]

On 11 August 2010 14:56, Rob 'Commander' Pike <r@golang.org> wrote:
> that would prevent a window being moved left or up off the screen.

I'll start a new golang-dev thread.