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

Pointcloud to voxelgrid conversion #361

Conversation

JerryJiehanWang
Copy link
Collaborator

Pointcloud to voxelgrid conversion


__all__ = ['pointclouds_to_voxelgrids']

def pointclouds_to_voxelgrids(pointclouds, resolution, return_sparse=False):
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't we adopt the same interface than trianglemesh_to_voxelgrids ? https://github.com/NVIDIAGameWorks/kaolin/blob/master/kaolin/ops/conversions/trianglemesh.py#L20

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean we should also add origin and scale as arguments?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added, as well as the tests.


__all__ = ['pointclouds_to_voxelgrids']

def _points_to_voxelgrids(points, resolution, return_sparse=False):
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe add the prefix _base like for: https://github.com/NVIDIAGameWorks/kaolin/blob/master/kaolin/ops/mesh/trianglemesh.py#L26

Also maybe we can add some details about the behavior? (i.e: For what I understand, it only convert points from 0 to 1)

def _base_points_to_voxelgrids(points, resolution, return_sparse=False):
r"""Converts points to voxelgrids. This is the base function for both trianglemeshes_to_voxelgrids
and pointclouds_to_voxelgrids. For point cloud, the points are the original points. For Triangle Mesh,
the points are sampled along the edges. The points are normalized into range [0, 1].
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure we should reference mesh behavior, the behavior that you described here ("For Triangle Meshes, the points are samples along the edges") has nothing to do with this function, isn't it? If you changed the way you sampled points along the edges this function would still be the same the description would be false, which is difficult to maintain.

Also, are the points normalized? It seemed to be that only points in the range [0, 1] (before doing anything to them) were sampled.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, the points are normalized. So will it be sufficient to say that the function converts normalized points in the range [0, 1] to voxelgrid?

Copy link
Collaborator

Choose a reason for hiding this comment

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

My understanding is that the point cloud MAY not be normalized, but regardless of that this base function will ONLY voxelize points that are in the range [0, 1]. Am I correct?

Signed-off-by: jiehanw <[email protected]>

finish pointclouds to voxelgrid

Signed-off-by: jiehanw <[email protected]>

fix according to review

Signed-off-by: jiehanw <[email protected]>
Signed-off-by: jiehanw <[email protected]>
Signed-off-by: jiehanw <[email protected]>
@JerryJiehanWang JerryJiehanWang force-pushed the jiehanw/pointclout_to_voxelgrid branch from d9fc4f5 to 6e00353 Compare April 8, 2021 14:18
Signed-off-by: jiehanw <[email protected]>
Copy link
Collaborator

@Caenorst Caenorst left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

@Caenorst Caenorst merged commit 9a9999a into NVIDIAGameWorks:master Apr 8, 2021
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.

2 participants