Open3D icon indicating copy to clipboard operation
Open3D copied to clipboard

fix TypeError in make_fragments.py due to change in openo3d.io.write_…

Open hexaflexa opened this issue 1 year ago • 3 comments
trafficstars

fix TypeError in make_fragments.py due to change in openo3d.io.write_point_cloud() as of version 0.18

bug fix in make_fragments.py, as described in https://github.com/isl-org/Open3D/issues/6646

  • [x ] 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

The current version (0.18) has this function definition:

open3d.io.write_point_cloud(filename, pointcloud, format='auto', write_ascii=False, compressed=False, print_progress=False)

but an older version (0.17) has a different function definition:

open3d.io.write_point_cloud(filename, pointcloud, write_ascii=False, compressed=False, print_progress=False)

Thus, in examples\python\reconstruction_system\make_fragments.py, the code o3d.io.write_point_cloud(pcd_name, pcd, False, True) throws the TypeError I reported above, since there is no format argument.

I changed the code to o3d.io.write_point_cloud(pcd_name, pcd, format='auto', write_ascii=False, compressed=False, print_progress=True) and now it runs without the TypeError

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

(sorry, this is my first ever pull request; thankfully, it is a very simple change)

hexaflexa avatar Feb 10 '24 03:02 hexaflexa

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

update-docs[bot] avatar Feb 10 '24 03:02 update-docs[bot]

@hexaflexa Thank you for this contribution! ~~Please apply the style as described in the PR checklist so the PR passes CI. Also, would you please check for other uses of write_point_cloud in examples and verify that the one make_fragments.py is the only one that needed to be fixed? Thanks.~~

@hexaflexa We just discussed this PR internally and we think that the real issue is that the new format parameter should have been placed at the end of the parameter list in order to maintain compatibility with existing code. This PR fixes the broken Open3D sample but we are worried about user code that the placement of the format parameter will break.

Would you be willing to change the signature of write_point_cloud in this PR instead of changing the example? You would only need to change the pybind for it here: https://github.com/isl-org/Open3D/blob/main/cpp/pybind/io/class_io.cpp#L205.

Thanks!

errissa avatar Feb 13 '24 14:02 errissa

Hi @errissa,

I am not an expert in the Open3D code (only discovered it a few days ago). But would changing the python signature of write_point_cloud make it inconsistent with the order in the C++ signatures for ReadPointCloud, ReadPointCloudOption, WritePointCloud, and WritePointCloudOption, where format comes before the other boolean options?

And then the pybind for read_point_cloud would also have to be changed for consistency with write_point_cloud, but both would be different from the C++ order.

hexaflexa avatar Feb 14 '24 04:02 hexaflexa