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

Add GetSurfaceArea() for t::TriangleMesh #6248

Merged
merged 6 commits into from
Dec 12, 2023

Conversation

shanagr
Copy link
Contributor

@shanagr shanagr commented Jul 4, 2023

Type

  • Bug fix (non-breaking change which fixes an issue): Fixes #
  • New feature (non-breaking change which adds functionality). Resolves #
  • Breaking change (fix or feature that would cause existing functionality to not work as expected) Resolves #

Motivation and Context

This is my first/test PR. I am just getting familiar with Open3D and was told by maintainers that t::TriangleMesh doesn't have a lot of the functionality provided in legacy TriangleMesh, and Open3D is in the process of porting these. I found the old TriangleMesh had a GetSurfaceArea() function and tried to implement it in t::TriangleMesh. I'm looking for feedback mostly.

Checklist:

  • I have run python util/check_style.py --apply to apply Open3D code style
    to my code.
  • This PR changes Open3D behavior or adds new functionality.
    • Both C++ (Doxygen) and Python (Sphinx / Google style) documentation is
      updated accordingly.
    • I have added or updated C++ and / or Python unit tests OR included test
      results
      (e.g. screenshots or numbers) here.
  • I will follow up and update the code if CI fails.
  • For fork PRs, I have selected Allow edits from maintainers.

Description


This change is Reviewable

@update-docs
Copy link

update-docs bot commented Jul 4, 2023

Thanks for submitting this pull request! The maintainers of this repository would appreciate if you could update the CHANGELOG.md based on your changes.

@ssheorey
Copy link
Member

ssheorey commented Jul 5, 2023

Hi @shanagr thanks for adding this! Please add a C++ unit test and Python bindings. See the corresponding unit test and python bindings for the Eigen based TriangleMesh class for inspiration.

@reyanshsolis
Copy link
Collaborator

Direct problem in this - You can't iterate on tensor ptr as you have done for non-contiguous tensor, so making them contiguous by adding .Contiguous will solve that part.

However we want to avoid using custom kernels as much as we can, as it creates future maintenance issues. Ideally we want to use Tensor operations for everything and the Tensor implementation handles all the contiguous, device, etc. I agree adding kernels offers faster implementation, but it disconnects it benefiting from improvements in tensor implementation. If you use Tensor operations, the improvement in tensor ops over the time will translate to overall lib. improvement.

For this particular case such as this PR, where one needs to iterate over each element and apply operation, we need to think if we can add a Tensor operation like apply_transform to do this. Will think about this when I get some bandwidth, meanwhile if you have any idea, please let me know.

@shanagr
Copy link
Contributor Author

shanagr commented Aug 30, 2023

@reyanshsolis thanks for the comment. I will try to digest it and make changes accordingly.

@ssheroy I haven't had the time the past few weeks to look at the Python bindings. Hopefully I will have more time going forward. I will look into this over the weekend.

@benjaminum
Copy link
Contributor

Looking at this PR, it seems only Python bindings are missing and it must be ensured that the data is contiguous.

@reyanshsolis Do you think it makes sense to merge the current implementation? It brings one more function to the new Tensor API. Besides, we don't have an apply_transform ready and there is a similar kernel for the cross product in o3d already.

@shanagr Do you think you could add the missing parts and let us know if you need help?

Copy link
Contributor

@benjaminum benjaminum left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 7 files reviewed, all discussions resolved (waiting on @reyanshsolis)

@ssheorey ssheorey merged commit 663d698 into isl-org:main Dec 12, 2023
36 checks passed
@shanagr
Copy link
Contributor Author

shanagr commented Mar 4, 2024

@benjaminum Thanks for filling in the missing pieces. I have not been as consistent as I hoped, but I'm giving a second shot to being a regular contributor.
@reyanshsolis I want to revisit what you said about avoiding custom kernels and using tensor operations whenever possible. Where can I see a list of available tensor operations? Once I see existing functions, it might be easier for me to implement something like apply_transform.

@benjaminum
Copy link
Contributor

@shanagr No worries, thanks for your contribution

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.

4 participants