Skip to content

Conversation

@soesau
Copy link
Member

@soesau soesau commented Nov 30, 2023

William Wen's GSoC 2023 Submission

Mentor: Martin Skrodzki
Co-Mentor: Sven Oesau

Implementation of the paper "Constraint-based Point Set Denoising using Normal Voting Tensor and Restricted Quadratic Error Metrics"

Also see Issue #7748

Todo

  • Add a section in the User Manual
  • Add a sentence in the Implementation History

…int_set.h


updated copyright

Co-authored-by: Sebastien Loriot <[email protected]>
@afabri
Copy link
Member

afabri commented Dec 1, 2023

Eigen seems to be a hard wired dependency. Is there an existing solver traits class we could use?

FT neighbor_radius = 0;
FT normal_threshold = 0.9 * (180/M_PI);
FT damping_factor = 3;
FT eigenvalue_threshold_nvt = 0.7;
Copy link
Member

Choose a reason for hiding this comment

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

Is the choice of the constants motivated?

{
update_threshold = 2 * neighbor_radius;

std::cout << "update threshold: " << update_threshold << "\n";
Copy link
Member

Choose a reason for hiding this comment

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

cleanup

// ----------------------------------------------------------------------------
/// \cond SKIP_IN_MANUAL

namespace internal {
Copy link
Member

Choose a reason for hiding this comment

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

As you can see above in line 20, the namespace internal is inside CGAL::Point_set_processing.

return Vector{new_normal[0], new_normal[1], new_normal[2]};
}

enum point_type_t {corner = 3, edge = 1, flat = 2}; // covm
Copy link
Member

Choose a reason for hiding this comment

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

The commented enum below mentions // nvt which corresponds to the function name. Is the right one used ?

// Detects type of point by using the covariance matrix eigenvalues
template <typename Kernel>
point_type_t feature_detection(
std::pair<Eigen::Vector3d, Eigen::Matrix3d> covm_eigens
Copy link
Member

Choose a reason for hiding this comment

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

const& ?

VectorMap normal_map,
const std::vector<typename PointRange::iterator>& neighbor_pwns,
const point_type_t point_type,
std::pair<Eigen::Vector3d, Eigen::Matrix3d>& eigens,
Copy link
Member

Choose a reason for hiding this comment

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

Does eigens get modified?

}

// calculate diameter of point set
typedef CGAL::Min_sphere_of_points_d_traits_3<Kernel,FT> Traits;
Copy link
Member

Choose a reason for hiding this comment

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

This introduces a heavy dependency. In case we have to keep it, we have to update package_info

namespace CGAL {

template< class F >
struct Output_rep< ::Color, F > {
Copy link
Member

Choose a reason for hiding this comment

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

Should that be moved to a CGAL header ?

namespace internal {

template <typename Kernel>
typename Kernel::FT measure_sphere_normal_deviation(
Copy link
Member

Choose a reason for hiding this comment

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

An example should not have CGAL::internal.

Copy link
Member

Choose a reason for hiding this comment

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

In fact it is a test. But nevertheless.

@MaelRL MaelRL added Not yet approved The feature or pull-request has not yet been approved. CHANGES.md not updated labels Jun 7, 2024
@MaelRL MaelRL added this to the 6.1-beta milestone Jun 7, 2024
@MaelRL MaelRL modified the milestones: 6.1-beta, 6.2-beta Mar 17, 2025
@sloriot sloriot changed the base branch from master to main September 18, 2025 05:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants