HSV Color Dialog

witte

Hi,
Du könntest im Grafikforum mal nachfragen ob es Optimierungsalgorithmen für sowas existieren, irgendwelche Bitmusten die sich leicht erstellen lassen.
Des weiteren könntest Du die Berechnung von q und t in HSVtoRGB nur dann ausführen lassen wenn die entsprechenden Fälle (hi == 1, 5 bzw 0, 4) auftreten.

Th

Desweiteren würde ich als Basisdatentyp 'int' (bzw. unsigned char) statt 'float' wählen, da sonst immer eine Umrechnung von float nach int bei den Zuweisungen

ptr[a] = RGB.R;
...

gemacht werden muß.

Die Methode HSVtoRGB könnte allerdings dann noch ein wenig optimiert werden.
Ganz so schlimm wie akari sehe ich es nicht, daß sie unbedingt von Hand geinlined werden muß, es würde z.B. "const HSVType &HSV" als Übergabeparameter reichen (nur noch ein "Zeiger"). Und der Rückgabewert sollte ebenfalls als "Referenzparameter" mitgegeben werden.
Aber insbesondere das Rauf- und Runterrechnen von den Bereichen [0...1] nach [0...255] kosten sehr viel Performance... (und ist auch unnötig).

Tsurai

Ok...ich habe jetzt meine Berechnung der Farbwerte korrigiert(die war nämlich total falsch) und habe eure Vorschläge zur Verbesserung der Rechenzeit mit eingebaut.
Leiber ist das Bild, was jetzt übrings richtig ist, immer noch leicht am ruckeln. Fällt euch eventuell noch etwas ein womit man das ganze beschleunigen könnte? Obwohl es sich schon um einiges gebessert hat wofür ich euch sehr dankbar bin.
Mein code sieht jetzt wie foglt aus:

typedef struct {int R, G, B;} RGBType;

//...

void TForm1::change_color()
{
 HSVType HSV;
 RGBType RGB;

 float H, S, V, f, p, q, t;
 int hi, a;

 HSV.H = edtH->Text.ToInt();
 HSV.S = edtS->Text / 100;
 HSV.V = edtV->Text / 100;

 RGB = HSVtoRGB(HSV);

 edtR->Text = int(RGB.R);
 edtG->Text = int(RGB.G);
 edtB->Text = int(RGB.B);

 Graphics::TBitmap *bmp = new Graphics::TBitmap;
 bmp->Width = 50;
 bmp->Height = 50;
 bmp->Canvas->Brush->Color = RGB(RGB.R, RGB.G, RGB.B);
 bmp->Canvas->FillRect(TRect(0,0,50,50));

 imgColorPreview->Picture->Bitmap = bmp;

 //Draw Color Palette
 Byte *ptr;
 Graphics::TBitmap *bmpColor = new Graphics::TBitmap;
 bmpColor->Width = 256;
 bmpColor->Height = 256;
 bmpColor->HandleType = bmDIB;
 bmpColor->PixelFormat = pf24bit;

 for(int y = 0; y < 255; y++)
 {
     ptr = (Byte *)bmpColor->ScanLine[y];
     for(int x = 0; x < 255; x++)
     {
         HSV.S = (0.0039 * x);
         HSV.V = (0.0039 * (255-y));

         //RGB = HSVtoRGB(HSV);

         H = HSV.H;
         S = HSV.S;
         V = HSV.V;

         if(H == 360)
            H = 0;

         hi = H / 60;
         f = (H / 60) - hi;

         p = V * (1 - S);
         q = V * (1 - S * f);
         t = V * (1 - S * (1 - f));

         switch(hi)
         {
          case 0:
            RGB.R = V * 255;
            RGB.G = t * 255;
            RGB.B = p * 255;
            break;
          case 1:
            RGB.R = q * 255;
            RGB.G = V * 255;
            RGB.B = p * 255;
            break;
          case 2:
            RGB.R = p * 255;
            RGB.G = V * 255;
            RGB.B = t * 255;
            break;
          case 3:
            RGB.R = p * 255;
            RGB.G = q * 255;
            RGB.B = V * 255;
            break;
          case 4:
            RGB.R = t * 255;
            RGB.G = p * 255;
            RGB.B = V * 255;
            break;
          case 5:
            RGB.R = V * 255;
            RGB.G = p * 255;
            RGB.B = q * 255;
            break;
         }

         a = x*3;
         ptr[a] = RGB.B;
         ptr[a+1] = RGB.G;
         ptr[a+2] = RGB.R;
     }
 }

 imgColorPalette->Picture->Bitmap = bmpColor;

}

audacia

Wenn ich schrieb, du solltest nicht manuell delete und delete[] verwenden, wollte ich nicht sagen, daß du deine Objekte gar nicht freigeben sollst
Benutze doch einfach einen auto_ptr.

Tsurai schrieb:

Fällt euch eventuell noch etwas ein womit man das ganze beschleunigen könnte?

Du führst zu viele Multiplikationen in der inneren Schleife aus, wo sie gar nicht notwendig sind. akari hat dazu schon ein paar recht nützliche Hinweise gegeben.

Wenn du dir die Definition des Algorithmus ansiehst, sollte klar sein, welche Berechnungen du in die äußere Schleife verschieben kannst. Beispielsweise bleiben die Werte h_i und f für die ganze Funktion gleich, da H sich nicht ändert. Und die Koeffizienten von p, q und t kannst du in der äußeren Schleife berechnen. Zudem ist es effizienter, V gleich als Integerzahl von 0 bis 255 iterieren zu lassen, anstatt es streng nach Vorschrift in [0..1] zu halten und danach zu skalieren.

Mit diesem Ansatz kam ich auf folgendes:

#pragma pack(push, 1)
struct RGBRawLEType
{
    unsigned char B;
    unsigned char G;
    unsigned char R;
};
#pragma pack(pop)

void TFrmMain::refreshColorPicker (int hue)
{
    std::auto_ptr <Graphics::TBitmap> bmpColor (new Graphics::TBitmap);
    bmpColor->PixelFormat = pf24bit;
    bmpColor->SetSize (256, 256);

    int hi = hue / 60.0;
    float f = hue / 60.0 - hi;
    unsigned char RGBRawLEType::* p;
    unsigned char RGBRawLEType::* q;
    unsigned char RGBRawLEType::* t;
    unsigned char RGBRawLEType::* v;
    switch (hi)
    {
    case 0:
        v = &RGBRawLEType::R;
        t = &RGBRawLEType::G;
        p = &RGBRawLEType::B;
        q = 0;
        break;

    case 1:
        q = &RGBRawLEType::R;
        v = &RGBRawLEType::G;
        p = &RGBRawLEType::B;
        t = 0;
        break;

    case 2:
        p = &RGBRawLEType::R;
        v = &RGBRawLEType::G;
        t = &RGBRawLEType::B;
        q = 0;
        break;

    case 3:
        p = &RGBRawLEType::R;
        q = &RGBRawLEType::G;
        v = &RGBRawLEType::B;
        t = 0;
        break;

    case 4:
        t = &RGBRawLEType::R;
        p = &RGBRawLEType::G;
        v = &RGBRawLEType::B;
        q = 0;
        break;

    case 5:
        v = &RGBRawLEType::R;
        p = &RGBRawLEType::G;
        q = &RGBRawLEType::B;
        t = 0;
        break;

    default:
        throw ERangeError ("Keep hue in [0..360)");
    }

    float pco, qco, tco;
    RGBRawLEType* rgbp;
    float delta = 1.0f / 256;

    int y = 0;
    for (float saturation = 0.0f; y < 256; saturation += delta, ++y)
    {
        rgbp = static_cast <RGBRawLEType*> (bmpColor->ScanLine[y]);

        pco = 1 - saturation;
        qco = 1 - saturation * f;
        tco = 1 - saturation * (1 - f);

        for (int value = 0; value < 256; ++value, ++rgbp)
        {
            rgbp->*v = value;
            if (p)
                rgbp->*p = value * pco;
            if (q)
                rgbp->*q = value * qco;
            if (t)
                rgbp->*t = value * tco;
        }
    }

    ImgColorPalette->Picture->Bitmap = bmpColor.get ();
    ImgColorPalette->Repaint ();
}

Hier kannst du das komplette Programm herunterladen. Läuft bei mir flüssig

Edit: kleine Lesbarkeitsverbesserung

witte

Alternativ kannst Du das Zeichnen der Farbfläche in einen anderen Thread auslagern, so dass das Benutzen des Sliders ruckelfrei bedienbar ist.

audacia

Nur das Zeichnen auf die Bitmap; der Zugriff auf das TImage-Element muß aus dem UI-Thread erfolgen.

Tsurai

Wenn ich deinen code übernehmen will bekomme folgenden Fehler:
[C++ Error] Unit1.cpp(18): E2316 'SetSize' is not a member of 'TBitmap'

audacia

Welche C++Builder-Version?

akari

Hallo

Im BCB 5 gibt es TBitmap::SetSize jedenfalls noch nicht. Da must du eben TBitmap::Height und ::Width getrennt voneinander setzen.

bis bald
akari

Tsurai

Ich benutze den Borland C++ Builder 6.0 Enterprise

audacia

Dann tu, was akari sagt.

Du kannst die innere Schleife übrigens noch weiter optimieren, indem du aus der Multiplikation mit einem fortschreitenden Wert eine Addition machst:

...
    float pco, qco, tco;
    float cpco, cqco, ctco;
    RGBRawLEType* rgbp;
    float delta = 1.0f / 256;

    int y = 0;
    for (float saturation = 0.0f; y < 256; saturation += delta, ++y)
    {
        rgbp = static_cast <RGBRawLEType*> (bmpColor->ScanLine[y]);

        pco = 1 - saturation;
        qco = 1 - saturation * f;
        tco = 1 - saturation * (1 - f);
        cpco = cqco = ctco = 0.0f;

        for (int value = 0; value < 256; ++value, ++rgbp)
        {
            rgbp->*v = value;
            if (p)
            {
                rgbp->*p = cpco;
                cpco += pco;
            }
            if (q)
            {
                rgbp->*q = cqco;
                cqco += qco;
            }
            if (t)
            {
                rgbp->*t = ctco;
                ctco += tco;
            }
        }
    }
    ...

Tsurai

Puh.....also von meiner Rechnung ist da eigentlich nichts mehr übrig. Ich muss mir die HSV Berechnung noch mal genau angucken um den code erst einmal wieder komplett zu verstehen.
Könntest du mir den spontan veraten wo ich was ändern muss um das Bild um 90° gegen den Uhrzeigersinn zu drehen? Also so das bei Hue 360 bzw 0 das Rot oben rechts ist, Weiß oben links und nach unten hin es zum Schwarzen geht?

audacia

Tsurai schrieb:

Puh.....also von meiner Rechnung ist da eigentlich nichts mehr übrig.

Doch, die Rechnung ist im Grunde die gleiche.

Tsurai schrieb:

Könntest du mir den spontan veraten wo ich was ändern muss um das Bild um 90° gegen den Uhrzeigersinn zu drehen? Also so das bei Hue 360 bzw 0 das Rot oben rechts ist, Weiß oben links und nach unten hin es zum Schwarzen geht?

So ganz einfach ist das dann auch nicht, da wir als Vorgabe durch ScanLine haben, das Bild zeilenweise zu errechnen, weshalb zwei meiner Optimierungen (Koeffizienten in der äußeren Schleife berechnen sowie Addition anstatt Multiplikation) schon wieder hinfällig werden.

Hinreichend schnell ist es bei mir dennoch:

RGBRawLEType* rgbp;
    RGBRawLEType* rgbp_end;
    static const float delta = 1.0f / 256;

    int y = 255;
    for (int value = 0; value < 256; ++value, --y)
    {
        rgbp = static_cast <RGBRawLEType*> (bmpColor->ScanLine[y]);
        rgbp_end = rgbp + 256;

        for (float saturation = 0.0f; rgbp < rgbp_end; saturation += delta, ++rgbp)
        {
            rgbp->*v = value;
            if (p)
                rgbp->*p = value * (1 - saturation);
            if (q)
                rgbp->*q = value * (1 - saturation * f);
            if (t)
                rgbp->*t = value * (1 - saturation * (1 - f));
        }
    }

Tsurai

Danke dir audacia, funtkioniert wunderbar und ruckelt auch nicht.
Jetzt wo die grafische Darstellung fürs erste fertig ist müssen die Werte in RGB und HSV natürlich auf der Form auch angezeigt werden. Wenn ich das aber mit einbaue, auch wenn es nur der hue Wert ist und mehr nicht wird der Wert auch nur verspätet aktualisiert. "Ruckelt" also schon wieder.
Ich glaube das man dieses Problem lösen kann indem man threads verwendet.
Liege ich richtig mit meiner Vermutung?

Kolumbus

Tsurai schrieb:

...
Ich glaube das man dieses Problem lösen kann indem man threads verwendet.
Liege ich richtig mit meiner Vermutung?

Threads können so einige Performance-Probleme lösen... Ich versuche mich allerdings auch immer davor zu drücken Threads zu verwenden Ich glaube aber, dass es wie mit vielen Dingen beim Programmieren ist: Wenn man es ein paar Mal verwendet hat und die wichtigsten Tricks und Kniffe kennt, ist es so einfach wie Kaffee kochen.

Übrigens finde ich den Thread hier sehr interessant, da diese Optimierungsgeschichten prinzipiell für jeden Programmierer mal entscheidend werden können. => Gestern meinte man noch "die Rechner sind heut' schnell genug", heute hat man schon ein Performance-Problem...

audacia

Tsurai schrieb:

Jetzt wo die grafische Darstellung fürs erste fertig ist müssen die Werte in RGB und HSV natürlich auf der Form auch angezeigt werden. Wenn ich das aber mit einbaue, auch wenn es nur der hue Wert ist und mehr nicht wird der Wert auch nur verspätet aktualisiert. "Ruckelt" also schon wieder.

Es ruckelt nicht, sondern es wird nicht hinreichend oft aktualisiert. Rufe einfach TEdit::Repaint() auf, nachdem du den Wert veränderst.

Tsurai schrieb:

Ich glaube das man dieses Problem lösen kann indem man threads verwendet.

Nein

Tsurai

Kolumbus schrieb:

Tsurai schrieb:

...
Ich glaube das man dieses Problem lösen kann indem man threads verwendet.
Liege ich richtig mit meiner Vermutung?

Threads können so einige Performance-Probleme lösen... Ich versuche mich allerdings auch immer davor zu drücken Threads zu verwenden Ich glaube aber, dass es wie mit vielen Dingen beim Programmieren ist: Wenn man es ein paar Mal verwendet hat und die wichtigsten Tricks und Kniffe kennt, ist es so einfach wie Kaffee kochen.

Übrigens finde ich den Thread hier sehr interessant, da diese Optimierungsgeschichten prinzipiell für jeden Programmierer mal entscheidend werden können. => Gestern meinte man noch "die Rechner sind heut' schnell genug", heute hat man schon ein Performance-Problem...

Das dumme ist nur das ich in den Wochen in denen wir Threads in der Berufsschule hatten krank und deswegen war ich immer etwas zu faul mich damit zu beschäftigen und habe es immer vermieden sie zu benutzen. Jetzt glaube ich aber, dass ich keine andere Wahl mehr habe.

Performence Probleme hatte ich bisher auch noch nie. Mal abgesehen davon das bei der Mikrokontroller Programmierung der Prozessor zu schnell war und man ihn verlangsamern musste um LED Ausgaben anständig erkennen zu können.

Edit:
Ahh audacia, jetzt sehe ich erst das du etwas gepostet hast in der zeit in der ich am schreiben war.
Wenn das wirklich so einfach ist und keine Threads braucht wäre das wirklich gut. Obwohl ich Threads trotzdem mal lernen sollte

Kolumbus

Mein Indianername: "DerAufDemHolzwegWandelt"
Trotzdem ist es sinnvoll sich mit Threads zu beschäftigen!

Tsurai

Oh Wunder oh Wunder es ist ein weiteres Problem aufgetreten...(sollte ich mal solangsam ein neuen Thread machen oder diesen weiter benutzen weil es ja alles teil des Dialoges ist?)
Wie auch immer....die eigentliche Farbe soll man jetzt in dem großen Farbfeld auswählen können. Dafür hätte ich gerne einen anderen Cursor.
Ich habe bereits einen eigenen Cursor erstellt und ihn in die Project.res hinzugefügt. Mit folgendem Code versuche ich den Cursor des Bildes zu ändern:

const crMyCursor = 5;
Screen->Cursors[crMyCursor] = LoadCursor(HInstance, "Cursor1");
imgColorPalette->Cursor = TCursor(5);

Weil das nicht funktioniert hat habe ich einmal zum testen den Cursor manuel geändert, d.h. in dem Object Inspector von crDefault auf crDrag gestellt. Seltsamerweise bewirkt das aber auch nichts, der Cursor bleibt gleich. Bei dem Bild mit den Hue Werten kann ich aber über den Inspector den Cursor ganz normal verändern.
Könnte mir jemand sagen woran das liegen könnte?

mfg,

Tsurai

audacia

Tsurai schrieb:

Weil das nicht funktioniert hat habe ich einmal zum testen den Cursor manuel geändert, d.h. in dem Object Inspector von crDefault auf crDrag gestellt. Seltsamerweise bewirkt das aber auch nichts, der Cursor bleibt gleich.

Bei welcher Komponente hast du nun die Cursor-Eigenschaft verändert?
Wenn ich in meinem Beispiel ImgColorPalette->Cursor auf crCross (oder etwas anderes) setze, wird der entsprechende Cursor erwartungsgemäß zur Laufzeit angezeigt.