Skip to content

Commit c4f8d26

Browse files
committed
quadtree: fix bad sort due to pointer allocation issue
1 parent 458ea58 commit c4f8d26

File tree

4 files changed

+40
-24
lines changed

4 files changed

+40
-24
lines changed

quadtree/maxheap.go

Lines changed: 12 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -6,27 +6,18 @@ import "github.com/paulmach/orb"
66
// the furthest point from the query point in the list, hence maxHeap.
77
// When we find a point closer than the furthest away, we remove
88
// furthest and add the new point to the heap.
9-
type maxHeap []*heapItem
9+
type maxHeap []heapItem
1010

1111
type heapItem struct {
1212
point orb.Pointer
1313
distance float64
1414
}
1515

1616
func (h *maxHeap) Push(point orb.Pointer, distance float64) {
17-
// Common usage is Push followed by a Pop if we have > k points.
18-
// We're reusing the k+1 heapItem object to reduce memory allocations.
19-
// First we manaully lengthen the slice,
20-
// then we see if the last item has been allocated already.
21-
2217
prevLen := len(*h)
2318
*h = (*h)[:prevLen+1]
24-
if (*h)[prevLen] == nil {
25-
(*h)[prevLen] = &heapItem{point: point, distance: distance}
26-
} else {
27-
(*h)[prevLen].point = point
28-
(*h)[prevLen].distance = distance
29-
}
19+
(*h)[prevLen].point = point
20+
(*h)[prevLen].distance = distance
3021

3122
i := len(*h) - 1
3223
for i > 0 {
@@ -53,21 +44,20 @@ func (h *maxHeap) Push(point orb.Pointer, distance float64) {
5344

5445
// Pop returns the "greatest" item in the list.
5546
// The returned item should not be saved across push/pop operations.
56-
func (h *maxHeap) Pop() *heapItem {
57-
removed := (*h)[0]
47+
func (h *maxHeap) Pop() {
5848
lastItem := (*h)[len(*h)-1]
5949
(*h) = (*h)[:len(*h)-1]
6050

6151
mh := (*h)
6252
if len(mh) == 0 {
63-
return removed
53+
return
6454
}
6555

6656
// move the last item to the top and reset the heap
67-
mh[0] = lastItem
57+
mh[0].point = lastItem.point
58+
mh[0].distance = lastItem.distance
6859

6960
i := 0
70-
current := mh[i]
7161
for {
7262
right := (i + 1) << 1
7363
left := right - 1
@@ -92,11 +82,12 @@ func (h *maxHeap) Pop() *heapItem {
9282
}
9383

9484
// swap the nodes
95-
mh[i] = child
96-
mh[childIndex] = current
85+
mh[i].point = child.point
86+
mh[i].distance = child.distance
87+
88+
mh[childIndex].point = lastItem.point
89+
mh[childIndex].distance = lastItem.distance
9790

9891
i = childIndex
9992
}
100-
101-
return removed
10293
}

quadtree/maxheap_test.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,11 @@ func TestMaxHeap(t *testing.T) {
1414
h.Push(nil, r.Float64())
1515
}
1616

17-
current := h.Pop().distance
17+
current := h[0].distance
18+
h.Pop()
1819
for len(h) > 0 {
19-
next := h.Pop().distance
20+
next := h[0].distance
21+
h.Pop()
2022
if next > current {
2123
t.Errorf("incorrect")
2224
}

quadtree/quadtree.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,7 @@ func (q *Quadtree) add(n *node, p orb.Pointer, point orb.Point, left, right, bot
113113
// Remove will remove the pointer from the quadtree. By default it'll match
114114
// using the points, but a FilterFunc can be provided for a more specific test
115115
// if there are elements with the same point value in the tree. For example:
116+
//
116117
// func(pointer orb.Pointer) {
117118
// return pointer.(*MyType).ID == lookingFor.ID
118119
// }
@@ -273,7 +274,8 @@ func (q *Quadtree) KNearestMatching(buf []orb.Pointer, p orb.Point, k int, f Fil
273274
}
274275

275276
for i := len(v.maxHeap) - 1; i >= 0; i-- {
276-
buf[i] = v.maxHeap.Pop().point
277+
buf[i] = v.maxHeap[0].point
278+
v.maxHeap.Pop()
277279
}
278280

279281
return buf

quadtree/quadtree_test.go

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -483,6 +483,27 @@ func TestQuadtreeKNearest_sorted(t *testing.T) {
483483
}
484484
}
485485

486+
func TestQuadtreeKNearest_sorted2(t *testing.T) {
487+
q := New(orb.Bound{Max: orb.Point{8, 8}})
488+
q.Add(orb.Point{0, 0})
489+
q.Add(orb.Point{1, 1})
490+
q.Add(orb.Point{2, 2})
491+
q.Add(orb.Point{3, 3})
492+
q.Add(orb.Point{4, 4})
493+
q.Add(orb.Point{5, 5})
494+
q.Add(orb.Point{6, 6})
495+
q.Add(orb.Point{7, 7})
496+
497+
nearest := q.KNearest(nil, orb.Point{5.25, 5.25}, 3)
498+
499+
expected := []orb.Point{{5, 5}, {6, 6}, {4, 4}}
500+
for i, p := range expected {
501+
if n := nearest[i].Point(); !n.Equal(p) {
502+
t.Errorf("incorrect point %d: %v", i, n)
503+
}
504+
}
505+
}
506+
486507
func TestQuadtreeKNearest_DistanceLimit(t *testing.T) {
487508
type dataPointer struct {
488509
orb.Pointer

0 commit comments

Comments
 (0)