Design issue/code review

[daviddoria]

I have been working with this code for quite a long time now, and I continually refactor and refactor and just keep running into different forms of the same problem. In the last couple of weeks I've tried to take a step back and really think about the design, motivated by the fact that the code was nearly impossible to write tests for (I read a book on TDD :) ). Below is the design I've converged on.

The program's job is to solve an image processing problem - the high level outline (hopefully from bottom to top) of the part of the design I'm interested in is:

- A Patch objects that define a square region in an image

- A PatchPair class that holds two patches to be compared

- A PixelDifference class that has a float Difference(PixelType, PixelType) function.

- A template PatchDifference<typename TPixelDifference> class that sums the difference corresponding pixels in a PatchPair using the provided PixelDifference class.

- A PairDifferences class that stores a collection of different difference types. Each PairPair has a PairDifferences object.

- A template SelfPatchCompare<typename TPatchDifference> that compares a specified patch to all other patches in an image

- A PatchBasedInpainting class. This is the object that would be instantiated from main(). The logic of this class is not the concern of the part of the design I'm worried about. The one part that it does involve is that at some point it needs to use a SelfPatchCompare<typename TPatchDifference> to compute the differences between a specific patch and all others.

The main problem is that at run time I'd like to specify which types of differences are computed. This is done at the very highest level of the program, and somehow has to get into the SelfPatchCompare class. Since SelfPatchCompare is templated on the type of difference to compute, I could either:

1) Subclass from a non-templated SelfPatchCompareSuperclass so I can store objects of type SelfPatchCompare<T>* in a vector<SelfPatchCompareSuperclass*> and instantiate each with a different TPatchDifference. This would work, but then each SelfPatchCompare object would have it's PatchPairs each containing only one difference. I'd then have to combine all of these PatchPair objects.

or

2) Provide a list of functions (via boost::function or similar) to the SelfPatchCompare object and loop over these functions in the main Compute function. This is what I did for a while, but the symantics are annoyingly complicated.

Would you prefer one of these over the other? Are there any other choices?

If you're interested, the code is here: https://github.com/daviddoria/PatchBasedInpainting

I can certainly clarify anything I explained poorly, or provide more details where needed. I know this is a pretty big problem, but it's tough coding something this big with an army of one and no collaboration :)

Thanks for any input!

David

[mike_2000_17]

Wow, this is a bit messy, to say the least.

Your problem is with the mixed responsibilities more than any other issue (templates, non-templates, base-classes, etc., these are all just details). From my understanding, the overall purpose is to perform transformations which are characterised by the following:
- Input
- Output
- Map
- Iterator
- Fold

For example, taking the difference between two patches means that you need a way to iterate through the pixels of the patches, then map the two pixel values to some difference value, and finally fold all those differences somehow to obtain a single output that represents the overall difference between the patches.

Your SelfPatchCompare thing has a similar pattern, it takes a patch and an image as input, needs a way to iterate through the patches of the image, then map the patch-pairs to some difference value (patch-difference), and then fold those differences into a single overall difference value.

What I'm trying to emphasise here is that you shouldn't be asking yourself where to put what data or which inheritance-scheme makes more sense, but you should ask what is the functional skeleton of your problem and where are its knots (or joints, or atomic elements). This is how you get to modularity, and once that is done, static vs. dynamic or how to shape the inheritance tree are usually self-evident and trivial to solve with a little programming know-how.

One concrete problem with your design is this idea that somehow the "difference" method should be a part of the "patch-pair" or "self-patch-compare" classes. PatchPair defines the input data for a binary mapping of some kind (e.g. PatchPair -> float (difference), or PatchPair -> Patch). The SelfPatchCompare is basically a folding method (iterates over a set of patches, and then combines all the individual results into one (fold or "accumulator")). This issue is basically what is causing the main problem you stated, and my answer is that both solutions you proposed are bad, and my answer to your problem is to avoid this awkward mix of responsibilities.

Another concrete problem with your code is that you are over-restricting yourself by postulating that all your code can and will do is "compute differences" of patches or whatever. Computing differences between pixels / patches / images / other is no different, in process, than computing any other accumulated quantity (average, std-dev, etc.). This is a very clear case of over-restricting the scope of your design, which leads to mixing responsibilities without even being aware of it, which leads to an inflexible design. You need to look at the problem from the most generic point-of-view first, and then make conscious choices about what assumptions are acceptable such that you get the flexibility you need, the applicability you need, and don't have to make too crazy-generic code that solves every problem of the universe. I don't think you did that thought-exercise on your design, because it shows.

If I want to compute the average pixel value of an image, what do I do? I iterate through the pixels, keep an accumulated average pixel value that is updated by each pixel traversed, and that's it. What I need for this is an "ImageIterator", some "PixelVisitor", and some "PixelAccumulator". The idea here is that the visitor follows the image-iterator by being given its visited pixels, and registers them with the accumulator.

If I want to take the "total" difference between a patch and every patch of some image, then I need to iterate through all the patches of the image, form a pair with given patch and the visited patch, feed this to some visitor which will accumulate the difference between all patches. In other words, I need a "PatchPairIterator", a "PatchPairVisitor", a "PatchCompareMap", and some "Accumulator" for the output of the patch-comparison. In the specifics, the "PatchCompareMap" (derived-class or template argument) would take the difference between the two patches, the "Accumulator" would accumulate a sum, and the "PatchPairIterator" would be a kind of patch-over-image iterator that visits every patch-pair that is formed by one constant patch and every patch of a given image.

With this type of design, the visitor sort-of acts as a buffer between the input data (the patch-pairs) and the mapping / accumulator applied (the difference operation). And this solves your stated problem.

I suggest you also take a closer look at the VTK/ITK features, because I am certain that this kind of algorithmic structure is already present in that library because these types of operations are at the core of almost every image processing task.

[daviddoria]

Mike,

First, thank you for taking the time to look at this! I really appreciate it.

I was pretty confident it was bad, I just didn't know why :)

One concrete problem with your design is this idea that somehow the "difference" method should be a part of the "patch-pair" or "self-patch-compare" classes.

You are suggesting that the differences should NOT be contained in the PatchPair, right?

PatchPair defines the input data for a binary mapping of some kind (e.g. PatchPair -> float (difference), or PatchPair -> Patch). The SelfPatchCompare is basically a folding method (iterates over a set of patches, and then combines all the individual results into one (fold or "accumulator")). This issue is basically what is causing the main problem you stated, and my answer is that both solutions you proposed are bad, and my answer to your problem is to avoid this awkward mix of responsibilities.

This certainly is the main problem. The reason I had decided to put the differences inside the PatchPair is that they are tightly coupled. That is, if I have a vector<PatchPair> and a vector<float> storing the differences, those vectors must remain identical in order and length. That is, if I did a sort on the differences vector, I would no longer know which PatchPair each difference corresponded to. By this folding/accumulation you are suggesting, how do the differences stay associated with their generating PatchPair?

Another concrete problem with your code is that you are over-restricting yourself by postulating that all your code can and will do is "compute differences" of patches or whatever. Computing differences between pixels / patches / images / other is no different, in process, than computing any other accumulated quantity (average, std-dev, etc.). This is a very clear case of over-restricting the scope of your design, which leads to mixing responsibilities without even being aware of it, which leads to an inflexible design. You need to look at the problem from the most generic point-of-view first, and then make conscious choices about what assumptions are acceptable such that you get the flexibility you need, the applicability you need, and don't have to make too crazy-generic code that solves every problem of the universe. I don't think you did that thought-exercise on your design, because it shows.

You should have seen it before this last refactoring :) I thought I was doing good by having a "generic" PatchDifference class and then the subclass of PatchDifferencePixelWiseSum. By implementing another subclass, say PatchDifferenceAverage, I could do what you were referring to.

If I want to compute the average pixel value of an image, what do I do? I iterate through the pixels, keep an accumulated average pixel value that is updated by each pixel traversed, and that's it. What I need for this is an "ImageIterator", some "PixelVisitor", and some "PixelAccumulator". The idea here is that the visitor follows the image-iterator by being given its visited pixels, and registers them with the accumulator.

I understand the iterator. However, I don't understand the link between the visitor and the iterator. How does the visitor "follow" the iterator? Likewise with the link between the visitor and accumulator - how are they connected. Can you suggest a good reference (book/website) for this type of thing? I'd love to study a coded example. I don't think the jump from the theory to the code is as easy as you make it sound :)

If I want to take the "total" difference between a patch and every patch of some image, then I need to iterate through all the patches of the image, form a pair with given patch and the visited patch, feed this to some visitor which will accumulate the difference between all patches. In other words, I need a "PatchPairIterator", a "PatchPairVisitor", a "PatchCompareMap", and some "Accumulator" for the output of the patch-comparison. In the specifics, the "PatchCompareMap" (derived-class or template argument) would take the difference between the two patches, the "Accumulator" would accumulate a sum, and the "PatchPairIterator" would be a kind of patch-over-image iterator that visits every patch-pair that is formed by one constant patch and every patch of a given image.

I really like how these patterns (for pixels and for patches) are identical. It makes it look like a good design (probably because it is :) ). Now I just have to figure out how to implement it as I mentioned above.

Again, thanks a ton. I'll do some googling for "visitors" and "accumulators", but if you have any suggested reading I'd love to hear it.

David

[mike_2000_17]

>>You are suggesting that the differences should NOT be contained in the PatchPair, right?

Yes.

>>By this folding/accumulation you are suggesting, how do the differences stay associated with their generating PatchPair?

By an associative container, by a multi-index container (boost.multi-index), by a simple std::pair< float, PatchPair> , by a custom POD type, or whatever. Fundamentally, this is a multi-index structure and should probably use that if you need to associate the diff-values with the patch-pairs, but you shouldn't require this association because it can be a costly one and may not always be necessary. The important thing is that the patch-pair that is the input to the algorithm that computes the diff-value isn't bound to the algorithm that computes its diff-value, because that makes no logical sense and is only going to be hard to deal with programmatically.

>>Can you suggest a good reference (book/website) for this type of thing?

For references on accumulators (which is a very simple concept once you see it), I would guess Boost.Accumulator is a good example library for that. Fundamentally, many uses of lambda-expressions (C++11) that use closures make the lambda-expression act as a simple accumulator.

As for libraries that use visitors, I suggest you look at the design of the Boost.Graph library (BGL), more specifically, at the graph traversal algorithms like Dijkstra, A* and so on, they are all built upon three fundamental traversal algorithms (breadth-first, depth-first, and uniform cost search). And I mean, the different search algorithms are just implemented as a special kind of visitor for one of these three basic search algorithms. I have similar algorithm patterns in my library as well (link below, under the path-planning graph algorithms).

On more concrete image-processing applications, OpenCV works a lot like that when it comes to image filters (e.g. sweeping a filter window (or patch) over all the patches of the image to create an output image is the core algorithm in OpenCV).

>> How does the visitor "follow" the iterator?

Ok, this is a bit of a misuse of words, here by "iterator" I meant an iterating algorithm (like those graph-traversal algorithms in BGL) and the visitor is a helper object (with a few key functions) that are called by the controlling algorithm to register (or do some work with) the visited elements of the algorithm. Here is a simple example to take the average value of pixels:

class AveragingVisitor {
  private:
    int number_of_pixels;
    float average_grey_value;
  public:
    AveragingVisitor() : number_of_pixels(0), average_grey_value(0.0) { };
    int getPixelCount() const { return number_of_pixels; };
    float getAvgGreyValue() const { return average_grey_value; };
    void visitPixel( const GreyPixel& aPix) {
      ++number_of_pixels;
      average_grey_value = (number_of_pixels - 1) * average_grey_value + aPix.getValue();
      average_grey_value /= number_of_pixels;
    };
    void visitPixel( const ColorPixel& aPix) {
      ++number_of_pixels;
      average_grey_value = (number_of_pixels - 1) * average_grey_value + (aPix.getRed() + aPix.getGreen() + aPix.getBlue()) / 3.0f;
      average_grey_value /= number_of_pixels;
    };
};

//the ieration algorithm (core algorithm)
template <typename Image, typename Region, typename PixelVisitor>
void visitAllPixelsInRegion(const Image& im, const Region& rect, PixelVisitor& vis) {
  //in pseudo-code:
  for all pixels 'pix' in region 'rect' of image 'im' do
    vis.visitPixel(pix);
};

//The averaging algorithm for the grey values of an image:
template <typename Image>
float computeGreyValueAverage( const Image& im) {
  //create an averaging visitor (and accumulator):
  AveragingVisitor visitor;
  //call the core algorithm:
  visitAllPixelsInRegion( im, entire_image_region(im), visitor);
  return visitor.getAvgGreyValue();
};

The above is called either Inversion of Control, or the Visitor Pattern (in generic programming, because in OOP it has a different meaning).

The idea in the above is pretty clear, you can split the core traversal algorithm, the basic visitation method (note you can have more elaborate visitors that feed information back to the traversal algorithm too), the accumulation or output-writing code, and the concrete algorithms because simple compositions of the various elements (and, its in the possible combinations that you get all the flexibility you need).

Also, note that the above example and others you will find in BGL or Boost.Accumulators or even OpenCV are in generic programming (because it is generally more appropriate for algorithmic work), but it is easy to translate that code into OOP if you are compelled to do that.

[daviddoria]

I'm slowly getting the idea... I wrote a couple of examples here:
http://stackoverflow.com/questions/8771932/the-use-of-an-accept-in-the-visitor-pattern

Mike, in your code example, you have implemented visitPixel(GreyPixel) and visitPixel(ColorPixel) both inside AveragingVisitor. I've also seen this in most of the examples I've been looking at. Is there a reason for doing that, as opposed to writing a AveragingColorPixelVisitor, and an AveragingGreyPixelVisitor? It seems like the logic has to be duplicated anyway, so why not keep them totally separate? In my case, this would be an AveragingPixelVisitor and an AveragingPatchVisitor - if they are separate then the AveragingPixelVisitor won't depend on the Patch class, which seems like a good thing.

[mike_2000_17]

>>Is there a reason for doing that, as opposed to writing a AveragingColorPixelVisitor, and an AveragingGreyPixelVisitor?

Yes. As you said, "the logic has to be duplicated anyways", well, not exactly. From the outside world, the logic achieved by the visitPixel(GreyPixel) and visitPixel(ColorPixel) is the same (i.e. accumulates the given grey value to the average grey value), but the implementation of it is different when given color pixel because the grey value (average or max or whatever) has to be computed from the color values. The idea here is "encapsulation", that is, there is one logic (accumulate the average grey value) but multiple implementation details (grey or color pixels), so, you encapsulate the implementation details in one functor class that represents one unique logical operation.

Doing this also has practical benefits (these design practices are not there just for show). In the example I gave, one thing you would have to do, if you had two visitors (e.g. AveragingColorPixelVisitor and AveragingGreyPixelVisitor), is to provide some switch code to detect the kind of pixels in the image and provide the correct visitor:

//The averaging algorithm for the grey values of a grey-value image:
template <typename GreyImage>
typename boost::enable_if< is_grey_image<GreyImage>,
float >::type computeGreyValueAverage( const GreyImage& im) {
  //create an averaging visitor (and accumulator):
  AveragingGreyPixelVisitor visitor;
  //call the core algorithm:
  visitAllPixelsInRegion( im, entire_image_region(im), visitor);
  return visitor.getAvgGreyValue();
};

//The averaging algorithm for the grey values of a color image:
template <typename ColorImage>
typename boost::enable_if< is_color_image<ColorImage>,
float >::type computeGreyValueAverage( const ColorImage& im) {
  //create an averaging visitor (and accumulator):
  AveragingColorPixelVisitor visitor;
  //call the core algorithm:
  visitAllPixelsInRegion( im, entire_image_region(im), visitor);
  return visitor.getAvgGreyValue();
};

You have to regard the Visitor as an abstraction of a logic pattern, or a generic method, meaning that what it represents is the logic that it implements, the details should be encapsulated, otherwise, you inflict the burden of the details on all the code that uses it (as in the above example, where you require that the calling function worry about using the correct visitor for the correct type of image, these are implementation details that the caller shouldn't have to worry about).

So, the rule is that if the visitor has to do different things to achieve the same logical operation depending on the type of the visited element, then those switching implementation details should be hidden in the visitor's implementation. However, you should not implement different logic operations based on the given element type (in this case, split it into two distinct visitors). For example, if you wanted a visitor that would compute the average color value (not the average grey-value) you would need another Visitor class for that.

The pattern's philosophy is such that the caller assembles the logical blocks that creates the desired algorithm (e.g. "linear traversal + average accum. = averaging algo.", or "linear traversal + max accum. = max finding algo."), and the given blocks take care of realizing the details. So, ideally (it is not always possible), you want to create distinct blocks only for distinct logical purposes, not for the distinct implementation details (of course, you can implement the different details using different classes, but you should expose a unique interface).

[daviddoria]

I think I'm getting the hang of it. I have a visitor setup for single Patch objects.

https://github.com/daviddoria/PatchBasedInpainting/blob/master/Patch.hxx
https://github.com/daviddoria/PatchBasedInpainting/blob/master/PixelSumAccumulator.h
https://github.com/daviddoria/PatchBasedInpainting/blob/master/Tests/TestPixelSumAccumulator.cpp

For the real case of finding the difference between the two patches in a PatchPair object, I made a visitor that accepts two pixels in its Visit function (Visit(pixel, pixel)).

https://github.com/daviddoria/PatchBasedInpainting/commit/49b05e6327a48dc0c64fb4e9ef783d87a12e36e1

I see now that I can create a vector<PixelPairVisitor> and then apply all of these visitors to compute the differences I am interested in.

---------

I'm still uneasy about removing the PairDifferences object from the PatchPair object. I guess it depends on what you return from a PatchDifferenceVisitor that gets applied to a whole image. With the PixelDifference visitors, we are actually accumulating so we intentionally lose the localization of the differences. With the PatchDifferenceVisitor, would you return a vector<std::pair<PatchPair, float> >? If so, to apply more than once visitor which each computes a different difference, would you do something like this:

vector<PixelPairVisitor> visitorsToApply;
typedef vector<std::pair<PatchPair, float> > SinglePass;
vector<SinglePass> AllDifferences;

for all 'visitor' in visitorsToApply
{
  AllDifferences[i] = image.VisitAllPatches(visitor)
}

// Combine AllDifferences into a vector<std::pair<PatchPair, PairDifferences> >

That would work, but it seems dangerous. Thoughts?

[mike_2000_17]

>>If so, to apply more than once visitor which each computes a different difference, would you do something like this:

No offense, but that's a terrible idea. Not necessarily dangerous, but inefficient and ineffective.

If you need to do many things with each pixel / pixel-pair / patch / patch-pair, then make a composite visitor, which is, in turn, a visitor as well. It's that simple. Basically, a composite visitor would just be a visitor class that is also a container of visitors. For each element(-pair) that it is given, it traverses all the visitors that it contains and applies them to the element(-pair). You can also have special composite visitors with a special set of visitors, the possibilities are endless. So, your example would be more like this:

vector<PixelPairVisitor> visitorsToApply;
CompositeVisitor all_visitors(visitorsToApply);

image.VisitAllPatches(all_visitors)

// Extract the accumulated values from the "visitorsToApply" vector (or the composite visitor).

This is more effective because you can treat any kind of visitor just the same, in other words, it scales much better to the development of various kinds of visitors. To achieve this, you have to realize that you CANNOT channel the "output" of the visitors through the image-traversal function (the "VisitAllPatches" function for example), because this will almost inevitably cause you to make an assumption about the nature of the output of the visitor (unless you have some really good template meta-programming kung-fu skills under your belt). Don't make assumptions that restrict the applicability of your methods unless there is a really compelling reason to do so (and I don't see one in this case). What this means is this:

// what you did (which is WRONG), as seen from the call-site:

  output_of_visitation = image.VisitAllElements( my_visitor );

// what you should do, as seen from the call-site:

  my_visitor.set_ptr_to_output( &output_of_visitation );
  image.VisitAllElements( my_visitor );

 // for ease-of-use, you can wrap this up as:
Pixel Image::GetAveragePixel() const {
  PixelAveragingVisitor<Pixel> my_visitor;
  this->VisitAllPixels( my_visitor );
  return my_visitor.GetAvgPixel();
};

The important thing is that because the visitor is not required to provide some fixed type of output, you have total freedom in what kind of output the visitor can produce (including a second image, and thus, a visitor can act as an image filter (the second image can even be the original one, in cases where that works)). This way, you don't have to worry too much about whether the visitor's output is a single value, an image, a pixel, a vector of float, a bunch of heterogeneous data, etc.

It is also important to note that my alternative given above will be a lot more efficient in practice. If you are processing a large image, the golden rule is that you want to minimize the number of times that you have to traverse the image. It is a lot more efficient to traverse the image once and apply a number of visitors at each pixel, than it is to traverse the image for each visitor you have to apply. The reason for that: memory locality (or locality of reference). It is more likely that the processor will be able to cache all the visitation code and memory (for output), than it is it will be able to cache the entire image. That's a pretty good rule of thumb, the larger something is, the more you want to arrange your algorithm such that the traversal of that large chunk of memory is done as few times as possible.

>>With the PatchDifferenceVisitor, would you return a vector<std::pair<PatchPair, float> >?

What about creating an output image for the visitor, where each "pixel" would be the difference of the patch-pair corresponding to that image coordinate. I saw that your Patch class has the necessary information to localize it in the image from which it is drawn (duh!). So, the PatchDifferenceVisitor can easily use that information in the given patch (maybe differentiate between the "constant" patch and the image patch in the visitor's interface) to locate where the computed difference-value should be saved in some output "image" (I mean, just a grid of float-value or whatever). You see, this is basically an image filtering algorithm (where the constant patch is the filter-window, the visitor applies the filtering method (sum of differences), and you get the filtered output image).

Concretely, you might want to create different categories of visitors, like accumulators and filters. The obvious difference between the two is that an accumulator produces one output (scalar or elemental) from the set of elements it has visited, while a filter produces one output for each element (or pair) that it has visited. Why I'm saying this is that you could do something like this:

//Base classes:
class Visitor { /* ... */ };

class AccumulatingVisitor : public Visitor { /* ... */ };

class FilteringVisitor : public Visitor { /* ... */ };

class PatchFilterVisitor : public FilteringVisitor {
  private:
    AccumulatingVisitor* pixel_visitor;
    Image<float>* output_image;
  public:
    void visit( Patch& ImagePatch, Patch& FilterWindow) {
      pixel_visitor->Reset();
      for each 'pixel' in ImagePatch 
        pixel_visitor->visit( ImagePatch[pixel], FilterWindow[pixel]);
      (*output_image)[ImagePatch.GetCenterCoordinate()] = pixel_visitor->GetAccumValue();
    };
};

With the above you have one filtering visitor that can apply any underlying accumulating pixel-visitor to its given patch and produce a complete output image where each pixel is the patch-accumulated value. Obviously, your PatchDifferenceVisitor is essentially doing this (with the underlying accumulating visitor being the PixelPairDifferenceSum).

[rubberman]

When I have to deal with complex development issues, I start with a full-blown UML model. In fact, I spend 80% or more of my time in the model, and only about 10% in coding. The modeling helps me visualize and work out sequence, timing, and other problems before I even start to write code. I have been able to develop software of immense complexity that way, and that worked right the first time, every time. This is software that runs most of the semiconductor, flat-panel, and disc drive manufacturing plants in the world and is currently sold/maintained by Applied Materials, the 800lb gorilla of the semiconductor equipment & software world.

I'm now working for the biggest cell phone manufacturer in the world as Senior Systems/Performance Engineer, and after one week, most of my time is in the UML modeler. At this point, it is helping me see how the system works, and how the various components interact. Next, I will be able to identify where performance bottlenecks are likely to be found. Even though this group is pretty darned sophisticated, there are still areas where we are just guessing at the problems that may be contributory to system load issues - considering that we are currently handling 5-10M concurrent users and need to ramp up an order of magnitude over the next year...

So, start with a model and get a full understanding of the classes, relationships, components, and behaviors of the system BEFORE you start coding. If you have already started coding (as you have), then develop a UML model of what you have, and I think that the problem areas will become apparent, quite quickly. Just MHO (my "humble" opinion).

[daviddoria]

@rubberman - thanks, I'll try to do that at some point. I found Umbrello which was able to import my classes, but unfortunately it does not draw the connections/dependencies automatically. Any suggestions for free programs that can do this?

@mike_2000_17 - the composite visitor is a great idea. This code is getting easier by the minute. The reason that I don't want an "image" type output that stores the values is that I need to sort them. Their ranking (sorted distance) is more important than their location in the image, but the location has to come along for the ride because I need it later. I take your point though, this is an awesomely flexible framework.

[mike_2000_17]

>>The reason that I don't want an "image" type output that stores the values is that I need to sort them.

Well, when I said:
"This way, you don't have to worry too much about whether the visitor's output is a single value, an image, a pixel, a vector of float, a bunch of heterogeneous data, etc."
The important part of this sentence is "et cetera", meaning just about anything else you can think of. I suggested creating a few special visitor classes for some of the very important cases, that is, an accumulator (output = scalar value) and a filter (output = another image), but it does not mean all visitors have to be of one of the two types, because the design of the traversal algorithms shouldn't require them to be, in fact, shouldn't require them to be anything more than a Visitor (a fundamental principle of software engineering is the "principle of minimum requirement").

As for rubberman's point on UML, I totally agree that you need to spend a lot more time designing the software then you spend time actually coding it. However, I'm not a big fan of UML, but that is probably just because I never encountered a UML tool that was satisfactory to me (they either lack expressivity or they lack clarity). I would be glad to hear rubberman about what software he would recommend for that, if any, because I usually do my UML-like design with the good old pen-and-paper.

If you have code already and you want to generate a UML from it, or at least, something close to it, then you need a documentation generation system. Of course, for C++, I highly recommend doxygen (it would be really surprising if you didn't know that software already by now, although I did notice that you repo's code didn't have doxygen-tagged documentation in it).

[rubberman]

Re. Umbrello. I used it a long time ago. I would think that it should at least show class derivations/generalizations (the arrow from the derived to the base class) since that should be in the class declaration. Associations/Compositions/Aggregations are another issue, however; these are usually associated with member variables and what variety they are may not be obvious, especially for aggregations (a collection of stuff), since if they are owned by the container class, they would properly be composition types (solid diamond at one end), and if not, then they would be aggregations (empty diamond at one end). So, I expect that you would have to make those "associations" manually.