Jump to content
Unity Insider Forum

Codeüberprüfung und Feedback gesucht!


Kokujou

Recommended Posts

Hallo~ Ich wende mich heute mit einer ziemlich großen Bitte an euch. Ich programmier ja schon eine ganze Weile aber würde von mir nie behaupten dass ich... gut bin. Oder ... brauchbar, ich glaube in meinen Codes sind viele Probleme in allen möglichen Abteilungen und warscheinlich ist vieles was ich tue ja auch einfach ein schlechter Stil.

Das Blöde ist ich habe einfach niemanden der mir mal sagt was dumm gemacht ist und was ich besser  machen kann, und da ich beabsichtige ein Projekt auch für mobile Plattformen zu machen können solche Fehler nun doch ins Gewicht fallen, besonders wenns ums Thema performance geht.

Das Projekt ist natürlich noch nicht fertig, aber es wär schön wenn mal jemand drüber gucken könnte. Ich hab das ganze Projekt auf Google Drive hochgeladen, es wäre schön wenn sich jemand findet der viel Zeit hat und mal an allem rummeckern könnte was absolut mist ist^^

Eins Vorweg: Bei der Multiplayer Komponente geh ich ziemlich auf Low-Level, weil das mit den syncvar irgendwie einfach gar nicht funktioniert, also hab ich mir damit geholfen, da ich so wenigstens abfangen kann was wo ankommt oder aber auch nicht, bei SyncVar sieht man ja nur klappt oder klappt nicht.

https://drive.google.com/file/d/1sM8_ED-j0MjAz87MdKz9DhbZCcGXIjXW/view?usp=sharing

PS: Ich weiß natürlich dass das sehr viel verlangt ist, aber vielleicht hat ja trotzdem mal jemand Zeit und Muße einem Neuling die Leviten zu lesen^^

________________________________________________________________________
So für alle die neu dazukommen hier der Link in GitHub-Form, muss ja niemand nen halben GB runterladen!

https://github.com/kokujou/hanafuda

Wär schön wenn ihr, falls es Zeit und Lust erlaubt, mal drüber guckt und bei den neusten Änderungen seht ob ich jetzt totalen Pfusch verzapfe oder ob ich das Grundprinzip jetzt verstanden habe.

Link zu diesem Kommentar
Auf anderen Seiten teilen

  • Antworten 78
  • Created
  • Letzte Antwort

Ich habe nur mal kurz reingeschaut, was mir aber aufgefallen ist bezüglich Code Qualität:

Die Scripts sind zum Teil unübersichtlich. Vor allem die AI.cs. Die Update() ist über 400 Zeilen lang, hier würde das aufsplitten in mehrere Methoden alles etwas übersichtlicher machen und das unschöne goto wäre nicht nötig.

Die Global ist ein Sammelsurium aus Settings und anderen Statischen Funktionen. Eine logische Trennung wäre grundsätzlich anbracht, habe keine Angst mehrere Dateien Anzulegen.

Keine einheitliche Namensgebung von Feldern, mal camelCase, mal PascalCase
Öffentliche (dazu gehört auch protected) Felder und properties werden in der Regel PascalCase geschrieben, Unity hält sich mittlerweile bei neuen Sachen z.B. ECS selbst daran (meistens zumindest). Für private Felder und properties gibt es keine Namingconvention, allerdings sollten sie im Projekt konsistent sein.

Link zu diesem Kommentar
Auf anderen Seiten teilen

Ja dass  ich Dateien vollpacke ist schon viel besser geworden. Die GUIMobile, Global etc war für mich schon ein Fortschritt ^^ die GUI-Funktion hab ich auch schon Abgedockt, aber andererseits ist die Update() nunmal eine sehr umfangreiche Funktion die alle Input-Funktionen handhabt. Aber ich arbeite weiter dran. Das ist aber auch weniger ein Qualitätsmanko als eher in Sachen Übersichtlichkeit, schließlich macht es keinen Unterschied im Endergebnis

In der Global habe ich Funktionen versammelt die von mehreren Skripten aufgerufen werden, besonders die statische Settings-Klasse in der die Grundeinstellungen eines Spiels liegen.

Was du in dem letzten Abschnitt sagst... ich hab keine Ahnung... erst dachte ich du meintest iwas mit switch/case aber du redest wohl von Groß/Kleinschreibung? was ist camelcase? hast du mal Beispiele? und was ist ECS? Auf sowas hab ich wirklich noch nie geachtet solange der Name vom Inhalt irgendwie stimmt^^ ich versuch mir gerade die for(int i [...]) abzugewöhnen und da sinnvolle variablen zu verwenden^^

 

Aber was mich wirklich interessieren würde wären halt Fehler die sich auf die Performance auswirken, oder stellen die sich besonders verkürzen lassen. Früher hab ich z.B. nichtmal for-schleifen gekannt, ich hab nie ein Buch über Programmierung gelesen und alles über if/else geregelt, das sah vielleicht schrecklich aus XD

Link zu diesem Kommentar
Auf anderen Seiten teilen

Mich hat es ein wenig gewundert, dass du die alte OnGUI verwendest und nicht die neuere Unity UI. 2019 kommt mit UIElements auch schon die nächste UI auf XML und CSS basis.

Gerade wenn eine Funktion sehr umfangreich ist, ist es besser sie aufzuteilen, oft wiederholen sich bestimmte Codeblöcke die man so wiederverwenden kann und man hat dann auch wenig code.

Die Settings könnte man als ScriptableObject anlegen. 
Es spricht nichts gegen Statische Helfer-Klassen, aber man sollte sie der Übersichtlichkeit Logisch strukturieren, siehe die Math Klasse aus .NET (Mathf float variante in Unity)  Spezifische Sachen haben aber wiederum nichts in einer Global statische klasse zu suchen wie z.B Global.HoverCard, pack die Logik dort hin wo sie hin gehört.

Zur Performance kann ich momentan nicht viel sagen, aber ich glaube hier gibt es noch sehr viel Spielraum nach oben. Du solltest z.B. alle "GameObject.Find()" aus der Update() rausbekommen. Da solltest du mal dein Spiel mit dem Profiler anschauen.

vor einer Stunde schrieb Kokujou:

Was du in dem letzten Abschnitt sagst... ich hab keine Ahnung... erst dachte ich du meintest iwas mit switch/case aber du redest wohl von Groß/Kleinschreibung?

Siehe https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/names-of-type-members#names-of-fields

ECS steht für Entity Component System, und ist eine neue Art um Gamecode in Unity zu programmieren. Befindet sich aber noch in Preview.

Link zu diesem Kommentar
Auf anderen Seiten teilen

warte warte... noch neuer als die als GameObect? Canvas? Ich hab mein Projekt vor Monaten angefangen, da war ich noch auf älterem Stand, darum verwende ich aktuell ne Kombo aus dem Canvas und der IMGUI weil die sich gut macht, wenn man nen Screen Overlay verschieben will

Ich hab schon versucht wichtige Funktionen abzudocken, zwar nicht in neue Scripte weil ich das unsinnig finde und das nur den Koordinationsaufwand erhöht.
Ein aktuelles Problem ist auch dass ich das Skript spawnen muss und ich deswegen die variablen über Find zuweisen muss statt sie im Eidtor zuzuweisen. Entweder das oder ich hätte nen riesen Codeaufwand und müsste in der Spawn.cs für jede zugewiesene Variable ne Codezeile schreiben... nervig und unschön..

ScriptableObject? Hilf mir auf die Sprünge~

Die statischen Funktionen sind in der Global weil sie von mehreren Stellen verwendet werden, oder ursprünglich verwendet werden sollten. die hoverCard oder ResortCard und diverse andere, auf die werden z.B. von den neuen GUI Scripten von der Slidecard etc zugegriffen darum wollte ich sie in ne zentrale Klasse abdocken und darum mussten sie natürlich auch statisch sein

gut den Link seh ich mir mal an... zur Find hab ich ja schon was gesagt und alles was Resources.Load angeht wollte ich auch noch rauswerfen das ist ein guter Tipp.

Was mich noch interessieren würde wären Anmerkungen zum Networking-System da ich da auch noch sehr unsicher bin und vielleicht noch etwas zum Thema mobil? da gibts sicher viele Dinge zu beachten oder? Ich mag Kritik aber in meiner ... sie nennen es "Hochschule" kennt sich keine Sau mit irgendwas aus >.>

Ich hab grad mal die ScriptableObject-doku überflogen aber es wär hilfreich wenn du es nochmal ganz kurz mit eigenen Worten zusammenfasst damit ich sehe ob ich das richtig verstanden habe.

Link zu diesem Kommentar
Auf anderen Seiten teilen

Ich bekomme übrigens bei spielen die Meldung beim Zug der AI:

Zitat

NullReferenceException: Object reference not set to an instance of an object
AI.Update () (at Assets/Resources/Scripts/AI.cs:593)

 

Zum Networking und Mobile muss jemand anderes was dazu sage, da habe ich keine Erfahrung darin.

vor 11 Stunden schrieb Kokujou:

Ich hab schon versucht wichtige Funktionen abzudocken, zwar nicht in neue Scripte weil ich das unsinnig finde und das nur den Koordinationsaufwand erhöht.

Das ist nicht unsinnig, und der Koordinationsaufwand wird irgendwann auf diese Weise sogar größer. Wenn das Spiel wächst und du mehr Funktionen einbaust, wird es immer unübersichtlicher und wenn du mal nach einigen Wochen Abstinenz mal wieder in der Code zurück kehrst, dann kennst du dich selbst nicht mehr aus. Die Ausrede "es ist nur ein kleine Projekt" zählt nicht, da man sich sonst nie einen sauberen Programmierstil angewöhnt.
Pack bitter generell nicht mehrere Klassen in eine Datei. Eine Datei pro Klasse. Es gibt Order und Namespaces.

 

vor 11 Stunden schrieb Kokujou:

Ein aktuelles Problem ist auch dass ich das Skript spawnen muss und ich deswegen die variablen über Find zuweisen muss statt sie im Eidtor zuzuweisen. Entweder das oder ich hätte nen riesen Codeaufwand und müsste in der Spawn.cs für jede zugewiesene Variable ne Codezeile schreiben... nervig und unschön..

Ich habe momentan nicht die Zeit den genau den Workflow deines Spiele zu analysieren, aber wenn du variablen über Find zuweisen musst, dann hast du schon ein Konzeptionelles Problem.

Mit ScriptableObject kannst du dir als Asset im Asset-Ordner erstellen sie wie Prefabs einer Komponente zuweisen. Du könntest dir deine Karten hier definieren, anstatt Hardgecodet. So könnest du auch Variationen mit anderen Motiven erstellen.

Im Spiel "Tales of Berseria" gibt es ein Hanafuda Minispiel mit Charakteren auf vorherigen Spiele der Reihe als Motive.

Link zu diesem Kommentar
Auf anderen Seiten teilen

Ja die AI ist nur für den Alptraum-Modus aktiv, es ist ja noch nicht fertig, für den Alptraum-Modus hab ich also ne "ziehe einfach die erste Karte" AI entwickelt^^

Nagut, da hast du eigentlich recht, ich werde also mal etwas mehr abdocken.

Oho Tales of Berseria hab ich sogar mal angespielt bin aber soweit nicht gekommen.

Ist auf jeden fall höchst interessant dieses ScriptableObject funtkioniert das zur Laufzeit? Also wenn man das Spiel beendet, kann man die dann wiederherstellen wie ne Art Speicherdatei? Oder verschwinden die wenn das Programm beendet wird?

Ich bin mir nicht sicher wie ich das anwenden kann, aber es ist ein interessanter Ansatz

Achja ich hab noch eins vergessen: Beim Networking darf man das NetworkBehaviour nicht einfach so in die Szene reinschmeißen. Man muss es über ein anderes Skript instanziieren. Das Problem dabei ist aber, dass man so nicht mehr aktive GameObjects in der Szene zuweisen kann, wie man es sonst machen würde wenn das Script auch von Anfang an da war. Da es gespawnt wird existiert es nur als Asset und nicht als Komponente, und demzufolge kann man nur Prefabs und keine Szenen-Objekte zuweisen und das ist im Skript nunmal nötig... Also müsste ich alles dem Spawn-Skript zuweisen und dann in der Awake-Routine alles dem AI Script zuweisen. klingt irgendwie redundant.

Link zu diesem Kommentar
Auf anderen Seiten teilen

Wie gesagt, netwoking kenne ich mich nicht so aus, allerdings würde ich hier einen anderen Ansatz wählen. Trenne Die Darstellung und die Spiellogik. Die Animationen sind bei allen die selbe, das musst du nicht per Netzwerk synchronisieren.

Ein Ansatz wäre (cheating sicherer):

  • Nur der Server kennt alle Karten
  • Ein Spieler Client kennt nur die Karten die aufgedeckt sind.
  • Ein Client sagt dem Server den nächsten Zug
  • Der Server sagt dem anderen Client das Ergebnis.
  • Animationen und visuelle Positionen der Karten simulieren die Clients lokal.
vor 26 Minuten schrieb Kokujou:

st auf jeden fall höchst interessant dieses ScriptableObject funtkioniert das zur Laufzeit? Also wenn man das Spiel beendet, kann man die dann wiederherstellen wie ne Art Speicherdatei? Oder verschwinden die wenn das Programm beendet wird?

Änderungen gehen beim beenden verloren, sie eigenen sich daher für Einstellungen die du im Editor machst, oder auch um Daten zwischen verschiedenen Gameobjects/Komponenten auszutauschen.

Link zu diesem Kommentar
Auf anderen Seiten teilen

Das tue ich doch^^ Ich schicke synchronisiere den Random-Seed und mische so quasi jedes Deck auf die gleiche Weise und teile die Karten aus, dann schicke ich lediglich die Züge übers Netzwerk.

Okay das mit dem Austauschen klingt interessant, dann ist es aber ähnlich wie das was ich mit der Global realisiert hab, weil diese Klasse global von allen anderen angesprochen werden kann... Aber sowas wie globale Einstellungen suche ich schon lange, komisch dass ich's vorher nie gefunden habe. Danke!

Gibt's noch andere Performance-Einbrüche die du beobachtest?

Aber vielleicht kannst du mir ja nen Tipp wegen der restlichen Finds geben die ich nicht rauskriege. Ich habe eine Szene erstellt und benutze die GameObjekte darin als Parents als Referenzierung und da ich das AI-Skript Spawne kann ich Szenenobjekte nicht den öffentlichen Variablen zuweisen, das ginge nur bei Prefabs. Das Find ist nur im Start, wo nur wenige Objekte existieren dürften. Die ganzen Karten kommen ja erst später rein, nach der Start-Routine. Also die Frage:
Ich könnte diese Parents natürlich als Prefabs machen, dann könnte ich die Variablen zuweisen, dann müssten sie aber zur Laufzeit "gespawnt" werden. Die Frage ist lohnt das wirklich? Oder kann ich's einfach so stehen lassen, ich meine beim Start sind da vielleicht 10 Objekte in der Szene, die durchzuiterieren dürfte keine Performancelags verursachen, und ich weiß ja nicht wie teuer das Spawnen ist.

Oh, und ist Resources.Load eigentlich sehr effizient oder sollte das auch eher vermieden werden?

Uff je länger ich dran arbeite desto mehr Fragen ergeben sich: Wie dockt man eigentlich Funktionen ab? Gibt's da konventionen? z.B. die Update-Routine in der AI, ist ja extrem lang, jetzt will ich den Teil mit der Yaku-Erkennung abdocken. Wohin? In ne neue Klasse? Bei klassen gibt mir VisualStudio direkt die Option miteinem Klick ne neue Datei für anzulegen aber bei Funktionen macht es das nicht darum frag ich mich ob man da was anderes macht.

Am allermeisten regt mich auf, dass Unity alle Variablen die auf Skript-Assets einfach löscht wenn die Skripts erstellt werden. So ziemlich alle Resources.Load aufrufe sind darin begründet und ein Großteil der GameObject.Find genauso >.> statische Variablen können auch nicht im Inspektor zugewiesen werden also mussich de Fakto erstmal händisch überall Aufschreiben Variable1 = Variable 2 und das für jedes Variable, also für jedes Prefab dass ich benutze.

Link zu diesem Kommentar
Auf anderen Seiten teilen

Am 24.11.2018 um 13:04 schrieb Kokujou:

Gibt's noch andere Performance-Einbrüche die du beobachtest?

"Einbrüche" ist relativ, ich habe eine schnelle CPU und SSD, da hatte ich fast konstant bei 70-80 fps. Es gab aber manchmal spikes mit 15 fps. Bei schächeren PCs oder verallem Mobile, da könnte die Framerate etwas problematischer werden.

Am 24.11.2018 um 13:04 schrieb Kokujou:

Uff je länger ich dran arbeite desto mehr Fragen ergeben sich: Wie dockt man eigentlich Funktionen ab? Gibt's da konventionen? z.B. die Update-Routine in der AI, ist ja extrem lang, jetzt will ich den Teil mit der Yaku-Erkennung abdocken. Wohin? In ne neue Klasse? Bei klassen gibt mir VisualStudio direkt die Option miteinem Klick ne neue Datei für anzulegen aber bei Funktionen macht es das nicht darum frag ich mich ob man da was anderes macht.

Mit Visual Studio kannst du auch Teile des code automatisch in neue Methoden auslagern. Sie dir auch an ob die wiederholende Code-Bereiche hast, dann kann man dann gut ein eine einzelne Methode zusammenfassen.

Am 25.11.2018 um 12:28 schrieb Kokujou:

Ja ich nutze UNet... zumindest zum Aufbau der Verbindung. Danach benutze ich nur noch Nachrichten also keine Synchronisierungen von irgendwelchen Standorten ich brauchte das System eigentlich nur um Nachrichten von A nach B zu senden, ich wollte es low level machen^^

Ich kenne mich mit UNET jetzt nicht aus, aber ich habe gesehen du benutzt ja Network Identity, das ist aber Teil der high-level API und hat mit low level nichts zu tun.

So ein Kartenspiel könnte man auch sehr gut mit ECS/C# Jobs und dem neuen Unity Networking performant realisieren.

Link zu diesem Kommentar
Auf anderen Seiten teilen

ECS? das neue Unity Networking?

Ich benutze NetworkIdentity wie gesagt nur um die verbindung aufzubauen. Mir persönlich würde auch irgendne andere Methode reichen, die mir erlaubt Nachrichten zwischen A und B auszutauschen, aber ich kenne mich da leider nicht aus also benutze ich das Matchmaking (Siehe NetworkScript) und kann dann zwischen den Clients und dem Server Nachrichten schicken, der Server kann ja Broadcasten und die Clients an den Server senden, beim 1 vs 1 ist das also ganz gut. bei mehreren spielern wirds dann problematisch aber das ist ja hier Gott sei Dank nicht der Fall... Aber wenn du ne bessere Idee hast, nur raus damit :) Das Networking wird überarbeitet sobald ich mit den Performance-Korrekturen durch bin.

aktuell hab ich übrigens endlich sogut wie alle GameObject.Find* rausgehauen, jetzt hab ich nur noch die LINQ Find Methoden, aber die sind glaube ich nötig, Ich hab sogar ne Komponente hinzugefügt damit die GameObject Referenzen auf die Karten-Objekte haben die sie verkörpern (yeii~). Aber auf der virtuellen Ebene muss man natürlich die Matchs auf dem Feld finden etc... Wenn dir was einfällt wie man die auch raushaut, kannste ja bescheid sagen^^ oder wenn du noch andre Dinge siehst die performance fressen.

Die Frage war übrigens mehr, wohin man die Funktionen jetzt auslagert. Du hast ja gesagt pro Klasse eine Datei... Die Update-Routine mit 400+ Zeilen ist bleibt aber nunmal so groß, macht man jetzt auch ne Klasse für Funktionen? Ne Art Sammelklasse? oder ist es nur wichtig mehr FUnktionen zu haben auch wenn sie in derselben Datei sind?

Ich arbeite jetzt dank dir verstärkt mit Partial class, so konnte ich ohne große Korrekturen eine Prefab-Sammlung einbauen mit ner Singleton-Referenz die ich dann tatsächlich im Editor zuweisen kann... ich finds doof dass man keine statischen Variablen im INspektor zuweisen kann... aber naja... so hab ich dann auch eine Variablen-Sammlung gemacht, die alle bereits existierenden GameObjecte enthält... wenn ihr weiter nichts zu verbessern seht, lad ich mal die ganzen neuen Skripte hoch, dann könnt ihr mal überfliegen ob ich jetzt nicht alles verpfuscht habe

 

Link zu diesem Kommentar
Auf anderen Seiten teilen

Also ich hab jetzt mal die veränderten codes mit ins archiv gepackt, erreichbar wieder unter demselben link - danke google versionierung^^

https://drive.google.com/file/d/1sM8_ED-j0MjAz87MdKz9DhbZCcGXIjXW/view?usp=sharing

Die Klassen sind abgedockt, die GameObject.Find größtenteils draußen, Resources.Load genauso, andere Finds leider noch nicht da könnte ich noch Tipps gebrauchen

Link zu diesem Kommentar
Auf anderen Seiten teilen

  • 2 weeks later...

Uff :o danke für den Tipp!

Dann hoffe ich mal dass Unity die ganzen verweise automatisch mitnimmt. Ich hatte zuerst alles über Resources.Load gesammelt, weswegen ich mir das wohl angewöhnt habe, aber jetzt hab ich gemerkt dass das ein großes Loch ist und hab das dann alles über szeneninterne Verweise geregelt.

Dankeschön :) Ich freu mich über jede Kritik!

Link zu diesem Kommentar
Auf anderen Seiten teilen

Dann schau ich mal... da ich da nur so drüber gucke, schreibe ich nur, was ich anders machen würde. Also nicht wundern, dass ich nicht auch Dinge lobe. Geht schneller ;)

  1. Von UnityEngine.Object erbende Klassen sollte man nie zu mehreren in eine Datei tun.
  2. Dein Projekt hat keine .meta-Dateien im Repo. Die solltest du aber dringend mitcommitten, sonst funktioniert nichts.
  3. Du verwendest einen Mix aud deutschem und englischem Code. Lieber nicht :)
  4. Du bist außerdem sehr inkonsistent mit deinen Namen. Mal groß, mal klein, mal Unterstrich - und kaum etwas davon entspricht dem C#-Standard.
  5. Die Namen sind hier und da nicht sonderlich sprechend. Ich habe z.B. nicht einmal ansatzweise eine Idee, was das hier sein soll, und so etwas ist auch nicht so gut :)
  6. Du identifizierst Objekte über ihre Namen oder Tags. Davon ist abzuraten.
  7. Du identifizierst Objekte über ihre Typen. Davon ist dringend abzuraten.
  8. Du verwendest OnGUI.
  9. Du verwendest (gleich mehrere!) Klassen, die global den Spielverlauf beeinflussen. Das ist kein guter Stil.
  10. Teilweise sind diese Klassen sogar gefühlt für denselben Zweck. Warum gibt es "YakuHandler" und "YakuManager"?
  11. Du hast Code im Projekt, bei dem in der Summary "veraltet" steht. Du benutzt jetzt git. In git wird kein veralteter Code aufbewahrt, er wird einfach gelöscht :)

Das wäre erstmal alles, was mir so aufgefallen ist. Wenn du zu irgedetwas eine Frage hast oder eine Erklärung möchtest, sag Bescheid.

Link zu diesem Kommentar
Auf anderen Seiten teilen

Archiviert

Dieses Thema ist jetzt archiviert und für weitere Antworten gesperrt.

Ankündigungen


Hy, wir programmieren für dich Apps(Android & iOS):

Weiterleitung zum Entwickler "daubit"



×
×
  • Neu erstellen...