From f593725bf9e8b8572dd90d7a2d6d32d01c5bf980 Mon Sep 17 00:00:00 2001 From: Hajime Hoshi Date: Fri, 11 Nov 2022 19:34:10 +0900 Subject: [PATCH] Revert "internal/packing: refactoring" This reverts these commits * 8fa36cc7ef62ef4eb88ea4155a94ee4aa5101c00. * e08078d84a524fdd880b871c5e86958dd320af8f Reason: test failures Updates #2327 --- internal/atlas/image.go | 29 ++++++-- internal/packing/packing.go | 87 +++++++---------------- internal/packing/packing_test.go | 116 +++++++++++++++++++++---------- 3 files changed, 131 insertions(+), 101 deletions(-) diff --git a/internal/atlas/image.go b/internal/atlas/image.go index 2e390d132..ceb64ba2d 100644 --- a/internal/atlas/image.go +++ b/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 } diff --git a/internal/packing/packing.go b/internal/packing/packing.go index d284be518..65a1c381e 100644 --- a/internal/packing/packing.go +++ b/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 +} diff --git a/internal/packing/packing_test.go b/internal/packing/packing_test.go index ef1cc0aa7..946a45b6d 100644 --- a/internal/packing/packing_test.go +++ b/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) } }