Du bist nicht angemeldet.

Stilllegung des Forums
Das Forum wurde am 05.06.2023 nach über 20 Jahren stillgelegt (weitere Informationen und ein kleiner Rückblick).
Registrierungen, Anmeldungen und Postings sind nicht mehr möglich. Öffentliche Inhalte sind weiterhin zugänglich.
Das Team von spieleprogrammierer.de bedankt sich bei der Community für die vielen schönen Jahre.
Wenn du eine deutschsprachige Spieleentwickler-Community suchst, schau doch mal im Discord und auf ZFX vorbei!

code review

Werbeanzeige

idontknow

unregistriert

1

06.02.2013, 01:26

code review

moin :)

Ich dachte mir ich poste einfach mal nen Link zum Code meines aktuellen "Projekts" wenn mans denn so nennen möchte (irgendwann einmal vllt ein kleines feines spiel) in der hoffnung, dass 1-2 leute sich vielleicht die zeit nehmen rein zu kucken und Tipps/Hinweise etc. geben was man verbessern könnte hinsichtlich der Code Qualität.

Ziel ist mit dem Projekt endlich mal einigermaßen konsequente Fehlerbehandlung und gute Codequalität als auch ein einigermaßen sinnvolles Design durchzuziehen.

Repository: http://code.google.com/p/yetanothergame/source/checkout

Grundsätzlich steht alles zur Debatte was ich da gemacht habe, bzw ich bin bereit über alles (also auch Klassendesign und Beziehungen) zu diskutieren bei dem meisten hab ich mir einigermaßen Gedanken dazu gemacht bei manchen Sachen ist mir aber auch einfach keine besser Lösung eingefallen (z.b. MapBuilder und das Laden der Texturen)

Im Repository ist der reine Code und die paar Ressourcen die er verwendet (2 Bilder und 2 minimalistische Shader) und so kompiliert er zumindest bei mir. Verwendet wird DirectX11, FreeImage und rapidxml.

Momentan macht der Code leider nochn ocht soviel im aktuellen Zustand lieset er 2 Polygone ein und klatscht ne Textur drauf das wars. Werd da aber natürlich soferns die Zeit erlaubt fleißig weiter bauen!


Würde mich freuen paar Verbesserungsvorschläge zu bekommen!

David_pb

Community-Fossil

Beiträge: 3 886

Beruf: 3D Graphics Programmer

  • Private Nachricht senden

2

06.02.2013, 09:15

Ein paar Dinge dir mir beim überfliegen am Stil aufgefallen sind:

o Du verwendest gern "Fette" Konstruktoren (Device, Window, ...). Eigentlich, auch wenn da sicherlich viele Einspruch erheben, sollte der Konstruktor möglichst schlank bleiben und keine "schwere" Logik ausführen. Der Konstruktor sollte das Objekt in einen korrekt initialisierten Zustand bringen, sonst nichts.

o Bei Konstruktoren mit einem Parameter immer "explicit" verwenden, falls kein implizites Casten erlaubt sein soll (Window, VertexShaderBinary, ...)

o Null Kommentare vorhanden

o Deine Interfaces wirken eher pragmatisch und nicht logisch (VertexShaderBinary::CreateShader, VertexShaderBinary::CreateInputLayout)

o Kleine, unnötige Methoden (InitializeDeviceContext)

o union-Vektor
@D13_Dreinig

idontknow

unregistriert

3

06.02.2013, 09:59

Erstmal Danke, dass du dir den Code angeschaut hast :). Zu deinen Punkten:

Zitat von »David_pb«

o Du verwendest gern "Fette" Konstruktoren (Device, Window, ...). Eigentlich, auch wenn da sicherlich viele Einspruch
erheben, sollte der Konstruktor möglichst schlank bleiben und keine "schwere" Logik ausführen. Der Konstruktor sollte das Objekt in einen korrekt initialisierten Zustand bringen, sonst nichts.

Zugegebenermaßen die Konstruktoren sind Teils schon ziemlich groß, dabei macht auch das Device mehr als ein Device eigentlich tun sollte. Ich will aber auch verhindern, dass das ganze in Richtung Framework/Engine abdriftet - nen Spiel soll dabei raus kommen und nichts anderes. Deswegen hab ich einfach alles in eine Klasse reingeklatscht, aber hast schon recht ich werd mir mal Gedanken machen wie ich das aufteilen kann.
Wegen dem Konstruktor von Window: der ist ja eigentlich ziemlich schlank, tut nichts außer das Window Objekt zu erstellen.

Beim auftennen von Device und SwapChain wäre auch das Problem, dass ich zum erstellen ja die selbe Factory brauche und dann aus dem Device die Factory wiederherstellen müsste was ich ursprünglich verworfen habe weil der Code dazu extrem eklig war :/.

Zitat von »David_pb«

o Bei Konstruktoren mit einem Parameter immer "explicit" verwenden, falls kein implizites Casten erlaubt sein soll
(Window, VertexShaderBinary, ...)

Ok :).

Zitat von »David_pb«

o Null Kommentare vorhanden

Das stimmt leider, so fleißig bin ich da noch nicht.. finde es aber auch etwas kompliziert zu entscheiden was jetzt intuitiv klar ist und was nicht.

Zitat von »David_pb«

o Deine Interfaces wirken eher pragmatisch und nicht logisch (VertexShaderBinary::CreateShader, VertexShaderBinary::CreateInputLayout)

Okey das verstehe ich nicht, kannst du mal genauer darauf eingehen?

Zitat von »David_pb«

o Kleine, unnötige Methoden (InitializeDeviceContext)

Ja das hängt mit den fetten Konstruktoren zusammen, würde dann auch mit raus fliegen.

Zitat von »David_pb«

o union-Vektor

Das versteh ich auch nicht was ist denn schlecht daran? Ich hab hauptsächlich den Vorteilg esehen, dass ich die meisten Funktoinen und Operatoren in einem File für alle Vektoren deklarieren kann.

SupremeDeveloper

Alter Hase

Beiträge: 721

Wohnort: /dev/null

Beruf: Software-Entwickler/Nerd

  • Private Nachricht senden

4

06.02.2013, 10:07

* Die Device-Klasse sieht ziemlich sinnlos aus. Die Klasse erstellt N Ressourcen und bietet einfach nur Getter darauf. Wozu die Getter? Und wozu überhaupt die Klasse? Man könnte die Ressourcen per shared_ptr relativ einfach durch deine Klassenstruktur reichen, ohne die Speicherverwaltung zu verkomplizieren. Ich würde jede Ressource des Devices in einem kleinen Struct wrappen, welches einfach nur die minimale Initialisierung in einen nutzbaren Zustand, bzw. das Löschen der Ressource sicherstellt.

* Um Portierbarkeit zu ermöglichen würde ich alle nativen Interfaces aus den bereitgestellten APIs entfernen. Wie gesagt, ein minimaler Wrapper wäre an der Stelle sicherlich angebracht.

* Renderer::RenderTriangle(): Willst du jedes mal die Textur anhand eines Strings aus einer Liste laden und dann rendern? Alternativ könntes du mit IDs bzw. Instanzen auf die Textur arbeiten. Wäre vielleicht sinnvoller.

* In der LoadTexture()-Funktion sind implizite Casts von signed auf unsigned, jedenfalls wenn ich die API richtig in Erinnerung habe(FreeImage_GetWidth/GetHeight).

* Renderer::m_StartCalled ist imho wegzulassen. Wenn man RenderTriangle aufruft um etwas zu rendern, sollte klar sein, dass man vorher auch Start/EndRender aufrufen sollte.

* Die Ressourcen-Erstellung ist sehr vielschichtig. Findet man sowohl im Backend(Device,Renderer), als auch im Frontend(Game). Eine klare Trennung wäre logisch wesentlich besser nachzuvollziehen.
Bildbearbeitungs-Software
Black Silk Version 0.3.0

idontknow

unregistriert

5

06.02.2013, 10:25

Okey auch dir Danke für das Feedback.

Das Device werd ich auftrennen bin da schon dran.
Die Texturen über Strings zu identifizieren ist echt nicht so optimal.. muss ich mir was überlegen denen direkt eine ID zu verpassen. Die signed/unsigned Casts könnte gut sein, werd ich wegmachen.
Das m_Start.. braucht man eigentlich ehh nicht mehr. Rendern geht ejtzt nur noch über die SpriteBatch-Klasse die intern Start/End. Einfach nen popliges Scoped Objekt.

Wegen den Ressourcen muss ich zugeben, dass das teils etwas "hingeklatscht" ist werde das wenn ich Zeit finde versuchen zu verbessern.

Werbeanzeige

Ähnliche Themen