Add FOV API to OSG viewer #1048

jslee02 · 2018-04-01T14:38:08Z

Before creating a pull request

Document new methods and classes
Format new code files using clang-format

Before merging a pull request

Set version target by selecting a milestone on the right side
Summarize this change in CHANGELOG.md

codecov · 2018-04-02T13:47:46Z

Codecov Report

Merging #1048 into release-6.5 will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff              @@
##           release-6.5    #1048   +/-   ##
============================================
  Coverage        56.58%   56.58%           
============================================
  Files              314      314           
  Lines            24312    24312           
============================================
  Hits             13758    13758           
  Misses           10554    10554

mxgrey

Mostly nitpicks.

mxgrey · 2018-04-02T17:22:15Z

dart/gui/osg/Viewer.cpp

+  {
+    dtwarn << "[Viewer::getMasterCameraFieldOfView] Attemping to set vertical "
+           << "field of view while the camera isn't perspective view. "
+           << "Ignoring this request.\n";


If we say that we're ignoring the request, shouldn't we return here?

mxgrey · 2018-04-02T17:22:48Z

dart/gui/osg/Viewer.hpp

+  /// \param[in] fov Vertical field of view in degree.
+  void setVerticalFieldOfView(double fov);
+
+  /// Retruns the vertical field of view of the master camera of the view.


Retruns --> Returns

mxgrey · 2018-04-02T17:23:38Z

dart/gui/osg/Viewer.hpp

@@ -293,6 +293,15 @@ class Viewer : public osgViewer::Viewer, public dart::common::Subject
  /// Get the root ::osg::Group of this Viewer
  const ::osg::ref_ptr<::osg::Group>& getRootGroup() const;

+  /// Sets the vertical field of view of the master camera of the view.
+  /// \param[in] fov Vertical field of view in degree.


Nitpick: degree --> degrees

mxgrey · 2018-04-02T17:24:04Z

dart/gui/osg/Viewer.hpp

+  void setVerticalFieldOfView(double fov);
+
+  /// Retruns the vertical field of view of the master camera of the view.
+  /// \return Vertical field of view in degree if the camera is perspective


Nitpick: degree --> degrees

mxgrey · 2018-04-02T17:33:41Z

dart/gui/osg/Viewer.cpp

+  double zFar;
+
+  const auto* camera = getCamera();
+  assert(camera);


Does this mean we always assume the return value of getCamera() should always be non-null? Is it feasible that it will ever be nullptr (e.g. if this Viewer doesn't have any cameras)? I'm not finding anything in the OSG docs that indicate what we should expect.

Yeah, I also couldn't find related docs from OSG. I think it would be safer we just ignore when getCamera() returns null. Let me change this code so. Let me know what you think.

mxgrey · 2018-04-02T17:33:59Z

dart/gui/osg/Viewer.cpp

@@ -843,6 +843,53 @@ const ::osg::ref_ptr<::osg::Group>& Viewer::getRootGroup() const
  return mRootGroup;
 }

+//==============================================================================
+void Viewer::setVerticalFieldOfView(double fov)


Nitpick: const double

Hm, I see the reason for this suggestion; I found this SO post says specifying const modifier helps the compiler with optimizations. However, AFAIK, we haven't used const when we pass value parameters in DART codebase. Do you suggest we follow this convention from now on?

I recommend using const for every variable which isn't intentionally mutable. Besides compiler optimizations, it also catches bugs where a variable might get unintentionally modified.

Note that you don't need the const in the declaration of the function. E.g.:

// in foo.hpp void foo(int value); // in foo.cpp void foo(const int value) { /* ... do things ... */ }

works perfectly fine, because the function signature is the same, whether or not a value parameter is const-qualified. On the other hand, function signatures are different between const-references and mutable lvalue references, so this would not work:

// in foo.hpp void foo(int& value); // in foo.cpp void foo(const int& value) { /* ... do things ... */ }

To illustrate a potential bug, we might accidentally type

const bool result = camera->getProjectionMatrixAsPerspective( fov, aspectRatio, zNear, zFar);

instead of

const bool result = camera->getProjectionMatrixAsPerspective( fovy, aspectRatio, zNear, zFar);

which would accidentally overwrite the value that the caller passed to the function.

Thanks for the examples. I see the point of preventing a potential bug, but I have a little concern with that their function signatures are not "exactly" the same. Wouldn't it be more clear to just put const to both of declaration and definition in this case?

Edit: I'm fine with either way; just trying to learn. 😄

Just to be clear, the function signatures, in the technical sense, are exactly the same. A human might see that the text is different between the declaration and the definition, but the compiler/linker does not care, and the ABI signature will be identical.

There's nothing inherently wrong with putting the const in the declaration as well, although it's often discouraged because it's seen as unnecessary noise which a user has no reason to care about (it makes no difference whatsoever to a user whether a value parameter of a function is const-qualified or not, because it has no impact on how they use the function or how the function behaves).

Thanks for the explanation. That sounds reasonable to me as well. It's Good to know!

jslee02 added 2 commits March 31, 2018 23:14

Add API for field of view of OSG viewer

b2af096

Update FOV API

9d6f741

jslee02 added this to the DART 6.5.0 milestone Apr 1, 2018

jslee02 requested a review from mxgrey April 1, 2018 14:38

mxgrey reviewed Apr 2, 2018

View reviewed changes

jslee02 and others added 5 commits April 2, 2018 11:32

Address @mxgrey's comments

0fc0d21

Update changelog

039d4c3

Fix typo in changelog

d3425d4

Fix comment reflecting their function names

ed2f98c

Add const modifier to prevent possible bug

4d70ca6

mxgrey approved these changes Apr 2, 2018

View reviewed changes

jslee02 merged commit 6d7b453 into release-6.5 Apr 3, 2018

jslee02 deleted the enhance/osg_fov branch April 3, 2018 13:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FOV API to OSG viewer #1048

Add FOV API to OSG viewer #1048

jslee02 commented Apr 1, 2018 •

edited

Loading

codecov bot commented Apr 2, 2018 •

edited

Loading

mxgrey left a comment

mxgrey Apr 2, 2018

mxgrey Apr 2, 2018

mxgrey Apr 2, 2018

mxgrey Apr 2, 2018

mxgrey Apr 2, 2018

jslee02 Apr 2, 2018

mxgrey Apr 2, 2018

jslee02 Apr 2, 2018

mxgrey Apr 2, 2018

mxgrey Apr 2, 2018

jslee02 Apr 2, 2018 •

edited

Loading

mxgrey Apr 2, 2018

jslee02 Apr 2, 2018

Add FOV API to OSG viewer #1048

Add FOV API to OSG viewer #1048

Conversation

jslee02 commented Apr 1, 2018 • edited Loading

codecov bot commented Apr 2, 2018 • edited Loading

Codecov Report

mxgrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jslee02 Apr 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jslee02 commented Apr 1, 2018 •

edited

Loading

codecov bot commented Apr 2, 2018 •

edited

Loading

jslee02 Apr 2, 2018 •

edited

Loading