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.

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

Sample consensus prerejective fixes #736

Closed
wants to merge 26 commits into from
Closed

Sample consensus prerejective fixes #736

wants to merge 26 commits into from

Conversation

andersgb1
Copy link
Contributor

This PR fixes a couple of deficiencies in the sample consensus prerejective pose estimation class:

  • Instead of computing correspondences in each RANSAC iteration (as in SAC-IA), precompute them. This makes good sense when we want to turn up the number of iterations.
  • Better n-point sampler for generating relative poses (thanks to Troels Bo Jørgensen)
  • Removed a horrible condition making the algorithm continue without any evaluation in way too many cases
  • Avoided a duplicate squared Euclidean distance computation inside getFitness ()

taketwo and others added 26 commits June 11, 2014 13:51
The clang variant remains the same as it was before (i.e. complete
compilation), and the gcc task is reduced to only compiling `pcl_common`
target.
This package is guaranteed to be present on Travis
On gcc 4.8.2 (fedora 20) without c++11 enabled the definition of
uint64_t is missing.  Switch to the pcl typdef for boost::uint64.
Commit 3f363a enabled short file name generation in Doxygen to address
filesystem limitations that arose while packaging PCL. However, for the
purposes of online documentation, short names are not acceptable as they
change with every build. This makes referring to function/class
documentation impossible.

This pull request adds a CMake option `DOXYGEN_USE_SHORT_NAMES` which
affects the corresponding Doxygen option. By default it is set to `OFF`,
i.e. generated file names will be long and descriptive, just like this
used to be prior to the aforementioned commit.
Currently when using PCLConfig.cmake the libraries are searched with a
suffix of either _debug or _release.  When building with either
RelWithDebInfo or MinSizeRel these are not preoprly setup in the
CMakeLists.txt

As result when building with RelWithDebInfo you get libraries named
pcl_io.dll

but the PCLConfig.cmake looks for pcl_io_release.dll

This patch adds the release suffix to both RelWithDebInfo and
MinSizeRel to solve the problem.
Using boost definitions for uint8_t via pcl_macros.h

This allows compilation on visual studio 2008 which does not
provide stdint.h
 1. Faster sampling of n (3) random indices
 2. Precompute all correspondences, instead of matching the features in each iteration
 3. Remove a horrible bug that caused the algorithm to abort in too many cases, due wrong indexing in mask of already accepted features
 4. Avoid recomputation of squared Euclidean inlier distances inside getFitness ()

Many thanks to Troels Bo Jørgensen for 1. and 2., and to Jeppe Pedersen and Kent Hansen for 3.
Press ctrl + s to save camera parameters and ctrl + r to restore the
saved camera parameters.

If a camera file is specified with "-cam" option or pcd file(s) is(are)
available in the command line, the camera parameters will be written to
the corresponding file and can be restored later from that file.
In this case, PCLVisualizer will restore camera parameters automatically
from the corresponding file when the pragram restarts with the same
command line input.
- Add new FindGtest.cmake to find installed versions.
- Add libgtest-dev for Travis
Corrected return value for watch.getTimeSeconds() from int to double. Also corrected message that said time was in milliseconds, when watch.getTimeSeconds() returns elapsed time in seconds.
@andersgb1
Copy link
Contributor Author

OK, this is running amok. Can anyone tell me how I remove the external commits from this? FYI, they appeared when I ran step 5. of https://github.com/PointCloudLibrary/pcl/wiki/A-step-by-step-guide-on-preparing-and-submitting-a-pull-request using the command git pull upstream master.

@VictorLamoine
Copy link
Contributor

git rebase -i upstream/master
With upstream a remote to this PCL GitHub repository

After that you can also squash your commits into logical units:
https://github.com/PointCloudLibrary/pcl/blob/master/CONTRIBUTING.md#checklist

@taketwo
Copy link
Member

taketwo commented Jun 11, 2014

Hmmm, that's interesting. Maybe it makes sense to remove that step altogether, or make --rebase mandatory.

Speaking about removing the extra commits, there ase two ways. As Victor suggested, you may do interactive rebase and delete the unneeded commits. Another possibility is to start a brand new branch from the current master, and cherry-pick the needed commits onto it.

@jspricke
Copy link
Member

@taketwo Good point, normally I propose to start a branch and don't merge or rebase onto the master branch at all. If you really need commits from the master branch, you took the wrong starting point for your branch ;). In that case I would rebase onto master and test again. But I think that's something coming up rather seldom and we should not propose it in our documentation.
I would read point 5 in the documentation rather as a local test if your branch still merges. Maybe we should add something like [1] to it.

[1] http://stackoverflow.com/a/6283843

@taketwo
Copy link
Member

taketwo commented Jun 11, 2014

I would read point 5 in the documentation rather as a local test if your branch still merges.

I think in absolute majority of cases the branch will still merge, especially if the contributor followed the instructions and started the pull request branch from the latest master. So in order not to complicate things for the beginners, this point should be removed. Anyways, if a conflict happens, we'll see it in the GitHub interface and could then provide some guidance on how it could be resolved.

@jspricke
Copy link
Member

@taketwo I'm fine with that and changed the docs.

@andersgb1
Copy link
Contributor Author

OK, thanks all for the info. I see that @jspricke already edited the wiki. I will tear this PR down, and just start over, providing more sensible commits...

@andersgb1 andersgb1 closed this Jun 11, 2014
@andersgb1 andersgb1 deleted the sample_consensus_prerejective branch June 11, 2014 19:20
taketwo added a commit that referenced this pull request Jun 15, 2014
Bug fixes and optimizations for sample consensus prerejective (replaces #736)
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.

9 participants