Re: [boost] A preliminary proposal: AGG project

From: Jens Maurer (Jens.Maurer_at_[hidden])
Date: 2002-05-01 12:02:09

Maxim Shemanarev wrote:
> Since I received some positive responses about AGG
> http://lists.boost.org/MailArchives/boost/msg28757.php , I'd like to make a
> proposal for preliminary (not formal) discussion of the library. 

(It might have helped if, in your announcement, you would have put
a small paragraph of explanation what the library is about and
where to download it from, instead of relying on people to be
on-line while reading e-mail and clicking through the links.)

I've downloaded agg-2002-04-21.zip from www.antigrain.com.  My
platform is Linux, with a variety of compilers.
Here are a few random comments:

General
=======

 - The distribution .zip should contain the documentation so
that people can read it offline.

 - For a 2D vector graphics library, portability is a
major concern, since underlying graphics primitives vary
a lot by platform.  I'd certainly like to see an adaptation
to X11, first because I don't use Win32, and second because
it allows to check whether portability concerns have been
addressed successfully in the AGG design.

 - You ought to fix the links on your docs web page.
The documentation was mostly inaccessible.  The doxygen
documentation does not appear to have any explanation
in it.

Design
======

 - It appears your library uses RGB (and Alpha) as the
color scheme.  What about other color schemes such as YUV?
If there's really only one color scheme in the 2D vector
graphics application domain, I'd be surprised, i.e.
it should be documented, with rationale.

 - I don't understand why your constructors are
often relatively empty, and then you provide an init()
or create() member function (see, for example, agg::ellipse).
I could envision performance reasons to provide an init()
function *in addition* to a full-fledged constructor (that
avoids tearing down the object).

 - The STL often seems to avoid using set_ and get_ prefixes
for member functions, so poly.thickness(5) seems to be
more in line with this than poly.set_thickness(5).  In particular
since there does not appear to be a poly.get_thickness().
(see iostreams, for example).

 - A canvas appears to be named render_bgr24_solid.  I've so
far only seen "rgb", never the reverse, even though bits
might probably be stored in "bgr" and not "rgb" order.

 - An interface using "reset_iterator" and "next_vertex"
is exactly not STL-like, because it does not allow two
iterators on the same underlying container-like object.
The STL uses separate iterator classes and a begin() / end()
interface on the container, and operator++() on the iterator.
This allows dealing with subranges.

Filenames
=========

 - At boost, we tend to use subdirectories instead of prefixes
for header filenames.  We haven't had trouble with that so
far, as long as all includes (even internal sub-includes)
use the full relative path, e.g. (in your case) agg/something.h
I believe that using subdirectories helps organization.

 - There is no need to have filename prefixes in .cpp
implementation files.

Examples
========

 - It is unfortunate that all examples appear to be
Win32-only, and MSVC wizard-generated also.  There does not
seem to be a "Makefile" or other means around that would
allow users of Borland, Intel or Metrowerks compilers to
compile the examples.  I suggest using only bare-bones
features of the Win32 native library (no fancy AFX lib)
to open a window, and then pass control to the 2D library
to show its features (probably a controlled "quit" from
the example app needs a little event handling from the
underlying platform also).

It would show the portability of your library if several
target platforms such as Win32 and X11 could share the 2D
implementation, with only minimal surrounding platform-dependent
boilerplate.  Currently, the inherently Win32-specific OnDraw()
member function seems to have all the 2D code within it.
See  http://www.boost.org/more/imp_vars.htm.

- Your example code occasionally uses home-grown forward
declarations (IdeaView.h), which seems questionable design
to me.  If such forward declarations are indeed useful, there
should be a library header provided that has these.
   namespace agg
   {
       class render_bgr24_solid;
   }

 - The class pixel_map isn't even in the doxygen documentation.

 - You don't need to use the agg:: prefix for namespace-scope
functions when one of their arguments is in that namespace
(called "Koenig" or argument-dependent lookup).

Implementation
==============

 - Using the "register" keyword is redundant with today's
compilers.  I know of none that honors it any more.

 -     void render_bgr24_solid::render(scanline* sl)
uses a pointer, although the implementation clearly assumes
that "sl" cannot be 0.  Wouldn't a reference be more
appropriate here?

 - At that same function, calculations using >> 16 etc.
appear to assume a certain bit width for built-in
datatypes such as "char".  Are you certain that you can
deal with 32bit "char"s here?

 - Using Comeau C++, there were some warnings such as
these:

"../include/agg_scanline.h", line 91: warning: type qualifier on return type
          is meaningless
          const int      get_x()      const { return m_clipped_x; }

"agg_polyfill.cpp", line 696: warning: extra ";" ignored
      };

"../include/agg_conv_polyline.h", line 228: warning: statement is unreachable
          return flag;
          ^

"../include/agg_gamma_ctrl.h", line 147: error: identifier "polyfill" is
          undefined
          polyfill pf;
          ^

Also, you should run your code through gcc with -Weffc++, which
shows a few uninitialized members in member-initializer-lists.
(gcc is available on Win32 by means of Cygnus, see www.cygnus.com).

I won't look at the library any further until at least
some documentation is available.

Jens Maurer