Revert "internal/packing: refactoring"

This reverts these commits

* 8fa36cc7ef.
* e08078d84a

Reason: test failures

Updates #2327

internal/atlas/image.go

@@ -139,14 +139,33 @@ type backend struct {
 }
 
 func (b *backend) tryAlloc(width, height int) (*packing.Node, bool) {
-	n := b.page.Alloc(width, height)
-	if n == nil {
-		// The page can't be extended any more. Return as failure.
-		return nil, false
+	// If the region is allocated without any extension, that's fine.
+	if n := b.page.Alloc(width, height); n != nil {
+		return n, true
 	}
 
-	b.restorable = b.restorable.Extend(b.page.Size())
+	nExtended := 1
+	var n *packing.Node
+	for {
+		if !b.page.Extend(nExtended) {
+			// The page can't be extended any more. Return as failure.
+			return nil, false
+		}
+		nExtended++
+		n = b.page.Alloc(width, height)
+		if n != nil {
+			b.page.CommitExtension()
+			break
+		}
+		b.page.RollbackExtension()
+	}
 
+	s := b.page.Size()
+	b.restorable = b.restorable.Extend(s, s)
+
+	if n == nil {
+		panic("atlas: Alloc result must not be nil at TryAlloc")
+	}
 	return n, true
 }
 

internal/packing/packing.go

@@ -84,33 +84,7 @@ func square(width, height int) float64 {
 	return float64(height) / float64(width)
 }
 
-func (p *Page) canAlloc(n *Node, width, height int) bool {
-	if p.root == nil {
-		return p.size >= width && p.size >= height
-	}
-	return canAlloc(p.root, width, height)
-}
-
-func canAlloc(n *Node, width, height int) bool {
-	if n.width < width || n.height < height {
-		return false
-	}
-	if n.used {
-		return false
-	}
-	if n.child0 == nil && n.child1 == nil {
-		return true
-	}
-	if canAlloc(n.child0, width, height) {
-		return true
-	}
-	if canAlloc(n.child1, width, height) {
-		return true
-	}
-	return false
-}
-
-func alloc(n *Node, width, height int) *Node {
+func (p *Page) alloc(n *Node, width, height int) *Node {
 	if n.width < width || n.height < height {
 		return nil
 	}
@@ -155,22 +129,22 @@ func alloc(n *Node, width, height int) *Node {
 				parent: n,
 			}
 		}
-		return alloc(n.child0, width, height)
+		return p.alloc(n.child0, width, height)
 	}
 	if n.child0 == nil || n.child1 == nil {
 		panic("packing: both two children must not be nil at alloc")
 	}
-	if node := alloc(n.child0, width, height); node != nil {
+	if node := p.alloc(n.child0, width, height); node != nil {
 		return node
 	}
-	if node := alloc(n.child1, width, height); node != nil {
+	if node := p.alloc(n.child1, width, height); node != nil {
 		return node
 	}
 	return nil
 }
 
-func (p *Page) Size() (int, int) {
-	return p.size, p.size
+func (p *Page) Size() int {
+	return p.size
 }
 
 func (p *Page) SetMaxSize(size int) {
@@ -184,11 +158,6 @@ func (p *Page) Alloc(width, height int) *Node {
 	if width <= 0 || height <= 0 {
 		panic("packing: width and height must > 0")
 	}
-
-	if !p.extendFor(width, height) {
-		return nil
-	}
-
 	if p.root == nil {
 		p.root = &Node{
 			width:  p.size,
@@ -201,7 +170,8 @@ func (p *Page) Alloc(width, height int) *Node {
 	if height < minSize {
 		height = minSize
 	}
-	return alloc(p.root, width, height)
+	n := p.alloc(p.root, width, height)
+	return n
 }
 
 func (p *Page) Free(node *Node) {
@@ -239,29 +209,7 @@ func walk(n *Node, f func(n *Node) error) error {
 	return nil
 }
 
-func (p *Page) extendFor(width, height int) bool {
-	if p.canAlloc(p.root, width, height) {
-		return true
-	}
-
-	nExtended := 1
-	for {
-		if !p.extend(nExtended) {
-			// The page can't be extended any more. Return as failure.
-			return false
-		}
-		nExtended++
-		if p.canAlloc(p.root, width, height) {
-			p.rollbackExtension = nil
-			break
-		}
-		p.rollbackExtension()
-		p.rollbackExtension = nil
-	}
-	return true
-}
-
-func (p *Page) extend(count int) bool {
+func (p *Page) Extend(count int) bool {
 	if p.rollbackExtension != nil {
 		panic("packing: Extend cannot be called without rolling back or committing")
 	}
@@ -370,3 +318,20 @@ func (p *Page) extend(count int) bool {
 
 	return true
 }
+
+// RollbackExtension rollbacks Extend call once.
+func (p *Page) RollbackExtension() {
+	if p.rollbackExtension == nil {
+		panic("packing: RollbackExtension cannot be called without Extend")
+	}
+	p.rollbackExtension()
+	p.rollbackExtension = nil
+}
+
+// CommitExtension commits Extend call.
+func (p *Page) CommitExtension() {
+	if p.rollbackExtension == nil {
+		panic("packing: RollbackExtension cannot be called without Extend")
+	}
+	p.rollbackExtension = nil
+}

internal/packing/packing_test.go

@@ -253,63 +253,109 @@ func TestPage(t *testing.T) {
 	}
 }
 
-func TestAlloc(t *testing.T) {
-	p := packing.NewPage(1024, 2048)
-	w, h := p.Size()
-	p.Alloc(w/2, h/2)
-
-	n0 := p.Alloc(w*3/2, h*2)
-	if n0 == nil {
-		t.Errorf("p.Alloc failed: width: %d, height: %d", w*3/2, h*2)
+func TestExtend(t *testing.T) {
+	p := packing.NewPage(1024, 4096)
+	s := p.Size()
+	p.Alloc(s/2, s/2)
+	p.Extend(1)
+	if p.Size() != s*2 {
+		t.Errorf("p.Size(): got: %d, want: %d", p.Size(), s*2)
 	}
-	n1 := p.Alloc(w/2, h*3/2)
+	n0 := p.Alloc(s*3/2, s*2)
+	if n0 == nil {
+		t.Errorf("p.Alloc failed: width: %d, height: %d", s*3/2, s*2)
+	}
+	n1 := p.Alloc(s/2, s*3/2)
 	if n1 == nil {
-		t.Errorf("p.Alloc failed: width: %d, height: %d", w/2, h*3/2)
+		t.Errorf("p.Alloc failed: width: %d, height: %d", s/2, s*3/2)
 	}
 	if p.Alloc(1, 1) != nil {
-		t.Errorf("p.Alloc(1, 1) must fail but not")
+		t.Errorf("p.Alloc must fail: width: %d, height: %d", 1, 1)
 	}
 	p.Free(n1)
-	if p.Alloc(1, 1) == nil {
-		t.Errorf("p.Alloc(1, 1) failed")
-	}
 	p.Free(n0)
+
+	p.RollbackExtension()
+
+	if got, want := p.Size(), s; got != want {
+		t.Errorf("p.Size(): got: %d, want: %d", got, want)
+	}
+	if p.Alloc(s*3/2, s*2) != nil {
+		t.Errorf("p.Alloc(%d, %d) must fail but not", s*3/2, s*2)
+	}
+	if p.Alloc(s/2, s*3/2) != nil {
+		t.Errorf("p.Alloc(%d, %d) must fail but not", s/2, s*3/2)
+	}
 }
 
-func TestAlloc2(t *testing.T) {
-	p := packing.NewPage(1024, 2048)
-	w, h := p.Size()
-	p.Alloc(w/2, h/2)
-	n1 := p.Alloc(w/2, h/2)
-	n2 := p.Alloc(w/2, h/2)
-	p.Alloc(w/2, h/2)
+func TestExtend2(t *testing.T) {
+	p := packing.NewPage(1024, 4096)
+	s := p.Size()
+	p.Alloc(s/2, s/2)
+	n1 := p.Alloc(s/2, s/2)
+	n2 := p.Alloc(s/2, s/2)
+	p.Alloc(s/2, s/2)
 	p.Free(n1)
 	p.Free(n2)
-
-	n3 := p.Alloc(w, h*2)
-	if n3 == nil {
-		t.Errorf("p.Alloc failed: width: %d, height: %d", w, h*2)
+	p.Extend(1)
+	if p.Size() != s*2 {
+		t.Errorf("p.Size(): got: %d, want: %d", p.Size(), s*2)
 	}
-	n4 := p.Alloc(w, h)
+
+	n3 := p.Alloc(s, s*2)
+	if n3 == nil {
+		t.Errorf("p.Alloc failed: width: %d, height: %d", s, s*2)
+	}
+	n4 := p.Alloc(s, s)
 	if n4 == nil {
-		t.Errorf("p.Alloc failed: width: %d, height: %d", w, h)
+		t.Errorf("p.Alloc failed: width: %d, height: %d", s, s)
+	}
+	if p.Alloc(s, s) != nil {
+		t.Errorf("p.Alloc must fail: width: %d, height: %d", s, s)
 	}
 	p.Free(n4)
 	p.Free(n3)
-}
 
-func TestAllocJustSize(t *testing.T) {
-	p := packing.NewPage(1024, 4096)
-	if p.Alloc(4096, 4096) == nil {
-		t.Errorf("got: nil, want: non-nil")
+	p.RollbackExtension()
+
+	if got, want := p.Size(), s; got != want {
+		t.Errorf("p.Size(): got: %d, want: %d", got, want)
+	}
+	if p.Alloc(s, s*2) != nil {
+		t.Errorf("p.Alloc(%d, %d) must fail but not", s, s*2)
+	}
+	if p.Alloc(s, s) != nil {
+		t.Errorf("p.Alloc(%d, %d) must fail but not", s, s)
 	}
 }
 
 // Issue #1454
-func TestAllocTooMuch(t *testing.T) {
+func TestExtendTooMuch(t *testing.T) {
 	p := packing.NewPage(1024, 4096)
 	p.Alloc(1, 1)
-	if p.Alloc(4096, 4096) != nil {
-		t.Errorf("got: non-nil, want: nil")
+	if got, want := p.Extend(3), false; got != want {
+		t.Errorf("got: %t, want: %t", got, want)
+	}
+}
+
+func TestExtendWithoutAllocation(t *testing.T) {
+	p := packing.NewPage(1024, 4096)
+
+	if got, want := p.Extend(2), true; got != want {
+		t.Errorf("got: %t, want: %t", got, want)
+	}
+
+	p.RollbackExtension()
+	if got, want := p.Size(), 1024; got != want {
+		t.Errorf("p.Size(): got: %d, want: %d", got, want)
+	}
+
+	if got, want := p.Extend(2), true; got != want {
+		t.Errorf("got: %t, want: %t", got, want)
+	}
+
+	p.CommitExtension()
+	if got, want := p.Size(), 4096; got != want {
+		t.Errorf("p.Size(): got: %d, want: %d", got, want)
 	}
 }