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 bound to Uniform image. #22380

Closed
kybin opened this issue Oct 22, 2017 · 9 comments
Closed

proposal: image: add bound to Uniform image. #22380

kybin opened this issue Oct 22, 2017 · 9 comments

Comments

@kybin
Copy link
Contributor

@kybin kybin commented Oct 22, 2017

Currently image.Uniform is unbounded (infinite-sized) Image. But all other Images in the package are bounded. This unmatched property is cumbersome to me.

Because an unbounded image is good for filling all area, but bad for filling specific area.

For example, I usually code like this, but with Uniform, I can't.

draw.Draw(dst, src.Bounds(), src, src.Bounds().Min, draw.Src)

I once created my own Uniform image that has size. But draw.Draw got slower. (draw.Draw knows how to optimize image.Uniform but not MyUniform)

So I propose add bound to Uniform and new function like NewUniformBounded or NewUniformSized.

The original NewUniform will still there, so we could generate both bounded and unbounded images easily.

@gopherbot gopherbot added this to the Unreleased milestone Oct 22, 2017
@kybin kybin changed the title x/image: add bound to Uniform image. proposal: image: add bound to Uniform image. Oct 22, 2017
@artyom
Copy link
Member

@artyom artyom commented Oct 22, 2017

@kybin have you considered calling Draw like this:

r := dst.Bounds() // or some custom region to fill
draw.Draw(dst, r, image.NewUniform(color), image.ZP, draw.Src)

Loading

@kybin
Copy link
Contributor Author

@kybin kybin commented Oct 22, 2017

Hi, @artyom .

Thank you for your opinion.

But what I mean is Uniform image could not save the region info, while other Image types could. That is the image's bounds.

Consider we have a Image consists of splitted Images.

oneBigImage // image.Image
splittedImages // []image.Image

for _, img := range splittedImages {
    draw.Draw(oneBigImage, img.Bounds(), img, img.Bounds().Min, draw.Src)
    // I can't draw Uniform image this way.
}

While the unbounded Uniform image is convenient, but also a restriction for some cases I think.

Hope it makes sense to you.

Loading

@artyom
Copy link
Member

@artyom artyom commented Oct 22, 2017

@kybin thank you, I see your reasoning. Please consider that currently image.Uniform is documented as "[...] an infinite-sized Image of uniform color" — and there may exist code relying on this documented property which you suggest to change.

Maybe you can do something like this with your example — IMO this is rather low-overhead solution:

oneBigImage // image.Image
splittedImages := []struct{
	img  image.Image
	rect image.Rectangle
}{
	{img1, img1.Bounds()},
	{img2, img2.Bounds()},
	...
	{imgUniform, image.Rect(x0, y0, x1, y1)},
}

for _, x := range splittedImages {
    draw.Draw(oneBigImage, x.rect, x.img, x.rect.Min, draw.Src)
}

Loading

@bronze1man
Copy link
Contributor

@bronze1man bronze1man commented Oct 23, 2017

@kybin as a work around, you can write your own version of draw.Draw to do optimize with your type.

image.Uniform looks useless for me too.
It may be better to add a image.UniformSize , put color and size into it. and draw.Draw can optimize that.

Loading

@as
Copy link
Contributor

@as as commented Oct 23, 2017

Instead of a new type, I would add the SubImage method to image.Uniform. But thats really putting the solution before the problem. What is a practical use case for having a sized Uniform?

Loading

@bronze1man
Copy link
Contributor

@bronze1man bronze1man commented Oct 23, 2017

What is a practical use case for having a sized Uniform?

Use the image Bound size as ui image element size, and put an all white image as the some ui panel background with the windows gdi api.
I can workaround this problem by just provide a size from the caller. But it make my function interface complex.

Loading

@kybin
Copy link
Contributor Author

@kybin kybin commented Oct 23, 2017

@artyom I agree, it could break existing codes.

Then as @bronze1man suggested, add image.UniformSize could be an alternative.

@nigeltao What's your opinion about this?

Loading

@nigeltao
Copy link
Contributor

@nigeltao nigeltao commented Oct 27, 2017

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

Still, it looks a little weird to use the source bounds for the draw image rectangle. Yes, it would obviously work here, but conceptually, the dst and src coordinate spaces are potentially different spaces, and while you'd get away with it here, they aren't always interchangeable.

Also, I'm drifting off-topic, but in hindsight, the Go image bounds model, where the bounds is a rectangle (left/top/right/bottom or 4 ints) instead of a size (width/height or 2 ints) might have been a mistake. It made sense in the original inspiration - the Plan 9 drawing model - when it was used for clipping windows on screen, so there's a clear concept of an origin (0, 0) which can be outside an image's or window's bounds. But for Go in general, and not for Plan 9 specifically, programmers are frequently confused when img.At(0, 0) isn't necessarily the top-left pixel of an image. This confusion is usually apparent when they iterate "for x := 0; etc" instead of "for x := bounds.Min.X; etc". If I were doing it all again, or perhaps for a very hypothetical image 2.0 library, I would consider making bounds 2 ints instead of 4. In such a model, having a Uniform versus SizedUniform wouldn't help your underlying problem here.

Getting back to the original topic, I'd do what @artyom suggested, namely:

splittedImages := []struct{
	img  image.Image
	rect image.Rectangle
}{ etc }

which would work regardless of my off-topic philosophizing...

As for drawing various image.Image values on top of an all-white background, where are those image.Image values coming from? If they're e.g. JPEG or PNG images loaded from disk, then the loading process won't set the image bounds' rectangle to the position you want. In that case, you already have to supply an image.Rectangle out-of-band, so I'm guessing it would be straightforward to pass on an explicit (image.Image and image.Rectangle) instead of just an image.Image (with implicit rectangle).

If you're talking about GUIs, I think that the golang.org/x/exp/shiny code has some example that happily uses image.Uniform without seeming to need an image.SizedUniform.

Loading

@kybin
Copy link
Contributor Author

@kybin kybin commented Oct 28, 2017

@nigeltao Thank you for your detailed explanation.

Yes I am working on GUIs, so I will look into shiny code more closely.

Reason for this proposal is mainly about inconsistency with image.Images.
All others have bound, and Uniform not has it.
Sometimes it does not helps.
(I think it will same in your imaginary image 2.0 package)

But for now I need time to re-thinking about this problem, so I will close it.
Thank you all :)

Loading

@kybin kybin closed this Oct 28, 2017
@golang golang locked and limited conversation to collaborators Oct 28, 2018
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
6 participants