Skip to content

Conversation

@alberth
Copy link
Contributor

@alberth alberth commented Sep 7, 2015

While trying to understand the classes, I wrote some Doxygen comments, which may be useful for you.

@JKleinHub
Copy link

That seems to be very useful. It would be good though, if some of the developers could quickly check the comments, if they are all correct. It is easy to misunderstand things, and wrong documentation would be even worse than no documentation.

Additionally, I would vote for a somewhat 'minimalistic' comment style to avoid redundancy. Instead of documenting absolutely everything, I would only comment the interesting things. If a class is called "Logger", it is completely redundant to describe it as "A Logger class", it tells you nothing more than the name already did, but makes the documentation harder to understand (more reading for an equivalent amount of information).

But as correcting something is usually far easier than creating something, this should be a valuable start for documented code.

@xlz
Copy link
Member

xlz commented Sep 7, 2015

Good idea, but most of the comments added are trivially correct.

Two places you had wrong understanding: Buffer is just buffer, not Meta data of a data buffer, and enable_filter does not filter noise. It filters duplicate color pixels due to aspect of two cameras.

@alberth
Copy link
Contributor Author

alberth commented Sep 8, 2015

Both points fixed.

Changes:

  • double_buffer.h lines 38, 62, and 63
  • double_buffer.cpp, lines 75, 76, and 84, 85
  • registration.cpp: line 217

@JKleinHub: I agree, but the question is, what is "interesting"? Different people have different ideas about that. The only solution I see is to just do everything. It avoids "what is interesting" discussions, and takes pretty much no time at all, as trivial things are trivial to add.

(Note that by definition, an author that writes the code has a deep understanding of it, and thus finds everything trivial, otherwise he cannot write it. This is the whole problem of documentation, At that point in time, he is the only one that can write documentation real fast, yet he has no use for it at that moment, as he understand everything. Unfortunately, such knowledge is not automagically transferred to other people nor to the future himself.)

In my case, I just switched to blindly typing doxygen comments everywhere without much thinking, and only now can look again to see if I can find more structure.

@xlz: For me, a buffer is a char * address, things like length and capacity is meta data. However, it's not something to make a fuss about. As for the noise, I got that from line 232 "// check for allowed depth noise". Maybe that should be changed too?

Copy link
Contributor

Choose a reason for hiding this comment

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

By default, this function maps color pixels onto depth data, so you get a 512x424 color image. If the bigdepth buffer is provided, it also performs the inverse mapping. This means the buffer is filled with depth pixels mapped onto color data, so this gives you a 1920x1082 (yes, 1082) depth image with one blank row on the top and the bottom (i.e. (float*)(bigdepth->data)[1920] is the offset of the first color-mapped depth pixel).

@floe floe added this to the 0.1 milestone Sep 9, 2015
@alberth
Copy link
Contributor Author

alberth commented Sep 9, 2015

Changed as follows:

diff
 /**
- * Undistort/register a whole image.
+ * Map color pixels onto depth data, giving an \a registered output image.
+ * Optionally, the inverse map can also be obtained through \a bigdepth.
  * @param rgb RGB color image (1920x1080).
  * @param depth Depth image (512x424)
  * @param [out] undistorted Undistorted depth image.
- * @param [out] registered Image Color image fixed for depth.
- * @param enable_filter Whether or not to drop 'too noisy' pixels.
- * @param [out] bigdepth If not \c NULL, more accurate depth.
- * @note \a bigdepth has some type and size requirements that are blindly assumed.
+ * @param [out] registered Image color image for the depth data (512x424).
+ * @param enable_filter Use a depth buffer to remove pixels which are not visible to both camer
as.
+ * @param [out] bigdepth If not \c NULL, mapping of depth onto colors (1920x1082 'float' frame)
.
+ * @note The \a bigdepth frame has a blank top and bottom row.
  */

@xlz
Copy link
Member

xlz commented Sep 9, 2015

I think commenting would be after the restructure and be limited to public API only. Some of the trivial or uninformative comments would be avoided if they are only for internal code. C++ itself is already a verbose language. The comments do not give more information than just reading the source code. Give you an example:

/** Footer of a depth packet. */
 LIBFREENECT2_PACK(struct LIBFREENECT2_API DepthSubPacketFooter

DepthSubPacketFooter itself has as much information as Footer of a depth packet.

@alberth
Copy link
Contributor Author

alberth commented Sep 10, 2015

Comments are highlighted differently, and contain only the high level information. "Footer of a depth packet." says exactly what I need to know whether I can skip the next 13 lines to the next comment.
As such comments function as focus points to decide whether I want to read the next part of the code.

The C++ code has a lot of clutter "LIBFREENECT2_PACK(struct LIBFREENECT2_API" jumps out here due to all the upper case words, while "Depth" and Footer" are hidden in a very long word. You have to carefully read, parse the line, and then throw away the right 80% of crap, to find the words you need. Even then, maybe "Depth Sub Packet" is significant? The comment tells, the code doesn't.

So even "trivial" comments help, you sprinkle fast skipping points in the code, pointing out the significant words, guiding the reader.

For the discussion of what is trivial, I have been reading the code for a day, so sure I am in no position to make deep non-trivial additions to the documentation. On the other hand, "bigdepth" that I changed just above was completely undocumented so it was considered trivial too? While you may know what kind of frame to insert into it, it is far from trivial, you have to reverse engineer the code. The +2 lines is not easy to spot. The fact that there is supposed to be 1 blank line above and below it in the result isn't explained either. I would need an hour or so at least and still wonder whether that first blank line is supposed to happen, or whether it is a co-incidence. You can write that line of text in 5-10 seconds. I can read that line in 5 seconds, understand what it says, be sure I have the right solution (for as far as I trust the documentation), and do one hour more useful stuff (eg make a contribution), but I need that line of text for that.

You can read code, but it's always slower than reading a few lines of text. (Even for you in a year from now, do you remember all the little details of all code you ever wrote? I don't think so, at least I don't.) In the code, you may miss crucial details, and you're never sure your analysis of the code was deep enough to find all the edge cases that may occur. The correct solution means a working solution, but a working solution does not imply correctness.

In the end, documentation is about your readers, not about you. Just like you make a report or paper easy to read, documentation makes code more accessible for your readers. The main difference is that code also happens to be readable by a machine.

If you don't want to explain how nice the solution is that you implemented in an accessible way and instead have your readers struggle for days through all the little details of each line of unknown code, I think it's a missed opportunity for you, and a big waste of precious time at global level in the open source world.

But having said that, I needed to make the documentation to understand the structure, and find crucial details that I missed the first two passes. I offer my work to the project. It's up to you whether you find it useful enough to add.

@floe
Copy link
Contributor

floe commented Sep 10, 2015

Well, I think it's absolutely useful and will merge it soon, after #368 - you may need to re-merge your PR, but it shouldn't require a lot of changes.
As for the "trivial" comments, I'd rather prefer to have them in, especially as they also make the Doxygen output easier to read IMHO.

@alberth
Copy link
Contributor Author

alberth commented Sep 10, 2015

Rebased to d855d25 (merge of #368)

floe added a commit that referenced this pull request Sep 11, 2015
Add: Doxygen documentation comment for many of the classes.
@floe floe merged commit 76367d9 into OpenKinect:master Sep 11, 2015
@alberth alberth deleted the add_doxydocs branch September 14, 2015 07:07
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