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 visualization/draw ICP example and add warnings #6933

Merged
merged 4 commits into from
Sep 18, 2024

Conversation

rxba
Copy link
Contributor

@rxba rxba commented Aug 26, 2024

Type

  • Bug fix (non-breaking change which fixes an issue): Fixes ICP registration crashes in visualization/draw #6887
  • 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

A minor issue, but the ICP example in examples/python/visualization/draw.py requires the user to select point correspondences for a preliminary rough transformation before ICP registration, but users may not be aware of this. Currently, if no point correspondences are selected, the application crashes with an IndexError.

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

Here are the changes I've made:

  • prevented the IndexError when running the ICP example without point correspondences and added a RuntimeWarning for the user to hightlight the need to select point correspondences.
  • additionally prevented a KeyError when the "Source (yellow)" cloud is not contained in the first selection set and added a RuntimeWarning for this.
  • refactored the existing do_ICP_one_set and do_ICP_two_set with new functions _prep_correspondences and _do_ICP to reduce duplicate code.

Copy link

update-docs bot commented Aug 26, 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.

@ssheorey ssheorey self-requested a review September 4, 2024 05:33
Copy link
Member

@ssheorey ssheorey left a comment

Choose a reason for hiding this comment

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

Thanks for this much needed bug fix! Looks good after adding one more check (see below).

examples/python/visualization/draw.py Show resolved Hide resolved
@rxba
Copy link
Contributor Author

rxba commented Sep 5, 2024

Thank you for the feedback! I've added your suggested check, and also found two more things that needed checking to avoid a crash, which I also added:

  • add check if only one set provided to two_set ICP
  • add check if no Target (blue) points are provided to single set ICP
  • add check if overall number of corresponding points is mismatching and report the mismatch so that the user can add to the existing sets

@rxba rxba requested a review from ssheorey September 5, 2024 17:51
Copy link
Member

@ssheorey ssheorey left a comment

Choose a reason for hiding this comment

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

Thanks @rxba ! Looks good.

@ssheorey ssheorey merged commit 85981ff into isl-org:main Sep 18, 2024
35 of 39 checks passed
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.

ICP registration crashes in visualization/draw
2 participants