|
|
|
graphics/convolve: expose ConvolvePoint
Patch Set 1 #Patch Set 2 : diff -r fccf2bd86494 https://crawshaw%40google.com@code.google.com/p/graphics-go/ #Patch Set 3 : diff -r fccf2bd86494 https://crawshaw%40google.com@code.google.com/p/graphics-go/ #
Total comments: 2
Patch Set 4 : diff -r fccf2bd86494 https://crawshaw%40google.com@code.google.com/p/graphics-go/ #
Total comments: 12
Patch Set 5 : diff -r 1c2e9ddd42b8 https://crawshaw%40google.com@code.google.com/p/graphics-go/ #
Total comments: 1
Total messages: 6
|
crawshaw1
Hello nigeltao@golang.org (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://crawshaw%40google.com@code.google.com/p/graphics-go/
|
14 years ago (2012年01月11日 03:27:51 UTC) #1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Hello nigeltao@golang.org (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://crawshaw%40google.com@code.google.com/p/graphics-go/
API looks reasonable. I feel like there exists a nice design somewhere that unifies all these image transformations, but I'm happy to get there by evolutionary steps. http://codereview.appspot.com/5528071/diff/4001/graphics/convolve/Makefile File graphics/convolve/Makefile (right): http://codereview.appspot.com/5528071/diff/4001/graphics/convolve/Makefile#ne... graphics/convolve/Makefile:10: separable.go\ Forget to "hg add separable.go" (and possibly separable_test.go)?
PTAL http://codereview.appspot.com/5528071/diff/4001/graphics/convolve/Makefile File graphics/convolve/Makefile (right): http://codereview.appspot.com/5528071/diff/4001/graphics/convolve/Makefile#ne... graphics/convolve/Makefile:10: separable.go\ On 2012年01月11日 05:47:28, nigeltao wrote: > Forget to "hg add separable.go" (and possibly separable_test.go)? Done.
http://codereview.appspot.com/5528071/diff/3/graphics/convolve/convolve_test.go File graphics/convolve/convolve_test.go (right): http://codereview.appspot.com/5528071/diff/3/graphics/convolve/convolve_test.... graphics/convolve/convolve_test.go:26: desc: "identity", k doesn't look like an identity matrix to me. http://codereview.appspot.com/5528071/diff/3/graphics/convolve/convolve_test.... graphics/convolve/convolve_test.go:145: []float64{0, 1, 0}) Add a ",\n" before the ")". http://codereview.appspot.com/5528071/diff/3/graphics/convolve/separable.go File graphics/convolve/separable.go (right): http://codereview.appspot.com/5528071/diff/3/graphics/convolve/separable.go#n... graphics/convolve/separable.go:15: // kernel. x and x are the per-axis weights. Each slice must be the same s/x and x/x and y/ http://codereview.appspot.com/5528071/diff/3/graphics/convolve/separable.go#n... graphics/convolve/separable.go:21: // []float64{1, 2, 1}) Add a ",\n" before the ")". http://codereview.appspot.com/5528071/diff/3/graphics/convolve/separable.go#n... graphics/convolve/separable.go:183: buf := k.buildBuf(bounds, src) So, if the src is 1000x1000, you'll build a 1000x1000 buffer even if the kernel is 3x3? http://codereview.appspot.com/5528071/diff/3/graphics/convolve/separable_test.go File graphics/convolve/separable_test.go (right): http://codereview.appspot.com/5528071/diff/3/graphics/convolve/separable_test... graphics/convolve/separable_test.go:19: []float64{0, 1, 0}) Add a ",\n" before the ")".
http://codereview.appspot.com/5528071/diff/3/graphics/convolve/convolve_test.go File graphics/convolve/convolve_test.go (right): http://codereview.appspot.com/5528071/diff/3/graphics/convolve/convolve_test.... graphics/convolve/convolve_test.go:26: desc: "identity", On 2012年01月16日 05:18:10, nigeltao wrote: > k doesn't look like an identity matrix to me. Done. http://codereview.appspot.com/5528071/diff/3/graphics/convolve/convolve_test.... graphics/convolve/convolve_test.go:145: []float64{0, 1, 0}) On 2012年01月16日 05:18:10, nigeltao wrote: > Add a ",\n" before the ")". Done. http://codereview.appspot.com/5528071/diff/3/graphics/convolve/separable.go File graphics/convolve/separable.go (right): http://codereview.appspot.com/5528071/diff/3/graphics/convolve/separable.go#n... graphics/convolve/separable.go:15: // kernel. x and x are the per-axis weights. Each slice must be the same On 2012年01月16日 05:18:10, nigeltao wrote: > s/x and x/x and y/ Done. http://codereview.appspot.com/5528071/diff/3/graphics/convolve/separable.go#n... graphics/convolve/separable.go:21: // []float64{1, 2, 1}) On 2012年01月16日 05:18:10, nigeltao wrote: > Add a ",\n" before the ")". Done. http://codereview.appspot.com/5528071/diff/3/graphics/convolve/separable.go#n... graphics/convolve/separable.go:183: buf := k.buildBuf(bounds, src) On 2012年01月16日 05:18:10, nigeltao wrote: > So, if the src is 1000x1000, you'll build a 1000x1000 buffer even if the kernel > is 3x3? Yes, ConvolvePoint on a separable kernel is not a good idea. I could implement this as a traditional convolution (i.e. build the full matrix). Another possibility is designing it into the API: interface Kernel { With(image.Image) Convolve } interface Convolve { Convolve(dst draw.Image) ConvolvePoint(x, y int) color.Color) } Not sure if it pulls its weight. http://codereview.appspot.com/5528071/diff/3/graphics/convolve/separable_test.go File graphics/convolve/separable_test.go (right): http://codereview.appspot.com/5528071/diff/3/graphics/convolve/separable_test... graphics/convolve/separable_test.go:19: []float64{0, 1, 0}) On 2012年01月16日 05:18:10, nigeltao wrote: > Add a ",\n" before the ")". Done.
LGTM. http://codereview.appspot.com/5528071/diff/12001/graphics/convolve/convolve.go File graphics/convolve/convolve.go (right): http://codereview.appspot.com/5528071/diff/12001/graphics/convolve/convolve.g... graphics/convolve/convolve.go:38: RGBA(dst, src *image.RGBA) I think that the methods should be called ConvolveRGBA and ConvolvePointRGBA. The type should probably also be called KernelRGBA.