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

proposal: image: add (Rectangle).PointsBy for iterating over points as iter.Seq[Point] #69254

Open
tomocy opened this issue Sep 4, 2024 · 3 comments
Labels
Milestone

Comments

@tomocy
Copy link
Contributor

tomocy commented Sep 4, 2024

Proposal Details

Summary

I propose adding a new method PointsBy to image.Rectangle
that returns an iter.Seq[image.Point] for points within the rectangle.
This function would allow developers to directly iterate over points in a rectangle,
making the code more straightforward and reducing potential errors.

The PointsBy can be implemented as follows.

func (r Rectangle) PointsBy(delta Point) iter.Seq[Point] {
    return func(yield func(Point) bool) {
        for y := r.Min.Y; y < r.Max.Y; y += delta.Y {
            for x := r.Min.X; x < r.Max.X; x += delta.X {
                if !yield(image.Pt(x, y)) {
                    return
                }
            }
        }
    }
}

By this function, the common pattern with a image.Rectangle r

for y := r.Min.Y; y < r.Max.Y; y++ {
    for x := r.Min.X; x < r.Max.X; x++ {
        p := image.Pt(x, y)
        // Do something with p.
    }
}

could be simplified to

for p := range r.PointsBy(image.Pt(1, 1)) {
    // Do something with p.
}

Background

  • Iterating over points within a rectangle is a common operation in image processing tasks.

  • This operation requires manually writing nested loops every time.
    While not complex, it can indeed be cumbersome and repetitive.

  • Since image.Rectangle does not necessarily start at (0, 0), it's crucial to loop from Min to Max, as documented.
    However, there are cases where developers mistakenly start from 0.

Given this background, it may be beneficial to provide a dedicated API from the image package
to better address this demand for iterating over points.

Benefits

  • Provides a direct API for iterating over image.Points not over Xs and Ys within an image.Rectangle.

  • Eliminates the need for additional labels when breaking out of nested loops under specific conditions, such as

pointsLoop:
    for y := r.Min.Y; y < r.Max.Y; y++ {
        for x := r.Min.X; x < r.Max.X; x++ {
            // if (x, y) meets a certain condition
            break pointsLoop
        }
    }

    // then
  • Supports non-unit steps (e.g., y += 2, x += 3) by taking a delta parameter, keeping flexibility.

  • Aligns well with the iter package, enhancing integration with the iterator-based approach.

Considerations

Beyond this proposal, a few additional considerations might be relevant.

  • Whether to use image.Point to express delta (as image.Rectangle.Size does with func() image.Point) or x, y int (as image.Image.At does with func(x, y int) color.Color).

  • The iter package was added recently, and its adoption and consensus within the community might still be evolving.

@tomocy tomocy added the Proposal label Sep 4, 2024
@gopherbot gopherbot added this to the Proposal milestone Sep 4, 2024
@ianlancetaylor
Copy link
Contributor

CC @nigeltao

@nigeltao
Copy link
Contributor

nigeltao commented Sep 8, 2024

Supports non-unit steps (e.g., y += 2, x += 3) by taking a delta parameter, keeping flexibility.

What's the motivation for a delta that isn't (1, 1)? We already have flexibility in that the programmer can always write the for loops. Should the proposal instead be for:

for p := range r.Points() {
    // Do something with p.
}

Other than that, I'm OK with this proposal, although I haven't been following the new range-over-function-types language feature very closely. I assume that looping over r.PointsBy is basically as efficient as the explicit dual-loop we used to write. I'm also not sure how much the image package should be an 'early adopter', as we can't change (i.e.) fix any API with the hindsight of more iterator experience. I'll defer to @ianlancetaylor on the broader philosophy there.

@tomocy
Copy link
Contributor Author

tomocy commented Sep 9, 2024

Thanks.

What's the motivation for a delta that isn't (1, 1)?

Regarding the delta, my initial thought was that it could support use cases like those in image/jpeg,
or tiling, with the bonus of reducing loop counts.

go/src/image/jpeg/writer.go

Lines 520 to 522 in 5858205

for y := bounds.Min.Y; y < bounds.Max.Y; y += 8 {
for x := bounds.Min.X; x < bounds.Max.X; x += 8 {
p := image.Pt(x, y)

However, as you pointed out, it indeed makes more sense to let that kind of flexibility be handled by explicit for loop or iterator chain, while Points serves as a straightforward entry point for an iterator-based approach, alongside the proposed iteration itself.
I’d be happy to adjust the proposal accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Incoming
Development

No branches or pull requests

4 participants