Reformat all Imath header comments to doxygen style #81

cary-ilm · 2021-01-28T02:44:06Z

This is a large rewrite of all headers to reformat into doxygen style. Not yet final, still a work in progress so please review the style.

All members, methods, and functions are now documented, with the exception of half.
Some methods moved to new locations for better logical grouping.

The readthedocs output is here for work-in-progress reviewing:

https://cary-ilm-imath.readthedocs.io/en/latest/index.html

Some things still don't work:

I can't get sphinx to recognize certain functions when there are multiple functions with different signatures.
Not all functions are referenced in the rst docs yet.
Top-level information still needs some work

Signed-off-by: Cary Phillips <[email protected]>

- All methods now have comments - Some methods have been rearranged to fit into better logical groups Still to do: - Sphinx reports errors on some functions - half not yet commented Signed-off-by: Cary Phillips <[email protected]>

lgritz · 2021-01-28T22:43:35Z

I'll try to look this over tonight and give more detailed comments

meshula

That's a mighty diff! It looks good, I didn't spot anything needing a change.

lgritz · 2021-01-29T23:33:07Z

src/Imath/ImathEuler.h

+        //       ||||
+        //       VVVV
+        ///      ABCD
+        // Legend: 


this is not going to be rendered in the docs in a way that makes any sense

Yeah, the hex values don't show up, so I intended to leave these for just the header. The /// before the ABCD was a typo.

src/Imath/ImathEuler.h

lgritz · 2021-01-29T23:41:34Z

src/Imath/ImathFrame.h

+/// @file  ImathFrame.h
+///
+/// @brief Functions for computing reference frames.
+///
+/// These methods compute a set of reference frames, defined by their
+/// transformation matrix, along a curve. It is designed so that the
+/// array of points and the array of matrices used to fetch these
+/// routines don't need to be ordered as the curve.
+///


These comments will be lost. When you do a pure Doxygen run (to html), you end up with a "file by file" index (as well as class by class), and this stuff from @file will go to introduce each file. But the way you've arranged the docs through breathe and sphinx (which I approve), you are making class by class documentation, so the Doxygen comments on the files themselves will never show up anywhere.

I recommend transferring these @file comments to appropriate parts of the docs for the class themselves, or for just a collection of functions (not in a class), use a "group" appropriately to cluster functions and then these can become the group comments.

I removed the @file/@brief directives and left the introductory comments in non-doxygen style for the reader of the headers, and moved info to the appropriate to the class/functions as wel.

src/Imath/ImathFrustumTest.h

lgritz · 2021-01-29T23:47:41Z

src/Imath/ImathInt64.h

+/// Int64 - unsigned 64-bit integer
 typedef unsigned __int64 Int64;
+/// SInt64 - signed 64-bit integer
 typedef __int64 SInt64;
 #elif ULONG_MAX == 18446744073709551615LU
+/// Int64 - unsigned 64-bit integer
 typedef long unsigned int Int64;
+/// SInt64 - signed 64-bit integer
 typedef long int SInt64;
 #else
+/// Int64 - unsigned 64-bit integer
 typedef long long unsigned int Int64;
+/// SInt64 - signed 64-bit integer
 typedef long long int SInt64;
 #endif


Aside -- I can't quite remember, why do we still have these instead of using stdint?

This is worth a bigger discussion, can we eliminate this type declaration altogether? At the very least, the names are inconsistent and confusing. But they're used throughout OpenEXR.

lgritz · 2021-01-29T23:49:34Z

src/Imath/ImathNamespace.h

+/// @file  ImathNamespace.h
+///
+/// @brief The Imath library namespace
+///
+/// The purpose of this file is to make it possible to specify an
+/// IMATH_INTERNAL_NAMESPACE as a preprocessor definition and have all of the
+/// Imath symbols defined within that namespace rather than the standard
+/// Imath namespace.  Those symbols are made available to client code through
+/// the IMATH_NAMESPACE in addition to the IMATH_INTERNAL_NAMESPACE.
+///
+/// To ensure source code compatibility, the IMATH_NAMESPACE defaults to Imath


I think these are fine for documentation in the header itself for human readers, but none of the @file stuff will currently show up in your readthedocs.

Another strategy might be to put these introductory materials right into the .rst itself.

Converted to non-doxygen style.

It was fine as it was, it doesn't hurt anything to be doxygen style. I just wanted to be sure you knew that those weren't ending up anywhere in the final docs.

lgritz

I made a number of comments. They are not comprehensive -- I just noted one instance of I saw for each class of issues I noticed. They are more food for thought than hard requests for change.

I'm fine with merging this as-is and we can always improve it over time to turn it into beautiful docs.

lgritz · 2021-01-29T23:53:38Z

Some things seem to have broken in the rearrangement and the CI tests are failing.

lgritz · 2021-01-30T00:25:18Z

Thought:

The top level organization of these html docs is simply a recitation of the classes and functions, one chapter per class or function group. Very Doxygen like, very flat. It's ok as a html rendering of the header files for reference -- if you know which method of which class you are interested in, you can look it up. But it has no "affordances" for one to treat it as expository documentation, for example, it's nothing like a book from which one can learn about Imath in a logical fashion.

Maybe a better arrangement would be to actually organize chapters by major topic, each of which may end up including multiple classes and function groups, as well as introductory discussion of the topic.

I might be inclined to break the docs into the following top-level chapters:

Introduction and history of Imath (the "why" of this library and how to use it)
Half-precision floating point numbers
Basic linear algebra for computer graphics: Vectors and matrices
Colors and color operations
Math helper functions (including root finding, random numbers, interval math, etc.)
More spatial helpers (Box, Line, Plane, Frustum, quaternions, euler angles, etc.)

Breathe/sphinx ignores @file doxygen comments, so they're now straight C++ comments intended for the reader of the header file. Where appropriate, relevent details are covered in the class/function doxygen comments. Also restores some comments lost in the translation, and fixes some typos. Signed-off-by: Cary Phillips <[email protected]>

cary-ilm · 2021-01-31T21:04:02Z

The CI breakage was from a "constexpr" that should have been IMATH_CONSTEXPR.

I like the suggestion for high level "book-style" comments, but can be a project for another day.

Signed-off-by: Cary Phillips <[email protected]>

cary-ilm · 2021-01-31T21:45:39Z

The SonarCloud analysis is failed with "You're not authorized to run analysis", does anyone know what's up?

lgritz

LGTM

Fine merge now and take our time to mull over any further changes for later possible introduction.

cary-ilm added 2 commits January 13, 2021 10:03

Doxygen-style comments

dbaf0f8

Signed-off-by: Cary Phillips <[email protected]>

Rewrite header comments in doxygen format

89848ca

- All methods now have comments - Some methods have been rearranged to fit into better logical groups Still to do: - Sphinx reports errors on some functions - half not yet commented Signed-off-by: Cary Phillips <[email protected]>

meshula approved these changes Jan 29, 2021

View reviewed changes

lgritz reviewed Jan 29, 2021

View reviewed changes

src/Imath/ImathEuler.h Show resolved Hide resolved

lgritz reviewed Jan 29, 2021

View reviewed changes

src/Imath/ImathFrustumTest.h Show resolved Hide resolved

lgritz reviewed Jan 29, 2021

View reviewed changes

lgritz approved these changes Jan 29, 2021

View reviewed changes

Typo in ImathQuat.h: IMATH_CONSTEXPR14

af8fb45

Signed-off-by: Cary Phillips <[email protected]>

Merge branch 'master' into doxygen

0012118

lgritz approved these changes Jan 31, 2021

View reviewed changes

cary-ilm merged commit 162afe5 into AcademySoftwareFoundation:master Feb 1, 2021

cary-ilm added v3.0.0-beta documentation Improvements or additions to documentation labels Mar 17, 2021

cary-ilm deleted the doxygen branch May 7, 2021 01:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reformat all Imath header comments to doxygen style #81

Reformat all Imath header comments to doxygen style #81

cary-ilm commented Jan 28, 2021

lgritz commented Jan 28, 2021

meshula left a comment

lgritz Jan 29, 2021

cary-ilm Jan 31, 2021

lgritz Jan 29, 2021

cary-ilm Jan 31, 2021

lgritz Jan 29, 2021

cary-ilm Jan 31, 2021

lgritz Jan 29, 2021

cary-ilm Jan 31, 2021

lgritz Jan 31, 2021

lgritz left a comment

lgritz commented Jan 29, 2021

lgritz commented Jan 30, 2021

cary-ilm commented Jan 31, 2021

cary-ilm commented Jan 31, 2021

lgritz left a comment

Reformat all Imath header comments to doxygen style #81

Reformat all Imath header comments to doxygen style #81

Conversation

cary-ilm commented Jan 28, 2021

lgritz commented Jan 28, 2021

meshula 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

lgritz left a comment

Choose a reason for hiding this comment

lgritz commented Jan 29, 2021

lgritz commented Jan 30, 2021

cary-ilm commented Jan 31, 2021

cary-ilm commented Jan 31, 2021

lgritz left a comment

Choose a reason for hiding this comment