restorable: Bug fix: topological sort is required to restore images correctly (#357)

internal/restorable/image.go

@@ -192,11 +192,8 @@ func (p *Image) makeStaleIfDependingOn(target *Image) {
 	if p.stale {
 		return
 	}
-	for _, c := range p.drawImageHistory {
+	if p.dependsOn(target) {
-		if c.image == target {
+		p.makeStale()
-			p.makeStale()
-			return
-		}
 	}
 }
 
@@ -222,6 +219,15 @@ func (p *Image) resolveStalePixels() error {
 	return p.readPixelsFromGPU(p.image)
 }
 
+func (p *Image) dependsOn(target *Image) bool {
+	for _, c := range p.drawImageHistory {
+		if c.image == target {
+			return true
+		}
+	}
+	return false
+}
+
 func (p *Image) hasDependency() bool {
 	if p.stale {
 		return false

internal/restorable/images.go

@@ -90,22 +90,40 @@ func (i *images) restore() error {
 	defer i.m.Unlock()
 	// Framebuffers/textures cannot be disposed since framebuffers/textures that
 	// don't belong to the current context.
-	imagesWithoutDependency := []*Image{}
+
-	imagesWithDependency := []*Image{}
+	// Let's do topological sort based on dependencies of drawing history.
+	// There should not be a loop since cyclic drawing makes images stale.
+	current := map[*Image]struct{}{}
+	toBeDetermined := map[*Image]struct{}{}
+	sorted := []*Image{}
 	for img := range i.images {
 		if img.hasDependency() {
-			imagesWithDependency = append(imagesWithDependency, img)
+			toBeDetermined[img] = struct{}{}
-		} else {
+			continue
-			imagesWithoutDependency = append(imagesWithoutDependency, img)
 		}
+		current[img] = struct{}{}
+		sorted = append(sorted, img)
 	}
-	// Images depending on other images should be processed first.
+	// TODO: How to confirm that there is no loop?
-	for _, img := range imagesWithoutDependency {
+	for len(current) > 0 {
-		if err := img.restore(); err != nil {
+		next := map[*Image]struct{}{}
-			return err
+		for source := range current {
+			for target := range toBeDetermined {
+				if target.dependsOn(source) {
+					next[target] = struct{}{}
+				}
+			}
 		}
+		for img := range next {
+			sorted = append(sorted, img)
+			delete(toBeDetermined, img)
+		}
+		current = next
 	}
-	for _, img := range imagesWithDependency {
+	if len(toBeDetermined) > 0 {
+		panic("not reached")
+	}
+	for _, img := range sorted {
 		if err := img.restore(); err != nil {
 			return err
 		}

internal/restorable/images_test.go

@@ -71,6 +71,37 @@ func vertices() []float32 {
 	}
 }
 
+func TestRestoreChain(t *testing.T) {
+	const num = 10
+	imgs := []*Image{}
+	for i := 0; i < num; i++ {
+		imgs = append(imgs, NewImage(1, 1, opengl.Nearest, false))
+	}
+	defer func() {
+		for _, img := range imgs {
+			img.Dispose()
+		}
+	}()
+	clr := color.RGBA{0x00, 0x00, 0x00, 0xff}
+	imgs[0].Fill(clr)
+	for i := 0; i < num-1; i++ {
+		imgs[i+1].DrawImage(imgs[i], vertices(), &affine.ColorM{}, opengl.CompositeModeSourceOver)
+	}
+	if err := ResolveStalePixels(); err != nil {
+		t.Fatal(err)
+	}
+	if err := Restore(); err != nil {
+		t.Fatal(err)
+	}
+	want := clr
+	for i, img := range imgs {
+		got := uint8SliceToColor(img.BasePixelsForTesting())
+		if got != want {
+			t.Errorf("%d: got %v, want %v", i, got, want)
+		}
+	}
+}
+
 func TestRestoreOverrideSource(t *testing.T) {
 	img0 := NewImage(1, 1, opengl.Nearest, false)
 	img1 := NewImage(1, 1, opengl.Nearest, false)