Add OpenGR wrapper by sloriot · Pull Request #3477 · CGAL/cgal

sloriot · 2018-11-25T21:51:22Z

Add a CGAL wrapper to OpenGR providing methods for registering point clouds.

TODO:

get rid of FindOpenGR.cmake
small feature
add support for other registration methods (through named parameters?)
add interface to other functions (compute_transformation(), ...)
check potential issue of compiling OpenGR library with a given version of Eigen and using CGAL with another version of Eigen.

sloriot · 2018-11-25T22:31:51Z

@nmellado I think it would make think simpler on the cmake side for CGAL if when building OpenGR in addition to generated/OpenGRConfig.cmake (that is generated for the purpose of make install IIUC) you also generate a OpenCRConfig.cmake directly in the build directory that enables anyone to use the current build directory.

Basically, this means making a copy of libs in a lib directory in the build dir and updating

set(OpenGR_INCLUDE_DIR "/usr/local/include/")
set(OpenGR_LIB_DIR "/usr/local/lib/")
set(OpenGR_LIBRARIES opengr_accel opengr_io opengr_algo)

using ${CMAKE_SOURCE_DIR}/src and ${CMAKE_BUILD_DIR}/lib for exemple.

afabri · 2018-11-26T07:59:14Z

Do we really have to copy the points?

nmellado · 2018-11-26T08:04:31Z

Thanks @sloriot for preparing this PR.
This remind me I've still not merged my OpenGR branch with the stable API (STORM-IRIT/OpenGR#13)

I've just started, I've a couple of things to fix but will try to do it today.

@afabri I have to improve a bit the OpenGR API so points does not need to be copied from CGal. But as discussed with @sloriot, we can first try with the current version where points are copied.

nmellado · 2018-11-26T21:26:50Z

@sloriot FYI the new API have been merged in master, I will try to update this code tomorrow accordingly.

sloriot · 2018-11-29T00:57:20Z

@nmellado I did the update.

sloriot · 2018-11-29T01:29:18Z

@nmellado I created STORM-IRIT/OpenGR#15 that allows me to remove FindOpenGR.cmake from this branch.

nmellado · 2018-11-29T13:56:43Z

Regarding this point:

check potential issue of compiling OpenGR library with a given version of Eigen and using CGAL with another version of Eigen.
Having OpenGR as header only would solve this. RIght now OpenGR is header only, except the io package.

One solution would be to add package components to OpenGR, i.e. having one component per target that exists today.
Thus you could do something:

find_package(OpenGR COMPONENTS algo accel utils )

Of course this would also require an option in OpenGR to build the library (ie. generate the cmake package) without compiling IO.

Sounds good to you ?

sloriot · 2018-11-30T08:30:26Z

Right now it just does not link with the IO lib. So it is probably no longer an issue.

nmellado · 2018-11-30T08:31:07Z

Fair enough.

sloriot · 2018-12-04T15:20:43Z

@nmellado does it make sense to offer an interface to another method than super4PCS for now?
Could you point me to the documentation of the options?

nmellado · 2018-12-06T13:18:15Z

We can stick to Super4PCS now, the other approaches might need more testing.
Supporting 4PCS should come at no cost however.

Parameter setting is described here: https://storm-irit.github.io/OpenGR/a00012.html

sloriot · 2018-12-07T12:29:41Z

From what I see there are only 3 parameters. @sgiraudot what do you think? shall we pass the option struct or shall we add new named parameters (provided that I already added one to pass the options, it means that we'd need to only add two new). I think I'm in favor of adding new named parameters except if @nmellado says that the options are not stable and more might be added.
@nmellado About the documentation of the parameters, are you OK if we somehow copy/paste your doc?

sgiraudot · 2018-12-07T13:01:03Z

I'm also in favor of adding new parameters, especially because they can be re-used by other algorithms (something like accuracy is general enough to have another usage somewhere else later on). Which wouldn't be the case with the option struct. Also NP is already a quite complex/flexible way to handle parameters, using them to carry a struct of several parameters seems like adding an unnecessary layer of complexity.

By the way, we could already thinking of recycling some existing named parameters, for example number_of_output_points could be renamed number_of_samples, be also used for OpenGR and be documented more generally in the NP page (and more specifically on the functions that use it).

nmellado · 2018-12-07T14:05:14Z

@sloriot it's ok you can copy/paste the doc, and link to the OpengGR version, in case I update it.

The parameters are stables and should not move that much.
Actually, there is no any parameters that is specific to Super4PCS itself:

Overlap: specific to RANSAC-based algorithms (e.g. 3PCS, (Super)4PCS). Other kinds of algorithms might use similar parameter to sub-sample the input clouds.
Number of samples: related to the sub-sampling stage. Note that for CGAL's wrapper, we could skip this step and assume the user gives a subsampled point cloud, obtained by CGAL algorithms. It is maybe more generic and powerful than using the simple quantification approach used in current version of OpenGR.
Registration accuracy: related to the alignment metric. This metric could be used in other contexts (to compute distances between models) or may be replaced by another metric.
Note: this is potential change for next release of OpenGR, where the metric can be changed (e.g. set as a template parameter of the registration method).

sloriot · 2019-01-10T05:20:54Z

BGL/include/CGAL/boost/graph/parameters_interface.h

+CGAL_add_named_parameter(number_of_samples_t, number_of_samples, number_of_samples)
+CGAL_add_named_parameter(accuracy_t, accuracy, accuracy)
+CGAL_add_named_parameter(maximum_running_time_t, maximum_running_time, maximum_running_time)
+CGAL_add_named_parameter(overlap_t, overlap, overlap)


new parameters should be tested in test_cgal_bgl_named_params.cpp

That is correct, I'll do it.

sloriot · 2019-01-10T05:21:11Z

BGL/test/BGL/test_cgal_bgl_named_params.cpp

  assert(get_param(np, CGAL::internal_np::face_normal).v == 36);
  assert(get_param(np, CGAL::internal_np::random_seed).v == 37);
  assert(get_param(np, CGAL::internal_np::do_project).v == 38);
+  assert(get_param(np, CGAL::internal_np::opengr_options).v == 9001);


this one no longer exists

I'll update the test with the new NP and remove this one.

sloriot · 2019-01-10T05:21:41Z

Documentation/doc/Documentation/Installation.txt


+\subsection thirdpartyOpenGR OpenGR 
+
+\sc{OpenGR} is a is a set C++ libraries for 3D Global Registration released under the terms of the APACHE V2 licence.


typo: is a is

sloriot · 2019-01-10T05:23:38Z

Point_set_processing_3/doc/Point_set_processing_3/Point_set_processing_3.txt

+\cgalCite{cgal:mam-sffgp-14};
+
+- `CGAL::OpenGR::register_point_sets()` computes the registration of
+one point set w.r.t. another and directly aligns it to it.


aligns them?

Sounds better, yes.

sloriot · 2019-01-10T05:29:27Z

Point_set_processing_3/include/CGAL/OpenGR/register_point_sets.h

+
+   \return the registration score.
+*/
+template <class PointRange1, class PointRange2,


What do you think of removing all the doc and point to the function computing the transformation matrix + saying that the transformation is applied to all the points from the second set?

It's a good idea, avoiding duplicating the full documentation of the function.

maxGimeno · 2019-01-21T08:42:08Z

Travis report : In file included from /home/travis/build/CGAL/cgal/Installation/../Point_set_processing_3/include/CGAL/OpenGR/register_point_sets.h:31:
/home/travis/build/CGAL/cgal/Point_set_processing_3/include/CGAL/OpenGR/compute_registration_transformation.h:33:10: fatal error: 'gr/algorithms/FunctorSuper4pcs.h' file not found
#include <gr/algorithms/FunctorSuper4pcs.h>
^

lrineau · 2019-06-03T11:36:07Z

@sloriot You asked this morning about this pull-request. It is stalled:

waiting for small feature page, and that is probably what blocked it at the first time,
Travis tests did not pass,
there is a conflict with master.

done based on super4pcs_test.cc

OpenGR HEAD = cbce48e030

sloriot · 2019-08-19T06:07:40Z

Replaced by #4153

sloriot added Small feature Pkg::Point_set_processing_3 Work in progress labels Nov 25, 2018

sloriot force-pushed the OpenGR_wrapper branch from ea11f74 to fb9f5a7 Compare November 25, 2018 21:51

lrineau added Conflicts rm: not for next release Indicate to the release team that a PR should not be merged before the next release branch is forked labels Jan 7, 2019

lrineau added this to the 4.15-beta milestone Jan 7, 2019

sgiraudot force-pushed the OpenGR_wrapper branch from dd56b93 to 5dc0653 Compare January 8, 2019 10:31

sloriot commented Jan 10, 2019

View reviewed changes

lrineau added Tests failing and removed Conflicts rm: not for next release Indicate to the release team that a PR should not be merged before the next release branch is forked labels Jan 21, 2019

maxGimeno added the Stalled label Jun 17, 2019

sloriot and others added 14 commits June 26, 2019 11:29

first draft version of OpenGR wrapper

7db93d0

done based on super4pcs_test.cc

add named parameters for OpenGR options

63fb9a6

return score and silent logger

3d616d1

update after OpenGR update

3574423

OpenGR HEAD = cbce48e030

remove no longer needed file

781b693

fix license checker

efb90f8

add compute_transformation

5c524b1

Separate files and rename functions

f83191e

Modify named parameters and begin ref manual

6c42f63

Documentation of registration functions (user + ref + biblio)

bbd27fe

Copy-paste official doc

0e9ec13

Add plugin for registration

ebaf506

Add verbose display of matrix

076cce0

Add tests for all Point Set Processing named parameters in BGL test

bc28543

sloriot force-pushed the OpenGR_wrapper branch from 8a69a1e to bc28543 Compare June 26, 2019 09:38

sloriot removed Conflicts Stalled labels Jun 26, 2019

lrineau modified the milestones: 5.0-beta1, 5.1-beta Jul 12, 2019

sloriot closed this Aug 19, 2019

lrineau added the Replaced label Aug 19, 2019

MaelRL modified the milestones: 5.1-beta, Trash / Attic Aug 19, 2019

MaelRL removed the Tests failing label Jan 15, 2020

sloriot deleted the OpenGR_wrapper branch July 1, 2020 13:01

MaelRL removed Small feature Work in progress labels Jul 3, 2020


		\subsection thirdpartyOpenGR OpenGR

		\sc{OpenGR} is a is a set C++ libraries for 3D Global Registration released under the terms of the APACHE V2 licence.

Conversation

sloriot commented Nov 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sloriot commented Nov 25, 2018

Uh oh!

afabri commented Nov 26, 2018

Uh oh!

nmellado commented Nov 26, 2018

Uh oh!

nmellado commented Nov 26, 2018

Uh oh!

sloriot commented Nov 29, 2018

Uh oh!

sloriot commented Nov 29, 2018

Uh oh!

nmellado commented Nov 29, 2018

Uh oh!

sloriot commented Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nmellado commented Nov 30, 2018

Uh oh!

sloriot commented Dec 4, 2018

Uh oh!

nmellado commented Dec 6, 2018

Uh oh!

sloriot commented Dec 7, 2018

Uh oh!

sgiraudot commented Dec 7, 2018

Uh oh!

nmellado commented Dec 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxGimeno commented Jan 21, 2019

Uh oh!

lrineau commented Jun 3, 2019

Uh oh!

sloriot commented Aug 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

sloriot commented Nov 25, 2018 •

edited

Loading

sloriot commented Nov 30, 2018 •

edited

Loading