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 (*Uniform).SubImage #71463

Closed
hajimehoshi opened this issue Jan 28, 2025 · 8 comments
Closed

proposal: image: add (*Uniform).SubImage #71463

hajimehoshi opened this issue Jan 28, 2025 · 8 comments
Labels
LibraryProposal Issues describing a requested change to the Go standard library or x/ libraries, but not to a tool Proposal
Milestone

Comments

@hajimehoshi
Copy link
Member

hajimehoshi commented Jan 28, 2025

Proposal Details

I propose to add SubImage method to image.Uniform, like other image types. SubImage takes a rectangle as a bound and returns an image.Image value. The underlying value of the returned value is a Uniform image with bounds, so we would need to add a (hidden) new member representing bounds to Uniform.

This is useful when I want to create a color-filled image and encode this as a PNG image for example.

@gopherbot gopherbot added this to the Proposal milestone Jan 28, 2025
@gabyhelp
Copy link

Related Issues

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@gabyhelp gabyhelp added the LibraryProposal Issues describing a requested change to the Go standard library or x/ libraries, but not to a tool label Jan 28, 2025
@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Jan 28, 2025
@ianlancetaylor
Copy link
Member

CC @nigeltao

@hajimehoshi
Copy link
Member Author

ping?

/CC @dmitshur

@ianlancetaylor
Copy link
Member

This is on the list for the proposal review committee to take a look.

It would help if you could write out the exact API that would be added to the image package: the new method and its doc comment. Thanks.

@hajimehoshi
Copy link
Member Author

// SubImage returns an image representing the portion of the image p visible through r. The returned value shares pixels with the original image.
func (p *Uniform) SubImage(r Rectangle) Image

The implementation will be like this

type Uniform struct {
	C color.Color
	bounds image.Rectangle
	useBounds bool
}

func (p *Uniform) Bounds() image.Rectangle {
	if p.useBounds {
		return p.bounds
	}
	return Rectangle{Point{-1e9, -1e9}, Point{1e9, 1e9}}
}

func (p *Uniform) SubImage(r Rectangle) Image {
	return &Uniform{
		C: p.C,
		bounds: p.Bounds().Inetrsect(r),
		useBounds: true,
	}
}

@nigeltao
Copy link
Contributor

nigeltao commented Mar 3, 2025

The image/draw package (not the image package) currently assumes that animage.Uniform is always essentially unbounded. In hindsight, maybe we should have done differently, but that discussion needed to have happened 15 years ago.

If we changed the API like you suggested then that assumption breaks. We would have to update the image/draw code, not just the image code.

But we'd also have to update the golang.org/x/image/draw code, as it also has fast-paths for image.Uniform images and makes the same assumption.

But, if we want to avoid breaking anyone (along the lines of Go 1.x compat promise), in terms of semantics and not just "it still compiles", we'd also have to update any other third party code that have forked image/draw or x/image/draw or done a similar type assertion. This doesn't seem feasible.


Per the other conversation:

I suppose that it's possible to add a image.SizedUniform type, and update image/draw and x/image/draw to look for it.

But I'm not convinced yet that we need an image.SizedUniform type in the standard library. If you really want one, you don't need to change the standard library to have one (that you can pass to image/png):

package somethingnotinthestdlib

func createColorFilledImage0(r image.Rectangle, c color.RGBA) image.Image {
    return &sizedUniform{r, c, color.RGBAModel}
}

type sizedUniform struct {
    r image.Rectangle
    c color.Color
    m color.Model
}

func (s *sizedUniform) At(x, y int) color.Color {
    if (image.Point{x, y}).In(s.r) {
        return s.c
    }
    return color.Black
}

func (s *sizedUniform) Bounds() image.Rectangle { return s.r }
func (s *sizedUniform) ColorModel() color.Model { return s.m }

This is useful when I want to create a color-filled image and encode this as a PNG image for example.

But really, I'd just do this (again, with no stdlib changes needed):

func createColorFilledImage1(r image.Rectangle, c color.RGBA) image.Image {
    m := image.NewRGBA(r)
    for y := r.Min.Y; y < r.Max.Y; y++ {
        for x := r.Min.X; x < r.Max.X; x++ {
            m.SetRGBA(x, y, c)
        }
    }
    return m
}   

@nigeltao
Copy link
Contributor

nigeltao commented Mar 3, 2025

It should be obvious from my previous comment that I'm not in favor of this proposal. But for the record, see also that image.Uniform is listed as "These image and image/color struct types are frozen. We will never add fields to them.".

@hajimehoshi
Copy link
Member Author

Hmm, ok, thank you for elaborating!

@dmitshur dmitshur closed this as not planned Won't fix, can't repro, duplicate, stale Mar 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LibraryProposal Issues describing a requested change to the Go standard library or x/ libraries, but not to a tool Proposal
Projects
None yet
Development

No branches or pull requests

6 participants