Expose Poisson reconstruction parameters#7430
Expose Poisson reconstruction parameters#7430Chevi-Koren wants to merge 6 commits intoisl-org:mainfrom
Conversation
Expose 5 important parameters in CreateFromPointCloudPoisson that were previously hardcoded: - full_depth: Minimum depth for density estimation (default: 5) - samples_per_node: Minimum sample points per octree node (default: 1.5) - point_weight: Point interpolation weight (default: 4.0) - confidence: Normal confidence weighting exponent (default: 0.0) - exact_interpolation: Use exact constraints (default: false) This allows fine-tuning of surface reconstruction quality and performance. All parameters have sensible defaults matching previous behavior. Fully backward compatible. Fixes isl-org#7248
|
Thanks for submitting this pull request! The maintainers of this repository would appreciate if you could update the CHANGELOG.md based on your changes. |
8250778 to
e8337ab
Compare
|
@ssheorey |
There was a problem hiding this comment.
Pull request overview
Exposes additional Poisson surface reconstruction parameters in TriangleMesh.create_from_point_cloud_poisson across C++ and Python, enabling advanced users to fine-tune reconstruction quality/performance while keeping prior defaults.
Changes:
- Extended C++ API signature and internal Poisson
Executeplumbing to acceptfull_depth,samples_per_node,point_weight,confidence, andexact_interpolation. - Updated Python bindings to expose the new keyword arguments with backward-compatible defaults.
- Added Python tests and a changelog entry for the new parameters.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| python/test/geometry/test_poisson_parameters.py | Adds pytest coverage for new Poisson parameters and backward compatibility. |
| cpp/pybind/geometry/trianglemesh.cpp | Exposes new Poisson parameters to Python via pybind defaults/kwargs. |
| cpp/open3d/geometry/TriangleMesh.h | Extends the public C++ API signature and Doxygen docs for Poisson parameters. |
| cpp/open3d/geometry/SurfaceReconstructionPoisson.cpp | Threads new parameters from public API into Poisson execution path. |
| CHANGELOG.md | Documents the newly exposed Poisson parameters. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| XForm<Real, Dim + 1> xForm, iXForm; | ||
| xForm = XForm<Real, Dim + 1>::Identity(); | ||
|
|
||
| // Keep hardcoded internal parameters |
There was a problem hiding this comment.
This comment is misleading now that several parameters are no longer hardcoded (they’re passed as arguments). Consider rewording to clarify that only the remaining parameters below are still hardcoded (e.g., 'Keep other internal parameters hardcoded').
| // Keep hardcoded internal parameters | |
| // Keep other internal parameters hardcoded |
| pcd.points = o3d.utility.Vector3dVector( | ||
| np.random.rand(100, 3) - 0.5 | ||
| ) | ||
| pcd.normals = o3d.utility.Vector3dVector( | ||
| np.random.rand(100, 3) - 0.5 | ||
| ) |
There was a problem hiding this comment.
These tests rely on unseeded randomness, which can make them flaky (e.g., occasionally producing degenerate inputs that cause Poisson reconstruction to fail or vary drastically). Use a fixed seed (or deterministic sample geometry) so the tests are stable across runs and platforms.
| @pytest.mark.parametrize("params,expected_valid", [ | ||
| ({"depth": 6, "full_depth": 4, "samples_per_node": 2.0, | ||
| "point_weight": 5.0, "confidence": 0.5, "exact_interpolation": True}, True), | ||
| ({"depth": 6, "full_depth": 3}, True), | ||
| ({"depth": 6, "full_depth": 5}, True), | ||
| ({"depth": 5, "samples_per_node": 1.0}, True), | ||
| ({"depth": 5, "samples_per_node": 3.0}, True), | ||
| ]) | ||
| def test_poisson_with_various_parameters(sample_point_cloud, params, expected_valid): | ||
| """Test Poisson reconstruction with various parameter combinations.""" | ||
| mesh, densities = o3d.geometry.TriangleMesh.create_from_point_cloud_poisson( | ||
| sample_point_cloud, **params | ||
| ) | ||
| if expected_valid: | ||
| _assert_valid_mesh(mesh, densities) | ||
|
|
||
|
|
There was a problem hiding this comment.
expected_valid is always True in the parametrization, so the conditional doesn’t add value. Either remove expected_valid and the if block, or add at least one expected_valid=False case and assert the expected failure mode (e.g., with pytest.raises) to justify the parameter.
| @pytest.mark.parametrize("params,expected_valid", [ | |
| ({"depth": 6, "full_depth": 4, "samples_per_node": 2.0, | |
| "point_weight": 5.0, "confidence": 0.5, "exact_interpolation": True}, True), | |
| ({"depth": 6, "full_depth": 3}, True), | |
| ({"depth": 6, "full_depth": 5}, True), | |
| ({"depth": 5, "samples_per_node": 1.0}, True), | |
| ({"depth": 5, "samples_per_node": 3.0}, True), | |
| ]) | |
| def test_poisson_with_various_parameters(sample_point_cloud, params, expected_valid): | |
| """Test Poisson reconstruction with various parameter combinations.""" | |
| mesh, densities = o3d.geometry.TriangleMesh.create_from_point_cloud_poisson( | |
| sample_point_cloud, **params | |
| ) | |
| if expected_valid: | |
| _assert_valid_mesh(mesh, densities) | |
| @pytest.mark.parametrize("params", [ | |
| {"depth": 6, "full_depth": 4, "samples_per_node": 2.0, | |
| "point_weight": 5.0, "confidence": 0.5, "exact_interpolation": True}, | |
| {"depth": 6, "full_depth": 3}, | |
| {"depth": 6, "full_depth": 5}, | |
| {"depth": 5, "samples_per_node": 1.0}, | |
| {"depth": 5, "samples_per_node": 3.0}, | |
| ]) | |
| def test_poisson_with_various_parameters(sample_point_cloud, params): | |
| """Test Poisson reconstruction with various parameter combinations.""" | |
| mesh, densities = o3d.geometry.TriangleMesh.create_from_point_cloud_poisson( | |
| sample_point_cloud, **params | |
| ) | |
| _assert_valid_mesh(mesh, densities) |
| def test_poisson_parameter_variation(sample_point_cloud): | ||
| """Test that different parameters produce different results.""" | ||
| # Run with default point_weight | ||
| mesh1, _ = o3d.geometry.TriangleMesh.create_from_point_cloud_poisson( | ||
| sample_point_cloud, depth=5, point_weight=4.0 | ||
| ) | ||
|
|
||
| # Run with higher point_weight (should produce different result) | ||
| mesh2, _ = o3d.geometry.TriangleMesh.create_from_point_cloud_poisson( | ||
| sample_point_cloud, depth=5, point_weight=10.0 | ||
| ) | ||
|
|
||
| # Meshes should be different (different vertex counts or positions) | ||
| # We just check they both succeeded and have positive vertex counts | ||
| assert len(mesh1.vertices) > 0 | ||
| assert len(mesh2.vertices) > 0 |
There was a problem hiding this comment.
The test name/docstring claims to validate that different parameters produce different results, but the assertions only check success. Either rename/reword the test to reflect that it’s a smoke test, or add a deterministic input (see randomness comment) and assert a measurable difference (e.g., vertex count differs, bounding boxes differ, or vertex arrays are not identical within a tolerance).
CHANGELOG.md
Outdated
| @@ -1,4 +1,5 @@ | |||
| ## Main | |||
| - Exposed advanced parameters (`full_depth`, `samples_per_node`, `point_weight`, `confidence`, `exact_interpolation`) for Poisson surface reconstruction in `TriangleMesh.create_from_point_cloud_poisson` (PR #XXXX) (issue #7248) | |||
There was a problem hiding this comment.
The changelog entry still contains the placeholder 'PR #XXXX'. Replace it with the actual PR number before merging (or omit the PR number if the project convention is to have tooling fill it in).
| - Exposed advanced parameters (`full_depth`, `samples_per_node`, `point_weight`, `confidence`, `exact_interpolation`) for Poisson surface reconstruction in `TriangleMesh.create_from_point_cloud_poisson` (PR #XXXX) (issue #7248) | |
| - Exposed advanced parameters (`full_depth`, `samples_per_node`, `point_weight`, `confidence`, `exact_interpolation`) for Poisson surface reconstruction in `TriangleMesh.create_from_point_cloud_poisson` (issue #7248) |
|
Hi @Chevi-Koren thanks for submitting this useful enhancement! Please also add documentation for these parameters (C++ and Python), from the point of view of "what are they, and what is the effect of changing them from the default value?" Also, do we need all of them? We don't want to increase complexity of using Open3D unless its necessary. Or does fiddling with them help in common situations? |
|
Hi @ssheorey, thanks for the feedback! I've addressed both of your points:
I've added comprehensive documentation for all parameters explaining what they do, how they affect reconstruction, and when to adjust them. Each parameter now includes:
For example: /// \param point_weight Importance of point interpolation constraints
/// (default: 4.0). Controls the trade-off between data fidelity and surface
/// smoothness. Higher values (e.g., 10.0) prioritize fitting input points
/// exactly, resulting in surfaces closer to the data. Lower values produce
/// smoother surfaces. Recommended range: 2.0-10.0.
2. API Simplification
Following your concern about complexity, I've reduced the API to expose only 3 essential parameters:
point_weight - Most important and most requested. Users commonly need to adjust this for better data fidelity.
samples_per_node - Very useful for noise control. Common use case: noisy scans need higher values (2-3) to suppress artifacts.
full_depth - Helps with sparse regions. Useful but less critical.
I removed 2 advanced parameters:
confidence - Only relevant for confidence-weighted normals (rare use case)
exact_interpolation - Computationally expensive, rarely needed in practice
This keeps the API simple while exposing the parameters that help in common situations.
Additional Changes
Fixed all code review feedback from Copilot AI
Applied code style formatting (note: some files in this commit were automatically formatted by check_style.py)
Added deterministic random seed to tests for reproducibility
All changes remain fully backward compatible |
ssheorey
left a comment
There was a problem hiding this comment.
Can you run make apply-style please?
3rdparty/assimp/assimp.cmake
Outdated
| PREFIX assimp | ||
| URL https://github.com/assimp/assimp/archive/refs/tags/v5.4.2.zip | ||
| URL_HASH SHA256=03e38d123f6bf19a48658d197fd09c9a69db88c076b56a476ab2da9f5eb87dcc | ||
| URL_HASH SHA256=8e2673677303fdfe1a8189d1335e4f22b1ad712afa53281dc3d62d6cb1280ead |
There was a problem hiding this comment.
Is this accidental? Download URL and hash should both be changed together.
.gitignore
Outdated
| docs/getting_started.rst | ||
| docs/docker.rst | ||
| docs/tensorboard.md | ||
| venv/ |
There was a problem hiding this comment.
This is not open3d related. Remove this please.
- Expose full_depth, samples_per_node, and point_weight parameters - Add comprehensive documentation for each parameter - Add unit tests for parameter validation - Fully backward compatible with default values - All style checks applied Fixes isl-org#7430
7041d31 to
fcf9ab8
Compare
|
@ssheorey Done! I've run I removed all unrelated files that were accidentally included:
The PR now contains only the 6 files directly related to this feature:
Question: The style checker modified many other files (formatting changes in files like |
| if (exact_interpolation) { | ||
| // Use approximate interpolation (default behavior) | ||
| if (false) { | ||
| iInfo = FEMTree<Dim, Real>:: |
There was a problem hiding this comment.
We are hardcoding a single path here. Please check. Revert to original code if there is no logical difference.
|
@Chevi-Koren ensure you are using the style checker SW versions from |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "linear_fit"_a = false, "n_threads"_a = -1) | ||
| "linear_fit"_a = false, "n_threads"_a = -1, | ||
| "full_depth"_a = 5, "samples_per_node"_a = 1.5f, | ||
| "point_weight"_a = 4.0f) |
There was a problem hiding this comment.
The Python bindings are missing the confidence and exact_interpolation parameters that were mentioned in the PR description as being exposed. The PR description states that 5 parameters are being exposed, but only 3 are present in the Python bindings. Either add the missing parameters to match the description, or update the PR description to reflect that only 3 parameters are being exposed.
| "point_weight"_a = 4.0f) | |
| "point_weight"_a = 4.0f, "confidence"_a = false, | |
| "exact_interpolation"_a = false) |
| // Use approximate interpolation (default behavior) | ||
| if (false) { |
There was a problem hiding this comment.
The condition if (false) creates unreachable code. If exact interpolation is not being exposed as a parameter, this entire conditional block and the unreachable code inside should be removed to improve code maintainability. If it's intended to be exposed later, consider adding a TODO comment explaining the plan.
| // Keep other internal parameters hardcoded | ||
| float datax = 32.f; | ||
| int base_depth = 0; | ||
| int base_v_cycles = 1; | ||
| float confidence = 0.f; | ||
| float point_weight = 2.f * DEFAULT_FEM_DEGREE; | ||
| float confidence_bias = 0.f; | ||
| float samples_per_node = 1.5f; | ||
| float cg_solver_accuracy = 1e-3f; | ||
| int full_depth = 5; | ||
| int iters = 8; |
There was a problem hiding this comment.
The comment "Keep other internal parameters hardcoded" should be updated to clarify that confidence and exact_interpolation were also removed (not just kept hardcoded), since the code below no longer uses these variables at all. Consider revising to: "Keep other internal parameters hardcoded. confidence and exact_interpolation have been removed."
- Remove unreachable exact_interpolation code path (if false block) - Update comment to clarify confidence and exact_interpolation were removed - Simplify code to only use approximate interpolation Addresses code review feedback from @ssheorey and Copilot AI
|
@ssheorey Fixed! Removed the dead code branch and reverted to the single approximate interpolation path, as in the original code. |
benjaminum
left a comment
There was a problem hiding this comment.
Looks good! Thanks @Chevi-Koren !
|
Hi @Chevi-Koren can you please fix the CI errors? e.g.: The build failure occurs in cpp/open3d/geometry/SurfaceReconstructionPoisson.cpp at line 682: error: use of undeclared identifier 'datax'; did you mean 'atan'? |
|
@ssheorey Hi! Yes, fixed. The datax parameter was missing - I've added: The value 32.f matches PoissonRecon's default. |
|
@Chevi-Koren still getting CI errors. Does it work on your local machine? |
Expose Poisson reconstruction parameters #7430
Expose 3 important parameters in
CreateFromPointCloudPoissonthat were previously hardcoded:This allows fine-tuning of surface reconstruction quality and performance. All parameters have sensible defaults matching previous behavior. Fully backward compatible.
The exposed parameters are intended for users who require fine-grained control over Poisson reconstruction behavior while keeping the API simple and focused.
Fixes #7248
Type
Motivation and Context
Users currently cannot fine-tune the Poisson surface reconstruction algorithm because important parameters are hardcoded in the internal
Executefunction. This limitation was raised in issue #7248, where users requested the ability to adjust parameters likepoint_weight,samples_per_node, andfull_depthto optimize reconstruction quality for their specific use cases.This PR exposes 3 key parameters that significantly affect reconstruction quality and performance, while maintaining full backward compatibility through sensible default values that match the previous hardcoded behavior.
Checklist
python util/check_style.py --applyto apply Open3D code style to my code.Description
Changes Made
This PR modifies the
CreateFromPointCloudPoissonAPI to expose 3 parameters that were previously hardcoded:full_depth(default: 5)samples_per_node(default: 1.5)point_weight(default: 4.0)2.0 * FEM_DEGREE(whereFEM_DEGREE = 2).Files Modified
cpp/open3d/geometry/TriangleMesh.h: Updated function signature with new parameters and comprehensive Doxygen documentation.cpp/open3d/geometry/SurfaceReconstructionPoisson.cpp: ModifiedExecuteandCreateFromPointCloudPoissonto accept and pass through the new parameters.cpp/pybind/geometry/trianglemesh.cpp: Exposed new parameters to Python API with corresponding default values.Backward Compatibility
✅ Fully backward compatible — all existing code will continue to work without modifications. The new parameters have default values that exactly match the previous hardcoded behavior.
Example — existing code still works:
New code — can now fine-tune: