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

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

Closed
wants to merge 1 commit into from

Conversation

hexaflexa
Copy link

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 #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)

Copy link

update-docs bot commented Feb 10, 2024

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

@errissa errissa self-requested a review February 13, 2024 14:45
@errissa
Copy link
Collaborator

errissa commented Feb 13, 2024

@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!

@hexaflexa
Copy link
Author

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.

@ssheorey ssheorey added this to the v0.19 milestone Apr 29, 2024
@hexaflexa hexaflexa closed this by deleting the head repository Sep 14, 2024
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.

3 participants