vanruesc / sparse-octree

A sparse octree data structure.
zlib License
117 stars 20 forks source link

PointOctree with 8 or fewer points fails to retrieve leaves #40

Closed canadaduane closed 2 years ago

canadaduane commented 2 years ago

I think I'm describing the issue properly, although I'm a bit new to octrees in general, so any help would be appreciated.

I'm creating a PointOctree to represent the centerpoints of objects in a scene. What I'm seeing is that when my worlds have fewer then 8 items, calling leaves(frustum) returns an empty iterator.

I can see that the transition happens between when root.data has 8 elements, and when root.data becomes null (presumably because adding the 9th item shifts points + data down into lower children leaves).

canadaduane commented 2 years ago

As a workaround, I'm adding 9 "dummy points" at initialization for now.

canadaduane commented 2 years ago

Here is a failing test case (append to test/points/PointOctree.js):

test("retrieves leaves when octree is small", (t) => {
  const octree = new PointOctree(box.min, box.max);

  octree.set(new Vector3(0.0, 0, 0), data0);
  octree.set(new Vector3(0.1, 0, 0), data1);
  octree.set(new Vector3(0.2, 0, 0), data2);

  const found = [];
  for (let node of octree.leaves()) {
    if (!node.data) continue;
    const { points } = node.data;
    if (points) {
      for (let point of points) found.push(point);
    }
  }

  t.is(found.length, 3, "should find points");
});
vanruesc commented 2 years ago

Thanks for the report!

Would you like to add the failing test with a PR? I've pushed some minor changes just now, so make sure to update beforehand if you do.

vanruesc commented 2 years ago

Also, to clarify: this is technically not a bug since the leaves method returns an iterator over Nodes which are not necessarily of type DataContainer<T>. But it's still practically a bug because returning the octree node itself instead of the root node is unintended behaviour.

Changing the following line to pass in this.root instead of this should fix it, similar to #41:

https://github.com/vanruesc/sparse-octree/blob/4c9af67f3bdbddecb1840ce3b24101f9125dee5b/src/core/Octree.ts#L231

canadaduane commented 2 years ago

Ok, I've added tests. Thanks for the help!

Should line 243 also be passed this.root?

https://github.com/vanruesc/sparse-octree/blob/4c9af67f3bdbddecb1840ce3b24101f9125dee5b/src/core/Octree.ts#L243

canadaduane commented 2 years ago

I've added a PR to fix based on your suggestion, and assumed "yes" to my question above, but I can fix / change if the answer is "no".