Binary Heap Driving Me Mad

I’m using a binary heap for getting intersections with distances in ascending order… at least it does most of the time, I get spurious numbers out of order. Here’s the code distilled down to demonstrate it:-


class HeapProblem
{
  /** Max number of intersections */
  static final int MAX_INTERSECTIONS = 300;

  /** Minimal version of the actual Intersection class */
  class Intersection
  {
    double distance;
  }
  
  /** Intersections stored in a binary heap so we can quickly get them in distance order */
  protected Intersection[] inSectHeap = new Intersection[MAX_INTERSECTIONS];
  /** Index of the end of the heap */
  protected int heapEnd = -1;

  /**
   * Test it
   */
  public static void main(String[] args)
  {
    HeapProblem hp = new HeapProblem();
  }

  /**
   * Testing...
   */
  protected HeapProblem()
  {
    // Shove a load of random numbers on
    java.util.Random rand = new java.util.Random();
    for (int i = 0; i < 100; i++)
    {
      Intersection inSect = new Intersection();
      inSect.distance = rand.nextDouble() * 1000;
      addIntersection(inSect);
    }
    
    // Pull 'em off, shout if out of order
    double prevDist = 0;
    while (heapEnd >= 0)
    {
      Intersection inSect = getNextIntersection();
      System.out.println(inSect.distance + ((inSect.distance < prevDist)? " *WRONG!*" : ""));
      prevDist = inSect.distance;
    }
  }
  
  /**
   * Add intersection to heap
   * @param inSect The intersection to add
   */
  private void addIntersection(Intersection inSect)
  {
    inSectHeap[++heapEnd] = inSect;
    int index = heapEnd;
    int parent = index >> 1;

    // Bubble intersection with shortest distance to the top
    while (inSectHeap[index].distance < inSectHeap[parent].distance)
    {
      Intersection temp = inSectHeap[parent];
      inSectHeap[parent] = inSectHeap[index];
      inSectHeap[index] = temp;
      index = parent;
      parent >>= 1;
    }
  }

  /**
   * Get next nearest intersection from the heap
   * @return intersection
   */
  Intersection getNextIntersection()
  {
    Intersection next = inSectHeap[0];      // Next nearest is on top
    inSectHeap[0] = inSectHeap[heapEnd--];  // Move the end of the heap to the top
    
    // Bubble next nearest to top
    for (int index = 0; (index << 1) < heapEnd; )
    {
      int left = (index << 1) + 1;
      int right = left + 1;
      // Choose the lower of the two children
      int child = (inSectHeap[left].distance < inSectHeap[right].distance)? left : right;
      if (inSectHeap[index].distance >= inSectHeap[child].distance)
      {
        Intersection temp = inSectHeap[child];
        inSectHeap[child] = inSectHeap[index];  // Swap it with current one
        inSectHeap[index] = temp;
      }
      index = child;
    }

    return next; 
  }  

}

Any thoughts on what might be wrong? Thanks.

(Moderators feel free to shunt this into another forum if more appropriate)

I think the problem lies with the division method;

int parent = index >> 1;

example: heap: {7,10,13}, heapEnd=2
addIntersection(dist=12)
so, heap={7,10,13,12}; ++heapEnd==3; index==3; parent==3>>1==1!!! should be 1.5 but rounds down not up
so you get;
‘while heap[3] < heap[1]’ (12<10: false, drops out)
so the result is
heap: {7,10,13,12} - no good!
it should be ‘while heap[3] < heap[2]’ (12<13 true, swaps values)
heap: {7,10,12,13} - good!

Concur. That works with 1-based indexing but not with 0-based. In your downheap you correctly have left = (parent << 1) + 1 and right = (parent << 1) + 2. Therefore right >> 1 = parent + 1, not parent. To compute the parent you want (index - 1) >> 1.

Incidentally, for testing it’s convenient to seed the RNG so that you always get the same results.

Hmmm… I tried it with (index - 1) >> 1 for the parent but that gets me an array index out of bounds, so I tried it starting with heapEnd = 0 and an Intersection already at 0 with distance = 0, but still have the same problem.
I’ve been working around it for the time being by using the Arrays.sort, but I’m trying to go for raw speed (It’s for a retro 90s-style raycaster).

Thanks anyway.