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

Unprojection function in the ViewControl #2852

Merged
merged 5 commits into from
Feb 17, 2023

Conversation

PieroV
Copy link
Contributor

@PieroV PieroV commented Jan 4, 2021

Hi!

I added a function to unproject 2D points to obtain a ray from the camera eye containing the 2D point.
It is useful to perform raycasting queries with clicks on the visualizer.
Also, since this operation is const, I added a const overload to obtain the ViewControl from the visualizer.

The function is aware that the camera may be orthogonal rather than perspective.
For these cases, the origin is in a plane that has look_at - eye_ as normal, and in which eye_ lies.

However, because of the overload, I had to modify the python bindings: I added a lambda instead of a cast, as from what I understand that is what you usually do in these cases. It compiled, but I could not create the pip package, so I could not test that everything still worked in Python.


This change is Reviewable

This function can be used to obtain a ray to perform raycasting queries
on the visualizer.
Also, since this operation is const, I added a const overload to obtain
the ViewControl from the visualizer.
@update-docs
Copy link

update-docs bot commented Jan 4, 2021

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

PieroV added 2 commits January 4, 2021 16:42
Previously, I ignored a format error, as it seemed to me that my code
was coherent with the other code, but the PR failed, so I now I
applied the style fix anyway.
@PieroV
Copy link
Contributor Author

PieroV commented Jan 5, 2021

Hi,
I am having some troubles in understanding why the CI fails on Ubuntu CI arm64.
I see that pillow failed to install, because it could not find jpeg (see lines 10777-10778):

  a required dependency when compiling Pillow from source.

but then I see (line 11028)
[100%] Built target install-pip-package
So I think that the creation/installation of the Python package fails, but the tests run anyway.
The same happened in PR #2833.

@ssheorey ssheorey requested review from theNded and yxlao February 19, 2021 17:36
@ssheorey ssheorey requested review from errissa and removed request for theNded December 12, 2022 05:49
@ssheorey ssheorey added this to the v0.17 milestone Jan 20, 2023
@ssheorey ssheorey removed the request for review from yxlao January 20, 2023 23:04
Copy link
Collaborator

@errissa errissa left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 4 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @PieroV)

@PieroV
Copy link
Contributor Author

PieroV commented Feb 16, 2023

Reviewed 2 of 4 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @PieroV)

Hello!

What do you need me to do? Squash and rebase?

@ssheorey ssheorey merged commit e6a3962 into isl-org:master Feb 17, 2023
dbs4261 pushed a commit to dbs4261/Open3D that referenced this pull request Apr 13, 2023
This function can be used to obtain a ray to perform raycasting queries
on the visualizer.

Co-authored-by: Rene Sepulveda <errissa@gmail.com>
yxlao added a commit that referenced this pull request May 17, 2023
ssheorey pushed a commit that referenced this pull request Jun 12, 2023
py::return_value_policy::reference_internal)
.def(
"get_view_control",
[](Visualizer &vis) { return vis.GetViewControl(); },
Copy link
Contributor

Choose a reason for hiding this comment

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

What was the purpose of this change?

Copy link
Contributor

Choose a reason for hiding this comment

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

These changes were reverted in this PR:
https://github.com/isl-org/Open3D/pull/6116/files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I saw the comment, but then somehow forgot to reply.

The purpose of that change was to make that object available to Python (it wasn't when I wrote this PR 2 years and a half ago, IIRC).
Otherwise, you could run this code only from C++.

IIRC the lambda was used to avoid using a cast to tell the version of the function pybind should have exposed.
The PR fixes the bindings bug, but it also breaks C++ const correctness, which isn't great for when you want to use it from C++.

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