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

Update visualization.ipynb #6263

Merged
merged 1 commit into from
Oct 24, 2023
Merged

Update visualization.ipynb #6263

merged 1 commit into from
Oct 24, 2023

Conversation

Sendeky
Copy link
Contributor

@Sendeky Sendeky commented Jul 12, 2023

LineSet does not have a "zoom" option

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

A simple correction to the documentation so that new users don't have to figure out why their "zoom" option isn't working for LineSet

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

Simple fix in the documentation. When drawing LineSet using "draw_geometries", you cannot use the "zoom" argument


This change is Reviewable

Linseed does not have a "zoom" option
@update-docs
Copy link

update-docs bot commented Jul 12, 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.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@Sendeky
Copy link
Contributor Author

Sendeky commented Jul 12, 2023

According to the bot, I should update the CHANGELOG.md, but I see that there is already a "Corrected documentation for visualisation tutorial" there. Should I edit it or is it fine?

@ssheorey
Copy link
Member

According to the bot, I should update the CHANGELOG.md, but I see that there is already a "Corrected documentation for visualisation tutorial" there. Should I edit it or is it fine?

Hi @Sendeky thanks for this PR! Don't worry about CHANGELOG - it's useful for larger PRs.

@@ -222,7 +222,7 @@
" lines=o3d.utility.Vector2iVector(lines),\n",
")\n",
"line_set.colors = o3d.utility.Vector3dVector(colors)\n",
"o3d.visualization.draw_geometries([line_set], zoom=0.8)"
"o3d.visualization.draw_geometries([line_set])"
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is not that LineSet doesnt support zoom parameter but that o3d.visualization.draw_geometries doesnt support zoom parameter when passed without front, lookat, and up params. More information here - https://stackoverflow.com/a/76828436/1874627 answered by me.

The code here docs/jupyter/visualization/visualization.ipynb however will work if run directly from open3d repository, since open3d jupyter notebook used a hack to override o3d.visualization.draw_geometries. See here:- https://github.com/isl-org/Open3D/blob/master/docs/jupyter/open3d_tutorial.py#L67

As a quick fix, I will still go for accepting this PR, as above stackoverflow question clearly shows users are copying code with zoom parameter.

Copy link
Contributor

@saurabheights saurabheights Aug 21, 2023

Choose a reason for hiding this comment

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

Alternative option - Remove https://github.com/isl-org/Open3D/blob/master/docs/jupyter/open3d_tutorial.py#L67 and use import correctly, i.e. open3d_tut.jupyter_draw_geometries. This shows a custom method is called that creates Open3D window as well as captures a screenshot of the window before closing.

IMO - draw_geometries method should be extended to include the functionality provided by https://github.com/isl-org/Open3D/blob/master/docs/jupyter/open3d_tutorial.py#L17C1-L62C25

@saurabheights saurabheights mentioned this pull request Oct 5, 2023
9 tasks
@ssheorey ssheorey merged commit 1cbf704 into isl-org:master Oct 24, 2023
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