Optimieren, need help



  • Hi,

    ich habe eine Funktion die relativ groß ist, funktioniert aber auch nicht gerade schnell ist!

    Drum bräuchte ich etwas hilfe für Optimierung, aber erstmal die Funktion:

    void CBitmap::drawNormal (int x, int y, unsigned short *dest)
    {
    	assert (this->m_buffer ? true : false);
    
    	unsigned short	*source		= NULL;
    
    	int				runLength	= 0;
    	int				skipped		= 0;
    
    	int				realCount	= 0;
    	int				index		= 0;
    
    	int				srcLeft		= 0;
    	int				srcTop		= 0; 
    	int				srcRight	= 0;
    	int				srcBottom	= 0;
    	int				srcWidth	= 0; 
    
    	bool			clipped		= false;
    
    	if (x + this->m_width > g_screenWidth)
    	{
    		srcRight = g_screenWidth - x;
    		clipped = true;
    	}
    	else
    	{
    		srcRight = this->m_width;
    	}
    
    	if (y + this->m_height > g_screenHeight)
    	{
    		srcBottom = g_screenHeight - y;
    		clipped = true;
    	}
    	else
    	{
    		srcBottom = this->m_height;
    	}
    
    	if (x < 0)
    	{
    		srcLeft = -x; 
    		x = 0;
    		clipped = true;
    	}
    	else
    	{
    		srcLeft = 0 ;
    	}
    
    	if (y < 0)
    	{
    		srcTop = -y;
    		y = 0;
    		clipped = true;
    	}
    	else
    	{
    		srcTop = 0;
    	}
    
    	dest += x;
    	dest += y * g_screenWidth;
    
    	if (this->m_hasTransparentPixels)
    	{
    		if (clipped)
    		{
    			source		= this->m_buffer + srcLeft + (srcTop * this->m_width);
    			srcWidth	= srcRight - srcLeft;
    
    			for (int i=srcTop; i<srcBottom; ++i)
    			{  
    				for (int j=0; j<srcWidth; ++j)
    				{
    					if (source[j] != g_transColor)
    					{
    						dest[j] = source[j];
    					}
    				}
    
    				source	+= this->m_width;
    				dest	+= g_screenWidth;
    			}
    		}
    		else
    		{
    			source = this->m_rleBuffer;
    
    			realCount	= 0;
    			index		= 0;
    			int i		= 0;
    			while (i < this->m_height)
    			{
    				if (!source [index])
    				{
    					++index;
    					skipped = source [index];
    
    					dest		+= skipped;
    					realCount	+= skipped;
    					++index;
    				}
    				else
    				{
    					runLength = source [index];
    					++index;
    
    					for (int j=0; j<runLength; ++j)
    					{
    						dest[j] = source[index + j];
    					}
    
    					index		+= runLength;
    					realCount	+= runLength;
    
    					dest		+= runLength;
    				}
    
    				if (realCount == this->m_width)
    				{
    					realCount = 0;
    
    					dest += (g_screenWidth - this->m_width);
    					++i;
    				}
    			}
    		}
    	}
    	else
    	{
    		source = this->m_buffer;
    
    		if (clipped)
    		{
    			source	+= srcLeft + (srcTop * this->m_width);
    			srcWidth = srcRight - srcLeft;
    
    			for (int i=srcTop; i<srcBottom; ++i)
    			{  
    				for (int j=0; j<srcWidth; ++j)
    				{
    					dest[j] = source[j];
    				}
    
    				source	+= this->m_width;
    				dest	+= g_screenWidth;
    			}
    		}
    		else
    		{
    			for (int i=0; i<this->m_height; ++i)
    			{  
    				for (int j=0; j<this->m_width; ++j)
    				{
    					dest[j] = source[j];
    				}
    
    				source	+= this->m_width;
    				dest	+= g_screenWidth;
    			}
    		}
    	}
    }
    

    Gibt es ein paar Tipps wie ich diese Funktion beschleunigen kann?



  • ja. Sag was die Funktion macht, erkläre die Variablen. Denn nur wenn man die Funktion genau kennt, kann man anfangen zu optimieren.
    Da der Code imho nicht selbsterklärend ist hab ich keine lust, mich lang reinzuarbeiten.



  • Die Funktion schneidet einen internes array (unsigned short) an die jeweilige oberfläche zurecht (g_screenWidth, g_screenHeight) und stopft diesen zugeschnittenen bereich an den "dest"-pointer.



  • Ich hab jetzt nur die ersten drei Zeilen angeschaut und das reicht mir schon.

    assert (this->m_buffer ? true : false); // HÄÄÄÄÄÄÄÄÄÄÄÄÄÄ?!?!!
    

    Warum nicht assert(m_buffer) (vorzugsweise noch ohne hässliches m_ 🙄 )? Außerdem kannst du nichts über den Buffer aussagen, wenn du schaust ob der Zeiger darauf 0 ist. Oder setzt du den Zeiger garantiert auf 0, wenn er nicht mehr gültig ist?

    🙄 🙄 🙄

    CBitmap
    

    🙄 🙄 🙄

    Übrigens dein größtes Problem ist erst mal dein Programmierstil. Aber das ist ja normal bei allen, die gleich sofort mit der Spieleprogrammierung anfangen wollen.



  • @Optimizer
    ich habe assert neu definiert und assert erwartet nun einen bool! Nebenbei: ich mag keine warnungen wie "Kann unsigned short nicht zu bool konvertieren".

    lass das m_ mal schön da, damit seh ich wenigstens direkt was ein member ist und was nicht. Auch wenn ich kein Fan der Ungarischen Notation bin, das und g_ sind die einzigen die ich übernommen habe.

    das C hat nichts mit MFC zu tun sondern bedeutet in meinem Programm Conquer, weil die bibliothek so heißt.

    Was hab ich mit Spieleprogrammierung zu tun? Ich greife im Protected Mode per VBO3.0 direkt auf den VRam zu, stopf den in einen unsigned short (dest) und gebe mit dieser Klasse eine Bitmap auf den Monitor (vorzugsweise ist es nur ein 1D Array.)

    Also nun mal Vorurteile weg. Nebenbei: dein Programmierstil könnte mir auch nicht gefallen 😉



  • Optimizer schrieb:

    Aber das ist ja normal bei allen, die gleich sofort mit der Spieleprogrammierung anfangen wollen.

    wahre worte. 🙄
    Der CKlasse Stil ist wirklich nicht optimal.

    So wie ich das sehe, willst du eine Bitmap Clippen. Warum schneidest du den Array? Du kannst ja die üblichen Clipping-Algorithmen nutzen, welche evtl schneller sein werden. Da musst du natürlich die x-y Koordinaten der Bitmap benutzen, nicht die Offsets.



  • Ist this->m_buffer vom Typ unsigned short int ? Ich dachte es handelt sich um einen Puffer ? Der müsste vom Typ unsigned short int * sein... egal 😉 .
    Aber warum warnt dich der Compiler wegen der Umwandlung ? Es hat sowieso keinen Sinn, denn auch mit der Bedingen bewertung brauchst du bool.

    Expression ? _True : _False
    

    Expression selbst muss den Typ bool haben, oder zumindest in bool konvertierbar sein. Deine Konstruktion macht also doch keinen Sinn...



  • LEUTE! Jetzt lasst doch mal das scheiss C weg, was bei mir (zum 2. male) NICHT Class heißt!

    So und nun noch ne klarstellung: NIXE Windows, NIXE Linux, NIXE BeOS und co, eigenes OS im ProtectedMode und es wird VBO3.0 (VESA) benutzt, drum das 1D Array weil der GrafikSpeicher nunmal ein 1D Array ist! Drum liegt alles als 1D Array vor.

    Denn es gibt keine BITMAP Struktur oder windows.h oder sonst was. Punkt. und können wir uns nun endlich mal auf das eigentliche thema beschrenken!



  • ------------- schrieb:

    drum das 1D Array weil der GrafikSpeicher nunmal ein 1D Array ist! Drum liegt alles als 1D Array vor.

    Das ist hier ja jedem klar 🙄

    Denn es gibt keine BITMAP Struktur

    void CBitmap::drawNormal (...
    


  • *lol*, schon gut, nur nicht aufregen !!! 😃



  • ch habe assert neu definiert und assert erwartet nun einen bool!

    Dann schreib doch

    assert(m_buffer != 0)
    

    Was heißt denn jetzt dein C dann?



  • ------------ schrieb:

    ...

    das C hat nichts mit MFC zu tun sondern bedeutet in meinem Programm Conquer, weil die bibliothek so heißt.

    ...

    Ok das mit dem Assert stimmt das != 0 auch gehen würde, sonstige optimierungsvorschläge?



  • 1.die funktion in mehrere blöcke aufteilen, damit das nicht so ein riesen wurstteil ist,dann optimiert sich das schon wie von selbst

    2. lass das C ganz weg und mach

    namespace Conquer{
    }
    

    präfixe und ungarische notation ist tot

    und nochwas: wenn dua ssert neu definierst oder andre änderungen machst, dann musst du uns das mitteilen, wir können nix optimieren, was wir nicht kennen.

    //edit
    3. wieso willste das optimieren, hast du da geschwindigkeitseinbußen entdeckt?



  • Optimizer schrieb:

    ch habe assert neu definiert und assert erwartet nun einen bool!

    Dann schreib doch

    assert(m_buffer != 0)
    

    Das ist gar nicht mal noetig. Ein einfaches

    assert(m_buffer);
    

    reicht voellig aus, da der Zeiger automatisch in eine boolischen Wert konvertiert
    wird.

    mfg
    v R



  • neben den ganzen hässlich- und merkwürdigkeiten, die dein Programmierstil beinhält, ist das Hauptproblem für die Ineffizient die Verwendung von Schleifen und von byte (bzw. 2byte) weise Kopier vorgängen. Verwende memcpy oder so was, wenn es geht, da diese Funktion deutlich effizienter sein sollte, als jeder Schleife.

    Das wars, was ich auf den ersten Blick gesehen habe



  • virtuell Realisticer schrieb:

    Das ist gar nicht mal noetig. Ein einfaches

    assert(m_buffer);
    

    reicht voellig aus, da der Zeiger automatisch in eine boolischen Wert konvertiert
    wird.

    Jo, dachte ich eigentlich auch, das war auch mein erster Vorschlag. Irgendwie hat -------- dann gemeint, er hat es redefiniert und es frisst jetzt ein bool 😮 aha!?). Naja ich wollte des gar nicht so genau wissen, wieso und wie das assert redefiniert ist 🙄 .



  • Au weia, au weia, au weia. Ohne dir zu nahe treten zu wollen, aber der Code ist wirklich dreckig. Zur Performance komm ich nachher noch, aber erst mal Programmierstil allgemein - kapsel deinen verdammten Code. Du hast allein in dieser kleinen Funktion zig Stellen, an denen du den selben Code immer und immer wieder stehen hast. Du könntest die Code-Größe deutlich verringen (und auch ein bisschen speed rausholen), wenn du zum Beispiel erst auf clipping und danach auf transparente Pixel prüfst, denn so wie der Code im Moment aussieht, ist es dir, wenn clipping an ist, eh egal, ob du transparente Pixel hast. Jedenfalls ist der Code haargenau gleich. Und wenn ich das richtig sehe, ist der Code für nicht clipping und keine transparenten Pixel eigentlich auch genau der gleiche, nur dass du m_height und m_width fest gecodet hast, obwohl in diesem Fall srcBottom, srcTop usw. genau die entsprechenden Werte haben.

    Dann hast du einige völlig unnötige Zuweisungen - srcLeft und srcTop sind schon 0, das musst du ihnen nicht mehr zuweisen. Eigentlich kannst du srcRight und srcBottom auch gleich entsprechend initialisieren.

    Ansonsten - memcpy benutzen.

    Ohne das jetzt groß getestet zu haben (Ich hab im Moment keinen Compiler zur Hand, und den Rest deines Codes kenn ich ja auch nicht...), ich denke, das hier sollte das selbe tun wie deine Methode, und wenn nicht, sollte zumindest die Idee klar werden, so dass du was damit anfangen kannst.

    void CBitmap::drawNormal (int x, int y, unsigned short *dest)
    {
        assert (m_buffer);
    
        unsigned short *source = m_hasTransparentPixels ? m_rleBuffer : (m_buffer + srcLeft + srcTop * m_width);
    
        int srcLeft   = x < 0 ? -x : 0;
        int srcTop    = y < 0 ? -y : 0;
    
        dest += x + srcLeft + (y + srcTop) * g_screenWidth;
    
        if (m_hasTransparentPixels)
        {
            int realCount = 0;
            int index     = 0;
    
            for(int i = 0; i < m_height;) {
                if (!source [index]) {
                    ++index;
                    dest      += source[index];
                    realCount += source[index];
                    ++index;
                } else {
                    memcpy(dest, &source[index + 1], source[index] * sizeof(unsigned short));
    
                    realCount += source[index];
                    dest      += source[index];
                    index     += source[index] + 1;
                }
    
                if(realCount == m_width) {
                    realCount = 0;
                    dest += g_screenWidth - m_width;
                    ++i;
                }
            }
        } else {
            int srcRight  = x + m_width  > g_screenWidth  ? g_screenWidth  - x : m_width ;
            int srcBottom = y + m_height > g_screenHeight ? g_screenHeight - y : m_height;
    
            for (int i=srcTop; i<srcBottom; ++i) {
                memcpy(dest, src, (srcRight - srcLeft) * sizeof(unsigned short));
    
                source += m_width;
                dest   += g_screenWidth;
            }
        }
    }
    


  • Whoopsie, kleiner Fehler unterlaufen. also so:

    void CBitmap::drawNormal (int x, int y, unsigned short *dest)
    {
        assert (m_buffer);
    
        unsigned short *source = m_hasTransparentPixels ? m_rleBuffer : (m_buffer + srcLeft + srcTop * m_width);
    
        int srcLeft   = x < 0 ? -x : 0;
        int srcTop    = y < 0 ? -y : 0;
        int srcRight  = x + m_width  > g_screenWidth  ? g_screenWidth  - x : m_width ;
        int srcBottom = y + m_height > g_screenHeight ? g_screenHeight - y : m_height;
    
        int no_clipping = srcLeft | srcTop | srcRight | srcBottom;
    
        dest += x + srcLeft + (y + srcTop) * g_screenWidth;
    
        if (m_hasTransparentPixels && no_clipping)
        {
            int realCount = 0;
            int index     = 0;
    
            for(int i = 0; i < m_height;) {
                if (!source [index]) {
                    ++index;
                    dest      += source[index];
                    realCount += source[index];
                    ++index;
                } else {
                    memcpy(dest, &source[index + 1], source[index] * sizeof(unsigned short));
    
                    realCount += source[index];
                    dest      += source[index];
                    index     += source[index] + 1;
                }
    
                if(realCount == m_width) {
                    realCount = 0;
                    dest += g_screenWidth - m_width;
                    ++i;
                }
            }
        } else {
            for (int i=srcTop; i<srcBottom; ++i) {
                memcpy(dest, src, (srcRight - srcLeft) * sizeof(unsigned short));
    
                source += m_width;
                dest   += g_screenWidth;
            }
        }
    }
    


  • Wieso ich keinen namespace benutze und assert neu definiere und kein memcpy benutzen kann ist einfach: ich kann keine Standardlib benutzen? Wie denn auch wenn ich sie für meine Zwecke nicht mitlinken darf?

    @0xdeadbeef
    danke.

    @otze
    nur weil ich m_ und g_ benutze heißt das das ich die ganze Ungarische Notation benutze? Nö. nur 2 sachen die sich für mich persönlich durchgesetzt haben! Merk dir: es gibt nicht immer nur schwarz und weis sondern auch graustufen.



  • achja zum assert:
    CBitmap.cpp(244) : warning C4800: 'unsigned short *' : forcing value to bool 'true' or 'false' (performance warning)


Log in to reply