Verbesserungsvorschläge für eine schlimme Funketion

ChrisJ

warum sollte man denn keine referenezen verwenden?

finix schrieb:

Ohne mir das so genau angesehen zu haben:

namespace sdl?

keine Magic Numbers verwenden

if/else -bodies in braces stecken wenn's anderweitig nicht eindeutig ist

" else ; "? wtf?

vec2d könnte hilfreich sein

eine Funktion test_intersection( circle, rect ) wäre sicher nicht unpraktisch

Google: collision response 2d

$THREADTITEL -> "2D Kollision" o.ä.

1. Was ist den an dem namespace falsch?
2. Ich finde es eindeutig, aber ich kann es ja umformatieren.
5. habe ich, aber ich muss wissen, von wo das Objekt getroffenm wird, weil dementsprechend der Ball geändert werden muss

lolz schrieb:

Benutze bitte Referenzen und schreib nicht x mal pro Zeile ball.getRect()->, zumindest in dem Code den du hier postest.

EDIT: Hier habe ich mich verlesen. Sorry. Wie bereits geschrieben der Code soll es nur verdeutlichen

chrische

ChrisJ schrieb:

warum sollte man denn keine referenezen verwenden?

sollte man doch. u.a. auch weil es viel langsamer ist, jedes mal eine methode aufzurufen anstatt direkt mit dem objekt zu arbeiten. wobei ich mir vorstellen könnte, dass compiler das wegoptimieren (könnten).

@chrische: wie verschachtelst du denn deine namespaces? chrische::sdl::algorithm::irgendwas . sollte für deine 2d engine / framework ein namespace nicht genügen, um namenskollisionen zu vermeiden?

edit: paar unsinnige fehler rauseditiert

Hallo

Du meinst mit den magic numbers die Rückgabewerte? Sorry:

Treffer von unten: 1
Treffer von oben: 2
Treffer von rechts: 3
Treffer von links: 4
keine Treffer: 0

chrische

this-*gt*that

Dafür gibts enums.

BTW find ich deine Bezeichnernamen mit den Unterstrichen ziemlich grausig.

collison_between_ball_and_stone <- wtf. Wieso nicht einfach collisionBallStone
ball.get_speed_y(): ball.speedY() usw.

Hallo

Ich freue mich über jede Kritik, aber ich suche da eher was Grundlegenderes. Einen anderen Ansatz oder Ähnliches. Bezeichnernamen sind nun mal Geschmackssache udn ich mag es so.

chrische

chrische5 schrieb:

Hallo

Ich freue mich über jede Kritik, aber ich suche da eher was Grundlegenderes. Einen anderen Ansatz oder Ähnliches. Bezeichnernamen sind nun mal Geschmackssache udn ich mag es so.

chrische

zu den namen:
anstatt irgendwann immer nur zu schreiben -> collision_between_spaceship_and_planet(...);

namespace Collision
{
   bool ball_stone(SDL_rect a,SDL_rect b);
}

Don06

Bis man den Algorithmus versteht dauert es wohl ziemlich lange.
Ich hab mal ein bisschen dran rumgewerkelt, nicht am Algorithmus sondern am Code:

enum Treffer
{
	NEIN,
	UNTEN,
	OBEN,
	RECHTS,
	LINKS
};

typedef std::list<SDL_Rect> RectList;

Treffer chrische::sdl::algorithm::collison_between_ball_and_stone(chrische::ball& ball) 
{  
	const crische::Data& data = crische::get_data();

    for(RectList::iterator cur = data.all_rects.begin();
		cur != data.all_rects.end(); ++cur)
		//in all_rects befinden sich alle Kollisionrechtecke der einzelnen Steine 
	{
		const SDL_Rect& ballRect = ball.get_rect();

        if(ball.get_speed_y() > 0) 
		{
            if(ball.get_speed_x() > 0) 
                if(ballRect.x + ballRect.w > cur->x && ballRect.y < cur->y + cur->h) 
				{
                    if(((ballRect.x + ballRect.w) - cur->x) < ((cur->y + cur->h) - ballRect.y)) 
                        return LINKS;
                    else 
                        return UNTEN;
				}
            else 
                if((cur->x + cur->w) > ballRect.x && (cur->y + cur->h) > ballRect.y) 
				{
                    if((cur->y + cur->h) - ballRect.y > (cur->x + cur->w) - ballRect.x) 
                        return RECHTS 
                    else 
						return UNTEN;
				}
		} 
	}

	return NEIN;
}

Schafft ein bisschen mehr Übersicht, findest du nicht? Mir fällt da sofort auf das es anschienend keine Kollisionsabfrage nach oben hin gibt. Ist das beabsichtigt, oder ein Fehler, der aus den Magic-Numbers resultiert?

Was passiert, wenn der Ball mehrere Steine berührt? nur eine Kollision wird zurückgegeben. In deinem Code wahrscheinlich nur die letzte, in meinem nur die erste.

Gruß
Don06

hustbaer

this->that schrieb:

BTW find ich deine Bezeichnernamen mit den Unterstrichen ziemlich grausig.

collison_between_ball_and_stone <- wtf. Wieso nicht einfach collisionBallStone
ball.get_speed_y(): ball.speedY() usw.

Ja, und ich finde sie schön. Und laut_statistik_sind_solche_bezeichner_besser zuLesenAlsoSoDoofeCamelCaseDinger.

p.S.: Der Vorteil der CamelCase Variante ist dagegen dass man schneller die Grenzen zwischen Bezeichnern sieht wenn diese nur durch ein Leerzeichen getrennt sind.

p.p.S.: ball.speed().x

Hallo

Don06 schrieb:

Bis man den Algorithmus versteht dauert es wohl ziemlich lange.
Ich hab mal ein bisschen dran rumgewerkelt, nicht am Algorithmus sondern am Code:
enum Treffer
{
	NEIN,
	UNTEN,
	OBEN,
	RECHTS,
	LINKS
};

typedef std::list<SDL_Rect> RectList;

Treffer chrische::sdl::algorithm::collison_between_ball_and_stone(chrische::ball& ball) 
{  
	const crische::Data& data = crische::get_data();

    for(RectList::iterator cur = data.all_rects.begin();
		cur != data.all_rects.end(); ++cur)
		//in all_rects befinden sich alle Kollisionrechtecke der einzelnen Steine 
	{
		const SDL_Rect& ballRect = ball.get_rect();

        if(ball.get_speed_y() > 0) 
		{
            if(ball.get_speed_x() > 0) 
                if(ballRect.x + ballRect.w > cur->x && ballRect.y < cur->y + cur->h) 
				{
                    if(((ballRect.x + ballRect.w) - cur->x) < ((cur->y + cur->h) - ballRect.y)) 
                        return LINKS;
                    else 
                        return UNTEN;
				}
            else 
                if((cur->x + cur->w) > ballRect.x && (cur->y + cur->h) > ballRect.y) 
				{
                    if((cur->y + cur->h) - ballRect.y > (cur->x + cur->w) - ballRect.x) 
                        return RECHTS 
                    else 
						return UNTEN;
				}
		} 
	}

	return NEIN;
}
Schafft ein bisschen mehr Übersicht, findest du nicht? Mir fällt da sofort auf das es anschienend keine Kollisionsabfrage nach oben hin gibt. Ist das beabsichtigt, oder ein Fehler, der aus den Magic-Numbers resultiert?

Was passiert, wenn der Ball mehrere Steine berührt? nur eine Kollision wird zurückgegeben. In deinem Code wahrscheinlich nur die letzte, in meinem nur die erste.

Gruß
Don06

Erstmal vielen Dank. Treffer von oben habe ich noch nicht abgefragt, weil ich mit der Funktion einfach sehr unzufrieden bin und den Ansatz für ungut hielt, aber anscheinend habt ihr auch keine bessere Idee.

Ich erklär mal kurz, was ich mir gedacht habe:
1. alle Steine werden durchlaufen
2. es wird geprüft, ob speed_y > 0 ist, also ob der sich von unten nach oben bewegt
3. es wird geprüft, ob sich der Ball von rechts nach links bewegt (speed_x > 0) (jetzt kann der Ball nur von unten oder rechts kommen)
4. dann wird geprüft, ob die rechte obere Ecke des Ball sich innerhalb des Steines befindet
5. dann wird geprüft, ob die Differenz zwischen rechter Seite des Ball und linker Seite des Steines größer ist als die Differenz zwischen oberer Seite des Balles und der unteren Seite des Steines, weil dann der Ball von rechts kommen muss.

Das kann es doch aber nicht sein!?

chrische

Don06

crische5 schrieb:

1. alle Steine werden durchlaufen

Vorschlag: Du kennst die Position des Balles. Du kennst die Position der Steine. Folglich musst du nur die in Frage kommenden Steine überprüfen.

Beispiel: Ball befindet sich zwischen 2. und 3. Reihe und 4. und 5. Spalte. Du musst nur die Steine (2/4), (2/5), (3/4), (3/5) überprüfen. Eventuell musst du dann die Steinstruktur noch einmal überdenken (Array statt Liste).

Gruß
Don06

Blue-Tiger

hustbaer schrieb:

Ja, und ich finde sie schön. Und laut_statistik_sind_solche_bezeichner_besser zuLesenAlsoSoDoofeCamelCaseDinger.

Hast du zufaellig die Quelle zu der Statistik noch?

Hallo

Wie wäre denn euer Ansatz zu prüfen, von welcher Seite ein Stein getroffen wurde? letzte Position speichern, so wie ich oder noch ganz anders?

chrische

Mr. N

Ich habs mal ein wenig aufgeräumt. Sieht doch gleich viel besser aus.

enum Hit
{
    NO,
    BOTTOM,
    UP,
    LEFT,
    RIGHT
};

typedef std::list<SDL_Rect> RectList;

Hit chrische::sdl::algorithm::collison_between_ball_and_stone(chrische::ball& ball) {  
    const crische::Data& data = crische::get_data();

    for(RectList::iterator cur = data.all_rects.begin();
        cur != data.all_rects.end(); ++cur)
        //in all_rects befinden sich alle Kollisionrechtecke der einzelnen Steine 
    {
        const SDL_Rect& ballRect = ball.get_rect();
        const SDL_Rect& stoneRect = *cur;

        int ballLeft = ballRect.x;
        int ballRight = ballRect.x + ballRect.w;
        int ballTop = ballRect.y;

        int stoneLeft = stoneRect.x;
        int stoneRight = stoneRect.x + stoneRect.w;
        int stoneBottom = stoneRect.y + stoneRect.h;

        if(ball.get_speed_y() > 0) {
            if(ball.get_speed_x() > 0) {
                if(ballRight > stoneLeft && ballTop < stoneBottom) {
                    if((ballRight - stoneLeft) < (stoneBottom - ballTop)) 
                        return LEFT;
                    else 
                        return BOTTOM;
                }
            } else {
                if(stoneRight > ballLeft && stoneBottom > ballTop) {
                    if((stoneBottom - ballTop) > (stoneRight - ballLeft)) 
                        return RIGHT; 
                    else 
                        return BOTTOM;
                }
           }
        } 
    }

    return NO;
}