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

Modified interactor style: display camera parameters #544

Merged
merged 1 commit into from
Mar 3, 2014
Merged

Modified interactor style: display camera parameters #544

merged 1 commit into from
Mar 3, 2014

Conversation

VictorLamoine
Copy link
Contributor

This is a more user friendly way to display camera info when the user presses "C" in the viewer. It also changes the way .cam files are written when pressing "J".

Example of new output :

Clipping plane: [near, far] 0.605189, 5.31035
Focal point: [x,y,z]    0, 0, 0
Position: [x,y,z]   -0.77742, 0.395603, -2.23865
View (see doc) [x,y,z]  0.11999, -0.969652, -0.213021
Camera view angle:  0.523599 rad
Window size: [x,y]  960, 540
Window position: [x,y]  903, 249

@taketwo
Copy link
Member

taketwo commented Mar 3, 2014

It also changes the way .cam files are written when pressing "J"

Well if you change the format of '.cam' files then you have to modify the code that loads '.cam' files as well (which you did not). But apart from that, I think it is not a good idea to change the format. For example, in my personal projects I have code that relies on the existing format of the file, so your change would break it.

Summarizing, I like the update for on-screen output, but the '.cam' format should stay the same.

@VictorLamoine
Copy link
Contributor Author

Agreed, there is no need to modify the way PCL writes '.cam' files.
The pull request now only changes the way PCL show camera parameters.

@taketwo
Copy link
Member

taketwo commented Mar 3, 2014

Also what do you think about the following:

  • Radians vs. degrees: perhaps it's easier for a human to parse the latter;
  • Put rad (or deg if we decide to switch to it) inside [] in front of the number (just like other [x,y,z] stuff);
  • Is (see doc) really helpful? Which doc?

@VictorLamoine
Copy link
Contributor Author

I changed the display to degrees. It's easier to parse this way. Moreover the field of view is rarely given in radians.

I changed "view (see doc)" to "view up" to match the name in the documentation:
http://docs.pointclouds.org/1.7.1/classpcl_1_1visualization_1_1_p_c_l_visualizer.html#ab2039927ec8f5a9771202f269987ec72

Is it ok this way ?

@taketwo
Copy link
Member

taketwo commented Mar 3, 2014

Great! Please do the last two things to polish the contribution:

  • Change tabs to spaces;
  • Squash the commits in a single one so we do not pollute the history. (After squashing locally just git push --force and this pull request will be automatically updated).

@VictorLamoine
Copy link
Contributor Author

Done. Thank you for the detailed instructions !

@taketwo
Copy link
Member

taketwo commented Mar 3, 2014

Looks perfect now, thanks for the contribution!

taketwo added a commit that referenced this pull request Mar 3, 2014
Modified interactor style: display camera parameters
@taketwo taketwo merged commit 71e4498 into PointCloudLibrary:master Mar 3, 2014
@VictorLamoine VictorLamoine deleted the camera_fixes branch March 3, 2014 12:58
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.

2 participants