Storing data in a Visitor

[daviddoria]

(Following up on this discussion : http://www.daniweb.com/software-development/cpp/threads/405285/1732795#post1732795 )

Is it ok to store data in a Visitor class? In the case below, I have a Visitor named PixelWisePatchPairVisitor that internally needs to itself use a visitor. At some point the Image to visit needs to come into the picture. In this case I gave PixelWisePatchPairVisitor a pointer to the Image and then passed the image as a function argument to void VisitAllPixelPairs(TImage* image, TPixelPairVisitor& visitor) const. Is this ok?

#include <iostream>
#include <vector>

template <typename TPixel>
class PixelPairVisitor
{
public:
  virtual void Visit(const TPixel& pixel1, const TPixel& pixel2) = 0;
};

template <typename TPixel>
class SumOfDifferencePixelPairVisitor
{
public:
  SumOfDifferencePixelPairVisitor() : Sum(0) {}
  
  void Visit(const TPixel& pixel1, const TPixel& pixel2)
  {
    Sum += pixel1 - pixel2;
  }

private:
  TPixel Sum;
};

class PatchPair; // Forward declaration

class PatchPairVisitor
{
public:
  virtual void Visit(const PatchPair& patchPair) = 0;
};


class Region
{

};

class Patch
{
  Region region;
};

class PatchPair
{
public: 
  template <typename TImage, typename TPixelPairVisitor>
  void VisitAllPixelPairs(TImage* image, TPixelPairVisitor& visitor) const
  {
    for(int i = 0; i < 10; ++i)
      {
      // These need to be pixels from an image
      float sourcePixel = image->GetPixel(i);
      float targetPixel = image->GetPixel(i);
      visitor.Visit(sourcePixel, targetPixel);
      }
  }
private:
  
  Patch SourcePatch;
  Patch TargetPatch;
};


template <typename TImage, typename TPixelPairVisitor>
class PixelWisePatchPairVisitor : public PatchPairVisitor
{
public:
  PixelWisePatchPairVisitor(TImage* image) : InternalImage(image){}
  
  TPixelPairVisitor InternalPixelPairVisitor;
  
  void Visit(const PatchPair& patchPair)
  {
    patchPair.VisitAllPixelPairs(InternalImage, InternalPixelPairVisitor); // How does 'image' get here?
  }
private:
  TImage* InternalImage;
};

class PatchPairContainer
{
public:
  void VisitAllPatchPairs(PatchPairVisitor& visitor)
  {
    for(unsigned int i = 0; i < this->PatchPairs.size(); ++i)
      {
      visitor.Visit(this->PatchPairs[i]);
      }
  }
private:
  std::vector<PatchPair> PatchPairs;
};

template <typename TPixel>
class Image
{
public:
  TPixel GetPixel(int i)
  {
    return 1; // Clearly this would return actual values
  }

private:
  std::vector<TPixel> Pixels;
};

int main()
{
  PatchPairContainer patchPairs;

  Image<float>* image = new Image<float>;

  PixelWisePatchPairVisitor<Image<float>, SumOfDifferencePixelPairVisitor<float> > visitor(image);
  patchPairs.VisitAllPatchPairs(visitor);
  return 0;
}

I could have instead made both iterators depend on the image and then none of the function calls would need to be passed the image:

VisitAllPixelPairs(TPixelPairVisitor& visitor) // as long as 'visitor' has the Image, we're good

Any thoughts on if this is ok to do?

[daviddoria]

I also realized that if I use a class template for a visitor:

template <typename TPixel>
class PixelPairVisitor
{
public:
  virtual void Visit(const TPixel& pixel1, const TPixel& pixel2) = 0;
};

That I wouldn't be able to put them in a container (as there is no superclass). However, c++ doesn't allow this:

class PixelPairVisitor
{
public:
  template <typename TPixel>
  virtual void Visit(const TPixel& pixel1, const TPixel& pixel2) = 0; //error: templates may not be ‘virtual’
};

How are you supposed to get a class template with a function that depends on the template parameter into a container??

[mike_2000_17]

Shouldn't the Patch class hold a pointer to the image to which it is a patch of? It would make a lot more sense if you had the Patch class to act as a view of the image. What is a Patch if it is not a sub-image? If the Patches would act as sub-images, you could simply visit all their pixels as you would when visiting all the pixels of an ordinary image. If you do that, I see no reason to pass the Image pointer down to the patch traversal algorithm, because that pointer would be encapsulated in the Patches themselves. And to answer the question, no, I don't think it is a good idea to pass down the image pointer, if that wasn't clear enough.

I think you do need to take a step back from coding and seriously think about your design.

As for your latest question about "virtual function templates", well you cannot do this. You would have to have a base-class for all you "pixels". Personally, I think that would be crazy. At this kind of algorithmic level, you will have to either give up on trying to get a flexible generic implementation, or give up on relying purely on object-oriented programming (I mean, on the idea of using virtual functions and base-classes as the central mechanism for polymorphism). In other words, you need to implement all this algorihtmic code using templates and generic programming techniques, otherwise, with OOP, you won't be able to get the kind of flexibility you seem to be looking for, and will have to give up on that. The possibilities for run-time flexibility are very limited compared to the compile-time counter-part, and you are experiencing those limitations.

[daviddoria]

Shouldn't the Patch class hold a pointer to the image to which it is a patch of? It would make a lot more sense if you had the Patch class to act as a view of the image. What is a Patch if it is not a sub-image?

I did it like this on purpose (though it might still be "wrong" haha). The idea is that all of the images are the same size, so if a patch is valid (inside) in one of them it is valid in all of them. The thought was that if I associate a specific image with a Patch, then I have to have the same patch duplicated for every image. Whereas if I am just describing a general region of an image with a Patch, it can be applied to any image. This was working fine until we introduced visitors :)

If the Patches would act as sub-images, you could simply visit all their pixels as you would when visiting all the pixels of an ordinary image. If you do that, I see no reason to pass the Image pointer down to the patch traversal algorithm, because that pointer would be encapsulated in the Patches themselves. And to answer the question, no, I don't think it is a good idea to pass down the image pointer, if that wasn't clear enough.

I agree that this part would be much easier if the Patch was actually related to an image.

I think you do need to take a step back from coding and seriously think about your design.

Definitely. Please see my PM :)