From b34834a89526a4766cfe89eeca403a5cc9f70784 Mon Sep 17 00:00:00 2001 From: Hajime Hoshi Date: Sun, 6 Jan 2019 03:15:32 +0900 Subject: [PATCH] graphicscommand: Explicitly forbide ReplacePixels for a part after DrawImage --- internal/graphicscommand/image.go | 37 +++++++++++++++++++-- internal/graphicscommand/image_test.go | 17 ++++++++++ internal/restorable/image.go | 13 ++++---- internal/restorable/images_test.go | 46 ++------------------------ internal/shareable/shareable.go | 2 -- 5 files changed, 60 insertions(+), 55 deletions(-) diff --git a/internal/graphicscommand/image.go b/internal/graphicscommand/image.go index 0818388ec..1fff4646d 100644 --- a/internal/graphicscommand/image.go +++ b/internal/graphicscommand/image.go @@ -20,11 +20,22 @@ import ( "github.com/hajimehoshi/ebiten/internal/graphicsdriver" ) +type lastCommand int + +const ( + lastCommandNone lastCommand = iota + lastCommandClear + lastCommandDrawImage + lastCommandReplacePixels +) + // Image represents an image that is implemented with OpenGL. type Image struct { - image graphicsdriver.Image - width int - height int + image graphicsdriver.Image + width int + height int + screen bool + lastCommand lastCommand } // NewImage returns a new image. @@ -48,6 +59,7 @@ func NewScreenFramebufferImage(width, height int) *Image { i := &Image{ width: width, height: height, + screen: true, } c := &newScreenFramebufferImageCommand{ result: i, @@ -71,7 +83,19 @@ func (i *Image) Size() (int, int) { } func (i *Image) DrawImage(src *Image, vertices []float32, indices []uint16, clr *affine.ColorM, mode graphics.CompositeMode, filter graphics.Filter, address graphics.Address) { + if i.lastCommand == lastCommandNone { + if !i.screen && mode != graphics.CompositeModeClear { + panic("graphicscommand: the image must be cleared first") + } + } + theCommandQueue.EnqueueDrawImageCommand(i, src, vertices, indices, clr, mode, filter, address) + + if i.lastCommand == lastCommandNone && !i.screen { + i.lastCommand = lastCommandClear + } else { + i.lastCommand = lastCommandDrawImage + } } // Pixels returns the image's pixels. @@ -87,6 +111,12 @@ func (i *Image) Pixels() []byte { } func (i *Image) ReplacePixels(p []byte, x, y, width, height int) { + // ReplacePixels for a part might invalidate the current image that are drawn by DrawImage (#593, #738). + if i.lastCommand == lastCommandDrawImage { + if x != 0 || y != 0 || i.width != width || i.height != height { + panic("graphicscommand: ReplacePixels for a part after DrawImage is forbidden") + } + } pixels := make([]byte, len(p)) copy(pixels, p) c := &replacePixelsCommand{ @@ -98,6 +128,7 @@ func (i *Image) ReplacePixels(p []byte, x, y, width, height int) { height: height, } theCommandQueue.Enqueue(c) + i.lastCommand = lastCommandReplacePixels } func (i *Image) IsInvalidated() bool { diff --git a/internal/graphicscommand/image_test.go b/internal/graphicscommand/image_test.go index 2566687fb..01518af15 100644 --- a/internal/graphicscommand/image_test.go +++ b/internal/graphicscommand/image_test.go @@ -63,3 +63,20 @@ func TestClear(t *testing.T) { } } } + +func TestReplacePixelsPartAfterDrawImage(t *testing.T) { + defer func() { + if r := recover(); r == nil { + t.Errorf("ReplacePixels must panic but not") + } + }() + const w, h = 32, 32 + clr := NewImage(w, h) + src := NewImage(16, 16) + dst := NewImage(w, h) + vs := graphics.QuadVertices(16, 16, 0, 0, w, h, 1, 0, 0, 1, 0, 0, 1, 1, 1, 1) + is := graphics.QuadIndices() + dst.DrawImage(clr, vs, is, nil, graphics.CompositeModeClear, graphics.FilterNearest, graphics.AddressClampToZero) + dst.DrawImage(src, vs, is, nil, graphics.CompositeModeSourceOver, graphics.FilterNearest, graphics.AddressClampToZero) + dst.ReplacePixels(make([]byte, 4), 0, 0, 1, 1) +} diff --git a/internal/restorable/image.go b/internal/restorable/image.go index f673f3c29..bab63cceb 100644 --- a/internal/restorable/image.go +++ b/internal/restorable/image.go @@ -165,11 +165,11 @@ func (i *Image) ReplacePixels(pixels []byte, x, y, width, height int) { // and this image can be restored without dummyImage. // // dummyImage should be restored later anyway. - dw, dh := dummyImage.Size() - w2 := graphics.NextPowerOf2Int(w) - h2 := graphics.NextPowerOf2Int(h) - vs := graphics.QuadVertices(w2, h2, 0, 0, dw, dh, - float32(width)/float32(dw), 0, 0, float32(height)/float32(dh), + sw, sh := dummyImage.Size() + dw := graphics.NextPowerOf2Int(w) + dh := graphics.NextPowerOf2Int(h) + vs := graphics.QuadVertices(dw, dh, 0, 0, sw, sh, + float32(width)/float32(sw), 0, 0, float32(height)/float32(sh), float32(x), float32(y), 1, 1, 1, 1) is := graphics.QuadIndices() @@ -193,8 +193,7 @@ func (i *Image) ReplacePixels(pixels []byte, x, y, width, height int) { } if len(i.drawImageHistory) > 0 { - i.makeStale() - return + panic("restorable: ReplacePixels for a part after DrawImage is forbidden") } if i.stale { diff --git a/internal/restorable/images_test.go b/internal/restorable/images_test.go index 03fe88433..985c1a04b 100644 --- a/internal/restorable/images_test.go +++ b/internal/restorable/images_test.go @@ -483,8 +483,8 @@ func TestReplacePixels(t *testing.T) { func TestDrawImageAndReplacePixels(t *testing.T) { base := image.NewRGBA(image.Rect(0, 0, 1, 1)) base.Pix[0] = 0xff - base.Pix[1] = 0xff - base.Pix[2] = 0xff + base.Pix[1] = 0 + base.Pix[2] = 0 base.Pix[3] = 0xff img0 := newImageFromImage(base) defer img0.Dispose() @@ -494,7 +494,7 @@ func TestDrawImageAndReplacePixels(t *testing.T) { vs := graphics.QuadVertices(1, 1, 0, 0, 1, 1, 1, 0, 0, 1, 0, 0, 1, 1, 1, 1) is := graphics.QuadIndices() img1.DrawImage(img0, vs, is, nil, graphics.CompositeModeCopy, graphics.FilterNearest, graphics.AddressClampToZero) - img1.ReplacePixels([]byte{0xff, 0xff, 0xff, 0xff}, 1, 0, 1, 1) + img1.ReplacePixels([]byte{0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff}, 0, 0, 2, 1) ResolveStaleImages() if err := Restore(); err != nil { @@ -540,46 +540,6 @@ func TestDispose(t *testing.T) { } } -func TestDoubleResolve(t *testing.T) { - base0 := image.NewRGBA(image.Rect(0, 0, 2, 2)) - img0 := newImageFromImage(base0) - - base := image.NewRGBA(image.Rect(0, 0, 1, 1)) - base.Pix[0] = 0xff - base.Pix[1] = 0x00 - base.Pix[2] = 0x00 - base.Pix[3] = 0xff - img1 := newImageFromImage(base) - - vs := graphics.QuadVertices(1, 1, 0, 0, 1, 1, 1, 0, 0, 1, 0, 0, 1, 1, 1, 1) - is := graphics.QuadIndices() - img0.DrawImage(img1, vs, is, nil, graphics.CompositeModeCopy, graphics.FilterNearest, graphics.AddressClampToZero) - img0.ReplacePixels([]uint8{0x00, 0xff, 0x00, 0xff}, 1, 1, 1, 1) - // Now img0 is stale. - ResolveStaleImages() - - img0.ReplacePixels([]uint8{0x00, 0x00, 0xff, 0xff}, 1, 0, 1, 1) - ResolveStaleImages() - - if err := Restore(); err != nil { - t.Fatal(err) - } - - wantImg := image.NewRGBA(image.Rect(0, 0, 2, 2)) - wantImg.Set(0, 0, color.RGBA{0xff, 0x00, 0x00, 0xff}) - wantImg.Set(1, 0, color.RGBA{0x00, 0x00, 0xff, 0xff}) - wantImg.Set(1, 1, color.RGBA{0x00, 0xff, 0x00, 0xff}) - for j := 0; j < 2; j++ { - for i := 0; i < 2; i++ { - got := img0.At(i, j) - want := wantImg.At(i, j).(color.RGBA) - if !sameColors(got, want, 1) { - t.Errorf("got: %v, want: %v", got, want) - } - } - } -} - func TestClear(t *testing.T) { pix := make([]uint8, 4*4*4) for i := range pix { diff --git a/internal/shareable/shareable.go b/internal/shareable/shareable.go index 23fa2f4f1..c4b4ff73d 100644 --- a/internal/shareable/shareable.go +++ b/internal/shareable/shareable.go @@ -67,8 +67,6 @@ func (b *backend) TryAlloc(width, height int) (*packing.Node, bool) { w, h := oldImg.Size() // Do not use DrawImage here. ReplacePixels will be called on a part of newImg later, and it looked like // ReplacePixels on a part of image deletes other region that are rendered by DrawImage (#593, #758). - // TODO: Add validations to ensure that an image cannot accept DrawImage after ReplacePixels on a part of - // it. // // Pixels() returns immediately as long as only oldImg.ReplacePixels is called. pix := oldImg.Pixels()