Alembic: addition of a simple logging class.
ClosedPublic

Authored by Kévin Dietrich (kevindietrich) on Mar 1 2017, 8:21 PM.

Details

Summary

The idea is to have a system where we properly log error messages and
let the users know that errors occured redirecting them to the console
for explanations. This is only implemented for the exporter since the
importer already has similar functionalities; however they shall
ultimately be unified in some way.

Diff Detail

Repository
rB Blender
Kévin Dietrich (kevindietrich) retitled this revision from to Alembic: addition of a simple logging class..Mar 1 2017, 8:21 PM
Kévin Dietrich (kevindietrich) updated this object.

I have a dislike for "util" packages/files/classes, as they often become a mixup of everything and anything. Why not create abc_logging for this, or abc_reporting?

source/blender/alembic/intern/abc_exporter.cc
423

Why the this-> everywhere? It's not necessary, especially since the m_ prefix already indicates it's a member variable.

source/blender/alembic/intern/abc_object.cc
95

Let's keep this patch to only changing the way we log, and not what we log.

source/blender/alembic/intern/abc_util.cc
545

This creates a copy of the entire stream contents, only to determine whether its length is non-zero. There must be a better way to do that.

source/blender/alembic/intern/abc_util.h
170

Document what the class is for, and whether it is thread-safe.

174

Each function should have some explanation of what it does. For example, right now there are clear() and empty(), which both could do each other's job if you just go by the name. The only thing hinting at their functionality is their constness.

Sybren A. Stüvel (sybren) requested changes to this revision.Mar 9 2017, 10:59 AM
This revision now requires changes to proceed.Mar 9 2017, 10:59 AM
Kévin Dietrich (kevindietrich) marked 5 inline comments as done.
  • Merge branch 'master' into abc_logger
  • Document SimpleLogger class.
  • Better way to tell whether or not the stream is empty.
  • Correct documentation.
  • Revert some changes, avoid using 'this' qualifier
Sybren A. Stüvel (sybren) requested changes to this revision.Mar 21 2017, 3:04 PM

Looking much better, just a few things to nag about.

source/blender/alembic/intern/abc_object.cc
94

"Boundbox" is not an English word; let's just write "Bounding box" instead.

source/blender/alembic/intern/abc_util.h
175

It would seem that different instances could be output to different streams. In what way would this cause thread-unsafety? And how is this related to thread-unsafety at the class level?

187

"Access" is quite unclear. Why not say "Returns a copy of ..."? That way the implications of calling that function are clearer.

source/blender/alembic/intern/alembic_capi.cc
307

IMO it would be better to make it explicit that we don't know the cause of the error.

This revision now requires changes to proceed.Mar 21 2017, 3:04 PM
Kévin Dietrich (kevindietrich) marked 3 inline comments as done.
  • Merge branch 'master' into abc_logger
  • Tackle review points: clarify comments and messages.
This revision is now accepted and ready to land.Apr 4 2017, 5:27 PM
This revision was automatically updated to reflect the committed changes.

One big downside that I've only just realised: the logger as it is implemented now doesn't play nice with other output, such as direct writes to std::cerr. This means that it's a one-or-nothing approach, where you either have to log everything using ABC_LOG (which is impossible from utility functions, as they don't have a reference to the logger), or you have to live with all std::err output coming before ABC_LOG output, which breaks ordering. Furthermore, it doesn't work well when Blender crashes, as then the log won't be shown at all, and a user cannot see what was done last before the crash.

I'm facing both issues now while I'm using ABC_LOG for debugging/development purposes.

I guess the solution to have the logger work in utility functions would be to have a global variable, a singleton, for the logger. Although, I would still try to keep a separation between the user level error messaging and the developer level one.