Skip to content

Fix #7449: sample_points_poisson_disk is extremely slow#7450

Open
nikste wants to merge 4 commits intoisl-org:mainfrom
nikste:fix/issue-7449
Open

Fix #7449: sample_points_poisson_disk is extremely slow#7450
nikste wants to merge 4 commits intoisl-org:mainfrom
nikste:fix/issue-7449

Conversation

@nikste
Copy link
Copy Markdown
Contributor

@nikste nikste commented Mar 1, 2026

Hi all,
I'm trying out some fun project for claude code github issue solver.
I think this should solve the issue. I'm looking at this also as a human.
Let me know if there is something i should change or try.

Summary

Automated fix for #7449: sample_points_poisson_disk is extremely slow

Fixes #7449

What does this PR do?

This PR addresses issue #7449 by implementing the fix described in the issue.


This PR was automatically created by Klaus Kode — donate your excess Claude Code credits to solve open-source issues.

Klaus Kode is not affiliated with or endorsed by Claude Code / Anthropic.

Complaints, praise, or opt-out requests: klauskode@protonmail.com

Replace the O(N × neighbors) weight recomputation in the sample
elimination loop with an O(N) incremental update that matches the
original paper (Fig. 2) and reference implementation.

Previously, when a point was eliminated, `ComputePointWeight` was
called for every neighboring point, each of which triggered a new
full KD-tree radius search. Now we reuse the distances already
returned by the single KD-tree query for the eliminated point and
simply subtract its weight contribution from each live neighbor,
reducing KD-tree queries during elimination from O(N × avg_neighbors)
to O(N) and achieving ~6-10× speedup.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@update-docs
Copy link
Copy Markdown

update-docs bot commented Mar 1, 2026

Thanks for submitting this pull request! The maintainers of this repository would appreciate if you could update the CHANGELOG.md based on your changes.

@nikste
Copy link
Copy Markdown
Contributor Author

nikste commented Mar 1, 2026

it seems when i run the stylecheck python util/check_style.py --apply as suggeste din the failing test github action, it changes some unrelated files ?

@BG4444
Copy link
Copy Markdown

BG4444 commented Mar 1, 2026

It still has some performance issues—for example, extra memory allocations for each KD-tree request (new std::vector for each time), and extra weight computations for points that have already been deleted.

@nikste
Copy link
Copy Markdown
Contributor Author

nikste commented Mar 1, 2026

just benchmarked with the bunny example you gave. The base speedup is already there from ~2.04s down to 0.368s on my machine.
I'll check the further examples you mentioned.

@nikste
Copy link
Copy Markdown
Contributor Author

nikste commented Mar 1, 2026

I'm moving the vector instantiations to outside of the loop.
It seems the KD lookup is a bit more complex and would have negligible gain for most cases.
"The deleted[nb] check is just a boolean array lookup — extremely cheap (L1 cache hit). Even if 50% of points are deleted, you're just doing a few extra branch-not-taken iterations per query"

Since there is already quite some substantial speedup, I'd defer this to another PR, since we'd either need to periodically recompute the KD tree it seems or expose it through the wrapper of open3d of Flann.

@BG4444
Copy link
Copy Markdown

BG4444 commented Mar 1, 2026

You might also consider disabling sorting in the KD-tree query output, or leveraging the sorted distances to skip weight computation for small values. The algorithm assumes that below a certain threshold distance, the weight is constant.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR targets Open3D issue #7449 by improving the performance of TriangleMesh::SamplePointsPoissonDisk through an incremental neighbor-weight update strategy during sample elimination (reducing repeated KD-tree queries).

Changes:

  • Update Poisson-disk sample elimination to decrement neighbor weights based on the deleted sample’s contribution rather than recomputing weights via additional KD-tree queries.
  • Add a changelog entry documenting the performance fix for TriangleMesh::SamplePointsPoissonDisk.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
cpp/open3d/geometry/TriangleMesh.cpp Optimizes Poisson-disk sampling by updating neighbor weights incrementally after deletions.
CHANGELOG.md Documents the Poisson-disk sampling performance improvement (issue #7449).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@nikste
Copy link
Copy Markdown
Contributor Author

nikste commented Mar 5, 2026

  • Added optional parameter to KD tree search for unsorted results
  • unit tests
  • also pulled out the vector instantiations out of the loop.

Let me know if there should be any more changes before merging.

KD tree sorted=true │ 0.347s │ 16384 points
KD tree sorted=false │ 0.309s │ 16384 points
speedup ~11%

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

std::vector<double> &distance2,
bool sorted = true) const;

template <typename T>
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

KDTreeFlann::SearchRadius’s signature was changed by adding a new parameter. Even with a default value, this is an ABI-breaking change (different mangled symbol) for any downstream C++ code built against the previous Open3D library. To avoid breaking existing binaries, consider keeping the original 4-arg overload and adding a new 5-arg overload that forwards internally (and explicitly instantiate both overloads as needed).

Suggested change
template <typename T>
template <typename T>
int SearchRadius(const T &query,
double radius,
std::vector<int> &indices,
std::vector<double> &distance2) const {
return SearchRadius(query, radius, indices, distance2, true);
}
template <typename T>

Copilot uses AI. Check for mistakes.
Comment on lines +1143 to +1147
double min_dist = std::numeric_limits<double>::max();
for (size_t i = 0; i < pcd->points_.size(); ++i) {
for (size_t j = i + 1; j < pcd->points_.size(); ++j) {
double d = (pcd->points_[i] - pcd->points_[j]).norm();
min_dist = std::min(min_dist, d);
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test uses std::numeric_limits and std::min, but this file does not include the standard headers that define them. Please add the appropriate includes (e.g., <limits> and <algorithm>) to avoid relying on transitive includes that may change across platforms/compilers.

Copilot uses AI. Check for mistakes.
Comment on lines +1150 to +1154
// For 100 points on a triangle with area 0.5, the theoretical Poisson disk
// radius is r = 2*sqrt(area/n / (2*sqrt(3))) ≈ 0.057. After sample
// elimination the actual minimum distance should be a meaningful fraction
// of that. We just check it's not degenerate (> 0.01).
EXPECT_GT(min_dist, 0.01);
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SamplePointsPoissonDisk relies on RNG-based sampling; this test doesn’t set a fixed seed, and the hard min_dist > 0.01 threshold may vary with randomness/implementation details, risking flaky CI. Consider seeding (e.g., utility::random::Seed(0)) and/or making the assertion deterministic (e.g., only require min_dist to be above a tiny epsilon, or compare against an expected value with a fixed seed).

Suggested change
// For 100 points on a triangle with area 0.5, the theoretical Poisson disk
// radius is r = 2*sqrt(area/n / (2*sqrt(3))) ≈ 0.057. After sample
// elimination the actual minimum distance should be a meaningful fraction
// of that. We just check it's not degenerate (> 0.01).
EXPECT_GT(min_dist, 0.01);
// The exact minimum distance depends on RNG and implementation details.
// To avoid flaky tests, only check that points are not degenerate
// (no coincident samples).
EXPECT_GT(min_dist, 1e-8);

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sample_points_poisson_disk is extremely slow

4 participants