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

computeTriangleMeshArea() : calculate the area of a triangle mesh #5804

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

Conversation

mynickmynick
Copy link
Contributor

implements the functions computeTriangleMeshArea() : part of what discussed in issue [surface] getArea(point cloud segment) #5735

to calculate the area of a Triangles mesh

depending on cloud surface smoothness it might be advisable some preprocessing (like voxel filtering) to smooth up the surface

i tested it on different point clouds, both global or cluster-segmented and it worked fine

Copy link
Member

@mvieth mvieth left a comment

Choose a reason for hiding this comment

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

Thanks! A few general comments:

  • It seems you are using PCL_NO_PRECOMPILE for testing? Putting the declaration in the .h file and the definition in the .hpp file like this will only work if there are explicit instantiation in the .cpp file, or if PCL_NO_PRECOMPILE is defined. Here I would suggest to put definition and declaration together in the .h file, since the functions are not too large.
  • Please add some simple test in test/surface/test_gp3.cpp. Something very simple like computing the area of a tetrahedron/pyramid is fine
  • I would prefer to use const pcl::PointCloud<PointT>& as the parameter, then the pointer doesn't have to be dereferenced inside the function repeatedly
  • To be honest, I don't quite understand the second overload with the indices parameter. In which use case would the indices from triangleMesh be used for a look-up in indices?

@mynickmynick
Copy link
Contributor Author

@mvieth thanks for the feedback. I will come back to you later because these days, week end included, I am extremely busy.

@mvieth
Copy link
Member

mvieth commented Dec 2, 2023

@mynickmynick Hi, I just wanted to ask whether you are still working on this pull request?

@mynickmynick
Copy link
Contributor Author

mynickmynick commented Dec 3, 2023 via email

@mvieth
Copy link
Member

mvieth commented Dec 3, 2023

yes but i am moving to switzerland the thing is on hold i have no time now

okay

@mynickmynick
Copy link
Contributor Author

@mvieth please some more time, i just moved to a new country, new job and i am very busy. by the way i need to investigate on implementations of stereoscopic (binocular) 3d reconstruction for a (very) custom application (epipolar lines etc.) do we have something in PCL or can you suggest something about it? I am now using Halcon which works fine but not enough. thanks

@mynickmynick
Copy link
Contributor Author

i am having a very hard time rebuilding and rerunning this branch because i am using here another PC where I don't think I have the original test app using it , a lot of time has passed and the mod here you made me do very different from my original modifications prepared for issue #5735 and my branch https://github.com/mynickmynick/pcl/tree/fork_getArea :))

@mynickmynick
Copy link
Contributor Author

mynickmynick commented Jan 6, 2024

Thanks! A few general comments:

  • It seems you are using PCL_NO_PRECOMPILE for testing? Putting the declaration in the .h file and the definition in the .hpp file like this will only work if there are explicit instantiation in the .cpp file, or if PCL_NO_PRECOMPILE is defined. Here I would suggest to put definition and declaration together in the .h file, since the functions are not too large.

I am sorry but I don't understand here why I should implement it not uniform with the other functions, they are all declared in .h and implemented in .hpp

I honestly forgot the reason for PCL_NO_PRECOMPILE. I think it was some side issue with openMP if I remember correctly. openMP would not work fine without this, we also opened an issue about it

also why all old notifications have disappeared from github?? that makes it even more difficult for me to reconstruct this process and find that discussion we had with Lars about PCL_NO_PRECOMPILE

this is it #5717
there seemed to be a problem both with OpenMP AND XYZRGB
I don't understand what i have to do about it with this PCL_NO_PRECOMPILE, is the issue solved or not in the master??

  • Please add some simple test in test/surface/test_gp3.cpp. Something very simple like computing the area of a tetrahedron/pyramid is fine
  • I would prefer to use const pcl::PointCloud<PointT>& as the parameter, then the pointer doesn't have to be dereferenced inside the function repeatedly
  • To be honest, I don't quite understand the second overload with the indices parameter. In which use case would the indices from triangleMesh be used for a look-up in indices?

in my original modifications differently structured in https://github.com/mynickmynick/pcl/tree/fork_getArea this case was used. now I am having a hard time reconstructing it. may be you can help.

@mynickmynick
Copy link
Contributor Author

  • I would prefer to use const pcl::PointCloud<PointT>& as the parameter, then the pointer doesn't have to be dereferenced inside the function repeatedly

you have this dualism all over the PCL, make up your mind! ;))

minor

test_gp3

test sample pcd

correction

minor

correction

minor

minor
@mynickmynick
Copy link
Contributor Author

@mvieth i think now it's ready to merge. i did all you asked

@mvieth
Copy link
Member

mvieth commented Jan 15, 2024

Thanks for the modifications, I have three minor additional requests:

  • please move the new function up, just before the function isVisible. If it is at the bottom of the file, it may be harder to find 🙂
  • since you are checking the number of vertices (if (triangle_.vertices.size() == 3)), it would be good to do something if the face is not a triangle. For example: print an error and/or return -1 to indicate a problem
  • just wondering: inside the function, everything is double, yet the function return type is float? Why not keep the higher precision?
  • I would prefer to use const pcl::PointCloud<PointT>& as the parameter, then the pointer doesn't have to be dereferenced inside the function repeatedly

you have this dualism all over the PCL, make up your mind! ;))

If the cloud has to be stored, e.g. inside a class, a shared pointer is used (the most prominent example would be setInputCloud). Otherwise, a reference is used, e.g. for free functions that only have to "know" the cloud until the function has ended. To my knowledge, this rule is followed consistently.

@mynickmynick
Copy link
Contributor Author

all right I cannot do it before Feb 3rd

@mynickmynick
Copy link
Contributor Author

hi sorry i am very busy also on week ends, i will try to finalize this in the next weeks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants