-
Notifications
You must be signed in to change notification settings - Fork 2
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
Backend cpp for model-based semantic segmentation #33
Conversation
…f joint detector function
…heck into semantic_segmentation
… segments to mesh faces
to do:
|
…heck into semantic_segmentation
@9and3 I believe we can start the Ping-Pong (it took a bit longer than planned because a bird 🐦 flew through the window in the office, and getting it out became my mission. Birds don't know what's good for them...) from top to bottom, the result of what this branch does: the rest of the clusters after the function is called, all the segments, with the selected points in black, and the selected point only. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DamienGilliard first: I hope the little bird is fine.
On a second and less important matter, I left some comments. They are mainly generic and broad. Ping!
* @param referenceMesh the vector of mesh faces to associate with the segments | ||
* @param clusters the vector of clusters from cilantro to associate with the mesh faces of the reference mesh | ||
* @param associationThreshold the threshold to consider the points of a segment and a mesh face as associable. The lower the number, the more strict the association will be and some poinnts on the mesh face might be wrongfully excluded. | ||
* @return std::shared_ptr<geometry::DFPointCloud> The unified segments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"merged"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
has been added to the brief.
Do you think it should also be the name of the function, e.g. AssociateAndMergeClusters() ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo we can leave it as it is
* @param referenceMesh the vector of mesh faces to associate with the segments | ||
* @param clusters the vector of clusters from cilantro to associate with the mesh faces of the reference mesh | ||
* @param associationThreshold the threshold to consider the points of a segment and a mesh face as associable. The lower the number, the more strict the association will be and some poinnts on the mesh face might be wrongfully excluded. | ||
* @return std::shared_ptr<geometry::DFPointCloud> The unified segments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we return a vector of DFPointClouds? So at the end we would have ideally as many as the meshes's vector length? Also what happened to your idea of returning the pointers of the DFPointCLouds that couldn't be asociated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The clusters are taken as reference, as you suggested, so no need to return them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad! Thanks for the clarification
/** @brief Associates point cloud segments to mesh faces. It uses the center of mass of the segments and the mesh faces to find correspondances. For each mesh face it then iteratively associate the points of the segment that are actually on the mesh face. | ||
* @param referenceMesh the vector of mesh faces to associate with the segments | ||
* @param clusters the vector of clusters from cilantro to associate with the mesh faces of the reference mesh | ||
* @param associationThreshold the threshold to consider the points of a segment and a mesh face as associable. The lower the number, the more strict the association will be and some poinnts on the mesh face might be wrongfully excluded. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a distance or angle what kind of value? Maybe you could add a distance but also normal threshold?
Might be worthy specifying it in the name and description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new description is: @param associationThreshold the threshold to consider the points of a segment and a mesh face as associable. It is the ratio between the surface of the closest mesh triangle and the sum of the areas of the three triangles that form the rest of the pyramid described by the mesh triangle and the point we want to associate or not. The lower the number, the more strict the association will be and some poinnts on the mesh face might be wrongfully excluded.
* @param associationThreshold the threshold to consider the points of a segment and a mesh face as associable. The lower the number, the more strict the association will be and some poinnts on the mesh face might be wrongfully excluded. | ||
* @return std::shared_ptr<geometry::DFPointCloud> The unified segments | ||
*/ | ||
static std::shared_ptr<geometry::DFPointCloud> DFSegmentation::AssociateClusters( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that we are trying to keep all very decoupled because of Grasshopper, but I still do not consider it as a "segmentation method". It's more of a "utility" or "segmentation refinement". Maybe we can add it in a section
public: ///< main segmentation methods
static std::vector<std::shared_ptr<geometry::DFPointCloud>> NormalBasedSegmentation(...)
public: ///< segmentation refiner
static std::shared_ptr<geometry::DFPointCloud> DFSegmentation::AssociateClustersByMeshes(...)
Also, maybe we can specify better that we associate clusters with meshes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
// Getting the normal of the mesh face | ||
Eigen::Vector3d faceNormal = face->NormalsFace[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
before running this we should make sure that meshes have normals, they might not have it. Let's just print an error message return an empty cloud and raise an error that meshes's should have normals
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually the comment was not correctly placed. the check was done a bit above. I have fixed the layout:
// Getting the normal of the mesh face
if (face->NormalsFace.size() == 0)
{
o3dFace->ComputeTriangleNormals();
face->NormalsFace.clear();
for (auto normal : o3dFace->triangle_normals_)
{
face->NormalsFace.push_back(normal.cast<double>());
}
}
Eigen::Vector3d faceNormal = face->NormalsFace[0];
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function to compute normals would be worthy to be integrated in our DFMesh object class (@DamienGilliard )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done !
|
||
// Getting the center of the mesh face | ||
Eigen::Vector3d faceCenter; | ||
open3d::geometry::OrientedBoundingBox orientedBoundingBox = o3dFace->GetMinimalOrientedBoundingBox(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a OBB for getting the centroid of a mesh face seems a bit brittle, maybe a simpler mean of vertices would do the trick?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moreover, if the o3dFace is in the x, y, or z plane, it is a OBB with volume = 0, which freaks out open3d. I have fixed this and use now the o3dFace->GetCenter() method (which I should have done from the beginning)
// iterate through the segments to find the closest ones compared to the face center taking the normals into account | ||
Eigen::Vector3d segmentCenter; | ||
Eigen::Vector3d segmentNormal; | ||
double faceDistance = std::numeric_limits<double>::max(); | ||
for (auto segment : clusters) | ||
{ | ||
for (auto point : segment->Points) | ||
{ | ||
segmentCenter += point; | ||
} | ||
segmentCenter /= segment->GetNumPoints(); | ||
|
||
for (auto normal : segment->Normals) | ||
{ | ||
segmentNormal += normal; | ||
} | ||
segmentNormal.normalize(); | ||
|
||
double currentDistance = (faceCenter - segmentCenter).norm() / std::abs(segmentNormal.dot(faceNormal)); | ||
// if the distance is smaller than the previous one, update the distance and the corresponding segment | ||
if (currentDistance < faceDistance) | ||
{ | ||
correspondingSegment = segment; | ||
faceDistance = currentDistance; | ||
} | ||
} | ||
|
||
// get the triangles of the face. | ||
std::vector<Eigen::Vector3i> faceTriangles = o3dFace->triangles_; | ||
|
||
for (Eigen::Vector3d point : correspondingSegment->Points) | ||
{ | ||
bool pointInFace = false; | ||
/* | ||
To check if the point is in the face, we take into account all the triangles forming the face. | ||
We calculate the area of each triangle, then check if the sum of the areas of the tree triangles | ||
formed by two of the points of the referencr triangle and our point is equal to the reference triangle area |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a thought here but in a future refinement having a ktree or some sort of structure for quick queries would highly improove this instead of looping through the cloud's points ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I aggree.
btw, the results are starting to look fine! 🥇 |
|
… for cluster to mesh face association
@9and3 I believe this PR is ready to be reviewed and merged if it's ok ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
In this branch we develop the backend of df cpp library to allow the model-based semantic segmentation.