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

Allow arbitrary pinhole camera #2564

Merged
merged 4 commits into from
Nov 11, 2020

Conversation

pablospe
Copy link
Contributor

@pablospe pablospe commented Nov 2, 2020

Function to get view controller from arbitrary pinhole camera parameters. In this function, it is responsibility of the users to
verify the validity of the parameters, in contrast to ConvertFromPinholeCameraParameters() function. This can be useful to render images or depthmaps without any restriction in FOV, zoom, etc.

Example of use (python): https://github.com/pablospe/open3D_depthmap_example
(code is already compiled, and can be installed with pip).

This PR is related to the following git issues: #1417, #497, #727, #834, #1389, #2443, #1164, #2036, #2190.


This change is Reviewable

parameters. In this function, it is responsibility of the users to
verify the validity of the parameters, in contrast to
`ConvertFromPinholeCameraParameters()` function. This can be useful to
render images or depthmaps without any restriction in FOV, zoom, etc.
@update-docs
Copy link

update-docs bot commented Nov 2, 2020

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

@pablospe
Copy link
Contributor Author

pablospe commented Nov 2, 2020

Style check seems to fail. I am not sure about the error. Any idea?

@theNded
Copy link
Contributor

theNded commented Nov 2, 2020

Style check seems to fail. I am not sure about the error. Any idea?

Can you please try make apply-style and push again?

Style applied to the following files:
/home/pablo/MS/Open3D_pablospe_PR/cpp/pybind/visualization/viewcontrol.cpp
@pablospe
Copy link
Contributor Author

pablospe commented Nov 2, 2020

Thanks, Wei Dong! I know you from your time at CVG :)

Copy link
Contributor

@theNded theNded 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 3 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @pablospe)


cpp/open3d/visualization/visualizer/ViewControl.cpp, line 191 at r2 (raw file):

}

bool ViewControl::ConvertFromArbitraryPinholeCameraParameters(

Many lines of code are duplicates here. I guess a better practice could be adding a flag to ConvertFromPinholeCameraParameters, and change the condition checks accordingly: ViewControl::ConvertFromPinholeCameraParameters(const camera::PinholeCameraParameters & parameters, bool allow_arbitrary=false)


cpp/pybind/visualization/viewcontrol.cpp, line 74 at r2 (raw file):

            .def("convert_from_pinhole_camera_parameters",
                 &ViewControl::ConvertFromPinholeCameraParameters,
                 "parameter"_a)

As mentioned above, might be good to add "allow_arbitrary"_a directly to convert_from_pinhole_camera_parameters

@theNded
Copy link
Contributor

theNded commented Nov 2, 2020

Thanks, Wei Dong! I know you from your time at CVG :)

Haha, nice to meet you here again! :-)

@pablospe
Copy link
Contributor Author

pablospe commented Nov 2, 2020

ViewControl::ConvertFromPinholeCameraParameters(const camera::PinholeCameraParameters & parameters, bool allow_arbitrary=false)

Sounds good. I will implement the changes, it will be a few ifs which might make the function even more difficult to understand than the current version, but I can give a try. Another possibility is to split the function is smaller functions like this:
https://github.com/intel-isl/Open3D/blob/6a111774345e551d520fa367e4d2c7ff78da3196/cpp/open3d/visualization/visualizer/ViewControl.cpp#L249-L253
and so on. Some code will be repeated but it will be more modular.

What I tried was to implement ConvertFromPinholeCameraParameters using the new function ConvertFromArbitraryPinholeCameraParameters but the ProjectionType::Orthogonal part makes the logic a bit tricky.

To be sincere I don't really understand why all these restrictions are needed in the first place, I believe the reason is to control, for instance, the amount of zoom the user can make to avoid going to infinity. Sometimes I actually would wish to be able to do "more zoom" and I find these constraints a limitation (especially for headless rendering where no user interaction is envolved). I also noticed (but only in Windows) that I can't render images of bigger size than my screen resolution, so the limit for the window size might be related to GLFW. Also, for the FOV, I didn't find a good reason to have a FIELD_OF_VIEW_MIN.

@pablospe
Copy link
Contributor Author

pablospe commented Nov 3, 2020

@theNded I made the changes you requested, but I now get the following error when importing open3d in python (compilation and installation seems with make install; make install-pip-package)

>>> import open3d
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/mnt/linux_home/pablo/miniconda3/envs/open3d/lib/python3.8/site-packages/open3d/__init__.py", line 83, in <module>
    from open3d.cpu.pybind import (camera, geometry, io, pipelines, utility, t)
ImportError: SystemError: <built-in method join of str object at 0x7f84fac28fb0> returned a result with an error set
>>> 

Here the proposed code:
pablospe@ed9b60a
(since it is not working I didn't push it in this branch)

@theNded
Copy link
Contributor

theNded commented Nov 5, 2020

@theNded I made the changes you requested, but I now get the following error when importing open3d in python (compilation and installation seems with make install; make install-pip-package)

>>> import open3d
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/mnt/linux_home/pablo/miniconda3/envs/open3d/lib/python3.8/site-packages/open3d/__init__.py", line 83, in <module>
    from open3d.cpu.pybind import (camera, geometry, io, pipelines, utility, t)
ImportError: SystemError: <built-in method join of str object at 0x7f84fac28fb0> returned a result with an error set
>>> 

Here the proposed code:
pablospe@ed9b60a
(since it is not working I didn't push it in this branch)

Sorry for coming back late, I will give you an update today!

@theNded
Copy link
Contributor

theNded commented Nov 6, 2020

It was from this line. Pybind has changed its API but docstring is still there. Removing this line will fix the problem.
https://github.com/pablospe/Open3D/blob/ed9b60a4115e98a1152a35bba1592572b7b26468/cpp/pybind/visualization/viewcontrol.cpp#L111

@pablospe
Copy link
Contributor Author

pablospe commented Nov 6, 2020

It was from this line. Pybind has changed its API but docstring is still there. Removing this line will fix the problem.
https://github.com/pablospe/Open3D/blob/ed9b60a4115e98a1152a35bba1592572b7b26468/cpp/pybind/visualization/viewcontrol.cpp#L111

Totally forgot about that. Thanks a lot for taking the time to look at it, specially now it is CVPR deadline! :)

pablospe and others added 2 commits November 6, 2020 18:22
… camera parameters. This can be useful to render images or depthmaps without any restriction on window size, FOV and zoom.
@theNded
Copy link
Contributor

theNded commented Nov 7, 2020

@errissa This PR lgtm and won't interrupt current functionalities. May I merge if you think it looks fine?

@errissa
Copy link
Collaborator

errissa commented Nov 10, 2020

@theNded This looks good to merge to me.

Copy link
Contributor

@theNded theNded left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 3 files at r3, 2 of 2 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @pablospe)

@theNded theNded merged commit 75dd367 into isl-org:master Nov 11, 2020
@Zumbalamambo
Copy link

@pablospe could you please share the corresponding python wheel that fixes this issue?

@pablospe
Copy link
Contributor Author

pablospe commented Dec 1, 2020

In this example of use (python): https://github.com/pablospe/open3D_depthmap_example
(code is already compiled, and can be installed with pip)

I believe in the open3d version 0.11.3 these commits will be included. I just don't know what is the open3d roadmap and how often a new version is released. Hopefully it is released soon:
https://pypi.org/project/open3d/#history

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.

None yet

4 participants