Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Closed
canadaduane opened this issue Feb 25, 2022 · 6 comments · Fixed by #43
Closed

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

canadaduane opened this issue Feb 25, 2022 · 6 comments · Fixed by #43
Labels

Comments

@canadaduane
Copy link
Contributor

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
Copy link
Contributor Author

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

@canadaduane
Copy link
Contributor Author

canadaduane commented Feb 25, 2022

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
Copy link
Owner

vanruesc commented Feb 26, 2022

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 vanruesc added the bug label Feb 26, 2022
@vanruesc
Copy link
Owner

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:

return new OctreeIterator(this, region);

@canadaduane
Copy link
Contributor Author

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

Should line 243 also be passed this.root?

return new OctreeIterator(this);

@canadaduane
Copy link
Contributor Author

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".

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants