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

Update refactoring code, new file loader #10

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Update refactoring code, new file loader #10

wants to merge 3 commits into from

Conversation

jmaleo
Copy link

@jmaleo jmaleo commented Aug 4, 2023

This pull request introduces a new code architecture designed to facilitate the implementation of new features.

Additional updates include:

  • A new file loader that enables users to load PLY files without a Mesh, using only vertex position and normal data. It also allows users to load meshes from other types of extensions, utilizing the read_triangle_mesh.h from libigl.
  • A minor feature: during the computation of a dry run MLS, the program now displays the mean neighborhood size being considered.

nmellado
nmellado reviewed Aug 4, 2023
View reviewed changes
Copy link
Contributor

@nmellado nmellado left a comment

Choose a reason for hiding this comment

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

Thanks for the proposal.
A few minors edits to fix, but it is a good start to build on.

src/GUI.h Outdated

}; // class GUI

#include "GUI.hpp"
Copy link
Contributor

Choose a reason for hiding this comment

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

Gui.hpp should be a cpp file as the class is not template and all functions are not inlined.

src/MyPointCloud.h Outdated
m_max = m_vertices.colwise().maxCoeff();
}

}; // class MyPointCloud
Copy link
Contributor

Choose a reason for hiding this comment

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

Add empty line at the end of the file

src/defines.h Outdated
#include "PointProcessing.h"

// CloudGeneration
#include "CloudGeneration.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Add empty line at the end of the file

src/definitions.h Outdated
Ponca::Basket<PPAdapter, SmoothWeightFunc, Ponca::CovariancePlaneFit>,
Ponca::DiffType::FitSpaceDer,
Ponca::CovariancePlaneDer,
Ponca::CurvatureEstimatorBase, Ponca::NormalDerivativesCurvatureEstimator>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Add empty line

src/poncaAdapters.hpp Show resolved Hide resolved
@jmaleo
Minor fixes
4a1a4bb 
minor fixes

Minor fixes 2
@jmaleo
Copy link
Author

jmaleo commented Aug 4, 2023

There is a bug occurring during the loading of some PLY files which not contain mesh information.

Some files with only vertex information (with normal) given work well, and others don't. I found that the issue is throwing by the "readPLY" function, when searching if it exists faces. A 0 division occurs at this moment.

I think, for the moment, it occurs when the PLY file contains certain information like the line : "element face 0".

Maybe the ply file wasn't well organize, or maybe I need to put an issue to this repo.

@nmellado
Copy link
Contributor

nmellado commented Oct 3, 2023

There is a bug occurring during the loading of some PLY files which not contain mesh information.

Some files with only vertex information (with normal) given work well, and others don't. I found that the issue is throwing by the "readPLY" function, when searching if it exists faces. A 0 division occurs at this moment.

I think, for the moment, it occurs when the PLY file contains certain information like the line : "element face 0".

Maybe the ply file wasn't well organize, or maybe I need to put an issue to this repo.

Right now the code automatically load the armadillo, and there is no way to pass options from the command line, right ? so
this is not a problem introduced by this PR, right ?

@jmaleo
Copy link
Author

jmaleo commented Oct 3, 2023

Yes, if we don't change the code, there is no problem for the user.

But, if someone change the filename here it could create the bug.

@nmellado
Copy link
Contributor

nmellado commented Oct 3, 2023

Did we had this bug before your PR ?

@jmaleo
Copy link
Author

jmaleo commented Oct 3, 2023 •
edited
Loading

As we may see it here into the current version of Poncascope, only obj file could be loaded. So, we didn't had this bug before.

In fact, this PR allows the user to use other files, computed by the LibIGL library, as used with the function read_triangle_mesh here.

It created the pointCloud, using the vertices and compute the normals thanks to the mesh.
But, the PLY file could contain vertices / normals without providing faces. And so there is a specific computation for this here. The problem, is that PLY file could introduce the line "element face 0", probably inducing the bug.

Maybe it isn't well pertinent, and it could exist an other way to use correctly the library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nmellado nmellado nmellado left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jmaleo @nmellado