Again, histograms

[Bart Coppens]

So I've been playing around a bit with the histograms locally, and currently I 
have something like the image at
http://www.bartcoppens.be/krita/krita-histogram-colors.png
The possibility to view all channels at a time without additional iterating 
than viewing a single channel. But while working this out, I ran into two 
problems. One is related to my design, and one to colorspace independence.

1) The design
I am starting to fear that the entire approach is overdone and that the 
endresult may look confusing to others. I'll try to explain a bit the design 
and some pieces of it, and maybe you don't find it too weird. (And if it's 
not too weird I can still make a file for doc/ out of this mail, so I'll try 
to be elaborate enough ;-) )
Currently, I have something like this:

* A KisHistogram is reduced to a basic interface that you can ask things of 
like getHighest() and getValue(uint8). It has become something that should do 
all the calculations concerning histograms (now it's basic work, but the idea 
is that that might change, that's why I kept it), but the actual bins and 
other needed information is stored in:

* A KisHistogramProducer. It is called a 'Producer' because it produces the 
data that the actual histogram will use. This is the basic building block in 
the histogram now. One such a producer contains info on several channels. So 
if an RGBA8  producer loops over a layer, it will have info on R, G, B and A 
channels stored in it. The basic usage is that you clear() it, ask which 
channels it supports with channels() or add data to it with
void addRegionToBin(KisRectIteratorPixel& it, KisAbstractColorSpace* 
colorSpace). Afterwards, you read the data from it with getBinAt(channel, 
position). It has some functions concerning with 'views' (or 'zooms', 
whatever you like to call them), I'm not exactly sure yet how those are best 
implemented, but that's not disastrous I think.

Now the idea was that we _might_ later on want to add some kind of cached 
histograms to a paint device. I had in mind that each such cache is 
represented by a single producer, so we need to have lots of producers and 
they can't be statically shared pointers. So we introduce:

* KisHistogramProducerFactory, which has a method generate() that creates us a 
new producer. Such a factory is a quite small class, with only 2 other 
functions: a function that returns a KisID and a function
bool isCompatibleWith(KisAbstractColorSpace* colorSpace)
which will return true only if the colorspace can be used by the producer 
generated by the factory. Now since we need to get to such a factory from the 
code somehow in a dynamic manner, we finally add the class

* KisHistogramProducerFactoryRegistry, which is a registry for producerfactory 
(see how it is becoming to feel a bit complicated and overengineered? ;-) ) 
This is a regular KisGenericRegistry, but with a nice method:
KisIDList listKeysCompatibleWith(KisAbstractColorSpace* colorSpace)
This lists all the factories that are compatible with that specific colorspace 
(that's why I added the isCompatibleWith to the factory, not the producer 
itself)

Now of course all of this would be a _lot_ to implement for each colorspace 
author, and for colorspaces which don't have a need for fun or exotic 
histograms it would be just a boring waste of time to implement each time. 
So, I made KisBasicHistogramProducer which implements most of the functions 
of a producer in a standard, non-exotic way that would suffice for must 
usages. Now because we want to be colorspace independent I need to have 
information on how many channels and so there are, so finally, we have the 
class the colorspace author will see: KisBasicU8HistogramProducer (and the 
other relevant sisterclasses, but I haven't bothered to implement those yet). 
This class only implements a single 10-line method.

The Factory gets a similar basic implementation with 
KisBasicHistogramProducerFactory, which is of ease of use a template class 
parametrized on a producer (like a basicu8 producer).

Now the problem is that the actual code the author of a plugin will need to 
add looks like this (from RGB8):

        KisColorSpaceRegistry::instance() -> add(m_ColorSpaceRGBA);
        KisHistogramProducerFactoryRegistry::instance() -> add(
                new 
KisBasicHistogramProducerFactory<KisBasicU8HistogramProducer>
                    (KisID("RGB8HISTO", "RGB8 Histogram"), 
m_ColorSpaceRGBA) );

And even though it's only 2 real lines of code a normal colorspace author will 
need to add, I fear it looks confusing and complicated (not to mention that 
the names are insanely long). Any thoughts on this?

2) Colorspace independence
Ah, our must fun and yet sometimes quite hateful goal ;-) While recoding and 
refactoring the KisHistogram code itself, I noticed something here. What will 
we do when we want to implement zooming (setView of a producer)? You have to 
set a range, but how precisely do you want to set a range? You probably want 
to say something like: 'I want to view the histogram of the pixel values 
between 0 and 125'. That works with uint8, but how are people supposed to 
input this in the widget, when a colorspace channel is a float, uint32, 
double, etc? Or even if we don't let them input it directly, and only press a 
'Zoom' button. Still you'd need to display the currently displayed range.
Or for that matter, how do I instruct the specific producer to set such a 
view? Currently my unused function uses Q_UINT32, but that won't be a good 
idea for doubles, etc. Any ideas or insights on this?

Bart Coppens

(This post originally had the image attached, but that mail was held for 
moderator's approval...)