graphicscommand: Explicitly forbide ReplacePixels for a part after DrawImage

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
+	image       graphicsdriver.Image
-	width  int
+	width       int
-	height 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 {

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)
+}

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()
+		sw, sh := dummyImage.Size()
-		w2 := graphics.NextPowerOf2Int(w)
+		dw := graphics.NextPowerOf2Int(w)
-		h2 := graphics.NextPowerOf2Int(h)
+		dh := graphics.NextPowerOf2Int(h)
-		vs := graphics.QuadVertices(w2, h2, 0, 0, dw, dh,
+		vs := graphics.QuadVertices(dw, dh, 0, 0, sw, sh,
-			float32(width)/float32(dw), 0, 0, float32(height)/float32(dh),
+			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()
+		panic("restorable: ReplacePixels for a part after DrawImage is forbidden")
-		return
 	}
 
 	if i.stale {

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[1] = 0
-	base.Pix[2] = 0xff
+	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 {

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()