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

Regularization #306

Merged
merged 21 commits into from
Mar 16, 2018
Merged

Regularization #306

merged 21 commits into from
Mar 16, 2018

Conversation

dcoeurjo
Copy link
Member

@dcoeurjo dcoeurjo commented Jul 20, 2017

PR Description

Digital Surface Regularization (DGCI2017)

Previously #299

Checklist

  • Doxygen documentation of the code completed (classes, methods, types, members...).
  • Main tool doxygen documentation (following existing documentation of DGtalTools documentation.
  • Check if it follows the tools structure described in CONTRIBUTING.md
  • New entry in the ChangeLog.md added.
  • Update the readme with potentially a screenshot of the tools if it applies.
  • No warning raised in Debug cmake mode (otherwise, Travis C.I. will fail).

@dcoeurjo
Copy link
Member Author

beside #307, this code can be reviewed

@kerautret
Copy link
Member

nice, thanks, I will have a look once I found a moment ;)

@copyme
Copy link
Member

copyme commented Jul 27, 2017

I will have a look too. For the obvious reasons ;-)

@dcoeurjo
Copy link
Member Author

;) 👌

@@ -104,42 +104,40 @@ void CCCounter(Rank& r, Parent& p, const Image& elements, const unsigned int con
typename Image::Point decz(0,0,1);

//Merging process
for(typename Image::Domain::ConstIterator e = elements.domain().begin();
e !=elements.domain().end(); ++e)
for ( typename Image::Domain::ConstIterator e = elements.domain().begin();
Copy link
Member

Choose a reason for hiding this comment

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

Did it fix the issue #255 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

not sure.. looks like formatting only

Options options;
po::options_description po_shape("File options");
po_shape.add_options()
("image-filename,i", po::value<std::string>(&options.image_filename)->default_value(""), "input vol filename for image shape or input cvs filename for surfels and normals")
Copy link
Member

Choose a reason for hiding this comment

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

For the case of vol file, what is the threshold used to consider the object ? (128?) perhaps precise it and ideally an option to change it could be nice ;)

Copy link
Member

Choose a reason for hiding this comment

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

Have you an example cvs file example to test ?

Copy link
Member Author

Choose a reason for hiding this comment

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

for the vol, the only assumption is that 0 is the background.

po_options.add(po_shape).add(po_normal).add(po_approx).add_options()
("help,h", "display this message")
;

Copy link
Member

Choose a reason for hiding this comment

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

by using a vol generated from a mesh2vol, I obtained such a result:
capture d ecran 2018-03-12 a 15 56 09
I suppose that it is due to the fact that the volume is not filled so the thin parts overlap?

@kerautret
Copy link
Member

fine, I think that we can merge, It works fine on set of full voxels:
capture d ecran 2018-03-16 a 10 00 22

@kerautret
Copy link
Member

not tested on imported normals since the conversion with surfers need a little time to check in my tools. For the thin structure I put an issue.

@kerautret kerautret merged commit 4d6f99b into DGtal-team:master Mar 16, 2018
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.

None yet

4 participants