Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Allow user to apply arbitrary constraint on models in sample consensus #4260

Merged
merged 4 commits into from
Sep 12, 2020
Merged

Allow user to apply arbitrary constraint on models in sample consensus #4260

merged 4 commits into from
Sep 12, 2020

Conversation

mvieth
Copy link
Member

@mvieth mvieth commented Jul 9, 2020 •
edited by kunaltyagi
Loading

I used a std::function to make SACModel more versatile. This allows:

  • finding a plane that is horizontal or vertical (parallel or perpendicular to the ground). With this PR, there is a nicer solution than first searching for horizontal planes (possibly without success), then vertical planes.
  • restricting to perpendicular planes, i.e. providing a SampleConsensusModelNormalPerpendicularPlane, similar to SampleConsensusModelNormalParallelPlane
  • control over the position of circles and spheres
  • define arbitary constraints for the models, the current options (radius, direction, ...) are a bit limiting

kunaltyagi
kunaltyagi reviewed Jul 9, 2020
View reviewed changes
sample_consensus/include/pcl/sample_consensus/sac_model.h Outdated
@@ -571,6 +586,9 @@ namespace pcl
{
return ((*rng_gen_) ());
}

/** \brief A user defined function that takes model coefficients and returns whether the model is acceptable or not. */
std::function<bool(const Eigen::VectorXf &)> is_model_valid_user_defined_;
Copy link
Member

Choose a reason for hiding this comment

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

Note: ABI breakage

@kunaltyagi kunaltyagi added changelog: ABI break Meta-information for changelog generation changelog: enhancement Meta-information for changelog generation module: sample_consensus needs: feedback Specify why not closed/merged yet labels Jul 9, 2020
@kunaltyagi
Copy link
Member

kunaltyagi commented Jul 9, 2020 •
edited
Loading

The check is in non-critical code. std::function seems like a good trade-off.

Tagging @PointCloudLibrary/maintainers to answer the following:

  • is there a larger refactor lurking in here that might simplify the design for downstream classes?
  • should current models be ported over to use this?
  • separate PR or this PR?

kunaltyagi
kunaltyagi reviewed Sep 7, 2020
View reviewed changes
Copy link
Member

@kunaltyagi kunaltyagi left a comment

Choose a reason for hiding this comment

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

Please provide a scenario where this would be useful. The code LGTM. Use of function-pointer would restrict the usability and since this is not hot path, it's ok.

@kunaltyagi kunaltyagi added needs: author reply Specify why not closed/merged yet and removed needs: feedback Specify why not closed/merged yet labels Sep 7, 2020
@mvieth
Copy link
Member Author

mvieth commented Sep 7, 2020

Please provide a scenario where this would be useful

  • finding a plane that is horizontal or vertical (parallel or perpendicular to the ground). With this PR, there is a nicer solution than first searching for horizontal planes (possibly without success), then vertical planes.
  • there is currently no SampleConsensusModelNormalPerpendicularPlane, only a SampleConsensusModelNormalParallelPlane. With this PR, the models can be restricted to perpedicular planes.
  • control over the position of circles and spheres
  • you can basically define any constraints for the models, the current options (radius, direction, ...) are a bit limiting

kunaltyagi
kunaltyagi reviewed Sep 7, 2020
View reviewed changes
Copy link
Member

@kunaltyagi kunaltyagi left a comment

Choose a reason for hiding this comment

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

Edited the main comment to reflect the use-case.

sample_consensus/include/pcl/sample_consensus/sac_model.h Outdated Show resolved Hide resolved
@kunaltyagi kunaltyagi changed the title Add option for user to specify any model constraints in sample consensus Allow user to apply arbitary constraint on models in sample consensus Sep 7, 2020
@kunaltyagi kunaltyagi changed the title Allow user to apply arbitary constraint on models in sample consensus Allow user to apply arbitrary constraint on models in sample consensus Sep 7, 2020
@SergioRAgostinho SergioRAgostinho removed their request for review September 7, 2020 10:49
@kunaltyagi kunaltyagi added needs: code review Specify why not closed/merged yet and removed needs: author reply Specify why not closed/merged yet labels Sep 10, 2020
@kunaltyagi kunaltyagi self-requested a review September 10, 2020 17:07
@kunaltyagi kunaltyagi added this to the pcl-1.12.0 milestone Sep 12, 2020
@kunaltyagi kunaltyagi merged commit 0816a8c into PointCloudLibrary:master Sep 12, 2020
@mvieth mvieth deleted the is_model_valid branch September 13, 2020 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Morwenn Morwenn Morwenn left review comments

@kunaltyagi kunaltyagi kunaltyagi approved these changes

Assignees
No one assigned
Labels
changelog: ABI break Meta-information for changelog generation changelog: enhancement Meta-information for changelog generation module: sample_consensus needs: code review Specify why not closed/merged yet
Projects
None yet
Milestone
pcl-1.12.0
Development

Successfully merging this pull request may close these issues.
3 participants
@mvieth @kunaltyagi @Morwenn