search

Dungeon-CampusMinden / Dungeon

The "Dungeon" is a tool to gamify classroom content and integrate it into a 2D Rogue-Like role-playing game.
MIT License
15 stars 36 forks source link

[GAME] `static` in `Game` untersuchen #746

Closed AMatutat closed 10 months ago

AMatutat commented 1 year ago 
          @AMatutat bei der gelegenheit: der ganze viele `static` kram ist ja historisch gewachsen. mal einen schritt zurückgetreten: brauchen wir das überhaupt noch so? muss das `Game` noch singleton sein? eigentlich nicht, oder? und dann würde sich einiges von dem setup/cleanup-problem, was @AHeinisch oben angesprochen hat, in luft auflösen.

Originally posted by @cagix in https://github.com/Programmiermethoden/Dungeon/issues/213#issuecomment-1592345297

Anmerkung: Es sollte unterschieden werden zwischen den Hilfsmethoden (z. B. tileAt), die weiterhin statisch in der Klasse "Game" liegen können/sollen (um einen einfachen API-Zugriff zu ermöglichen), und den Attributen/Funktionen, die mit der Game-Loop in Verbindung stehen. Ich denke dabei vor allem an die verschiedenen Sets, den LevelManager und den hero.

Früher waren diese in der Klasse "Game" nicht statisch, und wir haben die Informationen durch Konstruktorparameter immer tiefer weitergegeben. Das hat den gesamten Code jedoch nicht wirklich lesbar gemacht.

Wir nutzen jedoch viele dieser statischen Sets in unseren Hilfslogiken wie "AITools", um z. B. die Distanz von Monstern zum Helden zu berechnen. Ohne einen statischen hero funktioniert das nicht so gut.

Idee: Vielleicht könnten wir eine public static Game instance() Methode verwenden, um die Instanz (Singleton) überall abzurufen und dann auf die non-statischen (aber dennoch öffentlichen) Felder zuzugreifen.

Edit: Da wir in den Tests das statishce Game instance() mocken müssten, und statischen Mocking nicht so einfach funktioniert, könnte man sich auch überlegen hier über ein public static Attribut Game game nachzudenken. Im Code könnte man dann z.B mit Game.game.addEntity(this) darauf zugreifen und in den Tests könnten wir Game.game=Mockito.mock(Game.class) machen.

AMatutat commented 1 year ago

@cagix @AHeinisch ich habe zwar "later" gelabelt, aber irgendwie kribbelt es in den Fingern. Könnt ihr mal eure Meinung abgeben? Könnte das Funktionieren oder übersehe ich irgendwas drastisches?

cagix commented 1 year ago

@cagix @AHeinisch ich habe zwar "later" gelabelt, aber irgendwie kribbelt es in den Fingern. Könnt ihr mal eure Meinung abgeben? Könnte das Funktionieren oder übersehe ich irgendwas drastisches?

So aus dem Bauch heraus: Wir haben (a) ziemlich viele statische Attribute, beispielsweise Mengen von Entitäten und so, und (b) statische Methoden und (c) das Singleton-Pattern.

Mein Gefühl ist, dass wir über das (derzeit implementierte) Builder-Pattern durchaus mehrere/verschiedene neue Game-Objekte herausgeben könnten, d.h. die Klasse müsste nicht mehr Singleton sein und die ganzen Attribute könnten auch normale Instanzattribute werden - d.h. (a) und (c) könnten zusammen verschwinden. Bei den Zugriffsmethoden (b) müsste man schauen, was gegen eine Instanz gerichtet ist (das würde dann das static verlieren) und was eher eine Hilfsmethode ala Collections ist (würde static bleiben).

Ich sehe grad keinen Grund (mehr), warum wir den Start des Spiels auf ein Spiel beschränken, aber vielleicht habe ich grad auch irgendwas nicht mehr so richtig auf dem Schirm - das kann @AHeinisch vermutlich deutlich besser einschätzen.

Aber wie geschrieben, das ist nur mein Bauchgefühl. Ich hatte noch keine Zeit, mal gründlich drüber nachzudenken.

AHeinisch commented 1 year ago

Also der Grund warum wir ein statisches Singleton vom Game haben ist aktuell der Grund der schnellen und einfachen Art zugreifen zu können. Jeder der Game.hero() anspricht, weiß es handelt sich um genau das aktuelle Game.

Idee: Vielleicht könnten wir eine public static Game instance() Methode verwenden, um die Instanz (Singleton) überall abzurufen und dann auf die non-statischen (aber dennoch öffentlichen) Felder zuzugreifen.

Dies klingt mir als würden wir unseren Code so schreiben, dass er besser zu testen ist, aber nicht wie er für den Programmierer einfacher ist. Wenn ich nun also Game.instance().hero() schreiben muss, dann war das Ändern der API sinnlos. Da wir dort ja extra Game.level().tileAt() gegen Game.tileAt() ausgetauscht haben. Jedoch wäre Game.instance().tileAt() wieder ein Rückschritt.

Game ist theoretisch der Kern unseres Frameworks. Der Name "Game" sagt nicht wirklich aus, was es mittlerweile alles macht.

Dies sind jetzt nur die Punkte, die mir jetzt stark einfallen. Ich hatte erst mich verlesen. Also wir werden nicht alles statisch entfernen können. Reduzieren würde bedeuten wir geben irgendwo das Game mit oder es muss über ein "Singleton" erreichbar sein.

Vielleicht verstehe ich auch aktuell noch nicht ganz den Ansatz und verstehe hier gerade was falsch. Klingt für mich aber sinnvoll zu überlegen, ob wir an der großen Game Klasse was optimieren können.

cagix commented 1 year ago

Also der Grund warum wir ein statisches Singleton vom Game haben ist aktuell der Grund der schnellen und einfachen Art zugreifen zu können. Jeder der Game.hero() anspricht, weiß es handelt sich um genau das aktuelle Game.

aber dazu müsste Game nicht unbedingt singleton sein bzw. "alles" static sein, oder? der game-builder könnte ein objekt zurückgeben, auf dem man so wie jetzt die methoden aufrufen kann (nur eben dann auf dem objekt und nicht auf der klasse) und es könnte gleichzeitig mehrere game-objekte geben. damit das klappt, müssten alle akteure (klassen) eine referenz auf "ihr" game-objekt bekommen. da die systeme und entitäten sowieso beim game registriert werden, sollte das kein allzu grosser nachteil sein.

AHeinisch commented 1 year ago

Mehrere Game-objekte geht aktuell nicht, da Game.run() ein LWJGL Fenster startet, welches wiederum ein opengl oder GLFW Fenster erstellt. Dies darf pro Java Prozess anscheinen nur einmal existieren. Dadurch ergibt sich für mich das Game immer noch singleton bleiben muss. Ob nun die Instanz jeden herübergereicht wird und dann auf der Instanz gearbeitet wird anstelle von static würde klappen.

cagix commented 1 year ago

Ob nun die Instanz jeden herübergereicht wird und dann auf der Instanz gearbeitet wird anstelle von static würde klappen.

Hmmm, d.h. wir haben die Wahl zw. (a) static und (b) Instanz "herumreichen?

AMatutat commented 1 year ago

(b) sieht im Nutzer-Code vermutlich recht hässlich aus, würde uns aber den Test ziemlich erleichtern.

Das wird richtig hässlich. Im Grunde braucht dann fast jede Klasse/ jede static Methode (z.B in AITools) dann auch ein Game Parameter. Daher kam mein Vorschlag mit Game#instance, dann hätten wir das schönste aus beiden Welten, aber ich gebe @AHeinisch recht

Wenn ich nun also Game.instance().hero() schreiben muss, dann war das Ändern der API sinnlos. Da wir dort ja extra Game.level().tileAt() gegen Game.tileAt() ausgetauscht haben. Jedoch wäre Game.instance().tileAt() wieder ein Rückschritt.