HSV Color Dialog



  • Hallo

    Außerdem sollte man noch überlegen ob der Compiler schon so schlau ist die Funktion HSVtoRGB in die entsprechenden Aufrufe zu inlinen (ich glaub alleine das switch sorgt dafür das es der Compiler nicht macht). Wenn nicht sollte man das noch tun, am besten indem man in diesem Fall wirklich auf eine extra Funktion verzichtet. Denn so werden bei jedem Durchgang jeweils eine Funktion aufgerufen und 9 Variablen auf dem Stack angelegt, die beim Beenden der Funktion wieder gelöscht werden.
    Wenn man die Berechnung wirklich aus der Funktion in die Schleife übernimmt, kann man aber die Variablen schon vor der Schleife deklarieren und das Programm muß sie nur einmal anlegen und löschen anstatt immer wieder für jeden Pixel nochmal.

    bis bald
    akari



  • Kolumbus schrieb:

    Hallo,

    Ohne jetzt tiefergehend in der Materie zu stecken: Kannst du nicht ein fertiges Bild nehmen, welches alle Farben enthält und bei Auswahl durch den Benutzer die Farbwerte auslesen?

    MfG

    Alle Farben die es gibt auf ein Bild zubekommen würde unmöglich sein weil es ja unglaublich viele sind.

    akari schrieb:

    schon für sich allein verbesserungswürdig

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

    Danke dir das du mir diesen Tipp gegeben hast aber viel ändert es in meinem Fall auch nicht 😕

    akari schrieb:

    Hallo

    Außerdem sollte man noch überlegen ob der Compiler schon so schlau ist die Funktion HSVtoRGB in die entsprechenden Aufrufe zu inlinen (ich glaub alleine das switch sorgt dafür das es der Compiler nicht macht). Wenn nicht sollte man das noch tun, am besten indem man in diesem Fall wirklich auf eine extra Funktion verzichtet. Denn so werden bei jedem Durchgang jeweils eine Funktion aufgerufen und 9 Variablen auf dem Stack angelegt, die beim Beenden der Funktion wieder gelöscht werden.
    Wenn man die Berechnung wirklich aus der Funktion in die Schleife übernimmt, kann man aber die Variablen schon vor der Schleife deklarieren und das Programm muß sie nur einmal anlegen und löschen anstatt immer wieder für jeden Pixel nochmal.

    bis bald
    akari

    Ich habe mich bisher nie so wirklich mit Speicherverwaltung beschäftigt ob das daher kommt das ich nur extrem selten mit purem C gearbeitet habe sei dahingestellt. Aufjedenfall konnte ich dir leider nicht ganz folgen. Ich werde das mal alles nachschlagen und mich schlau darüber machen ^^;

    audacia schrieb:

    Wenn du noch ein paar weitere Anmerkungen gestattest:

    • Gibt es einen Grund, weshalb dein Farbfeld nur 255 Pixel breit und hoch ist und nicht 256?

    Das wüsste ich allerdings auch gerne....sollte natürlich 256 sein.

    audacia schrieb:

    • Du mußt bmpColor->HandleType auf bmDIB setzen, bevor du ScanLine verwendest, sonst gibt es AVs (jedenfalls bei mir).

    AVs bekomme ich nur wenn ich die Größe vorher nicht festlege was ja auch klar ist, aber sonst eigentlich nicht.

    audacia schrieb:

    • Diese Zeilen in der inneren Schleife
    HSV.S = ((HSV.S / 255.0) * x);
             HSV.V = ((HSV.V / 255.0) * y);
    

    haben zur Folge, daß HSV.S und HSV.V immer 0 sind.

    Damit wollte ich eigentlich den Maßstab festlegen. Weil das Feld ja 256 pixel Groß ist die Werte aber nur von 0 bis 1.0 gehen. Muss mich wohl mit der Rechnung etwas vertan haben 😕

    Was haltet ihr den generell von dem algorithmus den ich benutzen wollte? Kann man das überhaupt in der Geschwindigkeit mit so einem algorithmus schaffen? Weil das gesamte Bild ja immer geändert werden muss sobald man den Schieberegler benutzt und das soll auch noch flüssig sein.

    mfg Tsurai



  • Hallo

    Ich habe mich bisher nie so wirklich mit Speicherverwaltung beschäftigt ob das daher kommt das ich nur extrem selten mit purem C gearbeitet habe sei dahingestellt. Aufjedenfall konnte ich dir leider nicht ganz folgen. Ich werde das mal alles nachschlagen und mich schlau darüber machen ^^;

    Das hat eigentlich weniger mit C zu tun, ich mein einfach so ein Umbau von

    RGBType HSVtoRGB( HSVType HSV )
    {
     float H = HSV.H, S = HSV.S, V = HSV.V, f, p, q, t;
     int hi;
     RGBType RGB;
    
      // deine Berechnung
    }
    
    void TForm1::change_color()
    {
     // ...
    
     for(int y = 0; y < 255; y++)
     {
         ptr = (Byte *)bmpColor->ScanLine[y];
         for(int x = 0; x < 255; x++)
         {
    
             // ...
             RGB = HSVtoRGB(HSV);
             // ...
         }
     }
    
      // ...
    }
    

    in die optimiertere Version :

    void TForm1::change_color()
    {
     // ...
    
     float H, S, V, f, p, q, t;
     int hi;
     RGBType RGB;
    
     for(int y = 0; y < 255; y++)
     {
         ptr = (Byte *)bmpColor->ScanLine[y];
         for(int x = 0; x < 255; x++)
         {
    
             // ...
             H = HSV.H;
             S = HSV.S;
             V = HSV.V;
             // deine Berechnung
             // ...
         }
     }
    
      // ...
    }
    

    das wird dir auch helfen die Rechnung weiter zu vereinfachen, wenn du einmalige Multiplikation in aufeinanderfolgende Additionen umwandelst.

    bis bald
    akari



  • 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.



  • 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).



  • 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;
    
    }
    


  • 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 hi 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



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



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



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



  • Welche C++Builder-Version?



  • 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



  • Ich benutze den Borland C++ Builder 6.0 Enterprise



  • 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;
                }
            }
        }
        ...
    


  • 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?



  • 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));
            }
        }
    


  • 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?



  • 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...



  • 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 😉



  • 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 😉


Anmelden zum Antworten