Open stijnb1234 opened 3 years ago
Hoi Stijn,
Ja klopt. Dank @stijnb1234 voor je issue. edited
Hopelijk kun je met feedback die wij vanmiddag bespraken verder zonder meteen een aanpassing in Game Engine. Je kunt ook een eigen GameEngine extends maken waarin je de mousePressed() override. Maar kudo's voor aanmaken issue, met ook concrete code suggesties; dat weeg ik mee bij de beoordeling!
Omdat je binnen mousePressed()
met jouw oplossing de for loop voor gameObjects dan zo'n beetje dupliceert voor dashboards lijst, en daarnaast deze code dan ook nog voor alle andere mouseHandlers zoals mouseMoved() etc. events ook moet dupliceren vind ik dit echter niet zo'n goede oplossing. Don't repeat yourself... Hieronder onder subkopje A en B twee aanpakken die deze twee nadelen oplossen, maar eerst hier nog even nadere beschouwing van het probleem en of dit nu een bug of een feature is.
Het probleem ligt feitelijk in de game engine, al kun je in plaats van een bug dit ook een feature noemen. Want een Dashboard is bedoeld om info te tonen, niet om clicks en andere mousevents af te handelen. Al beschrijft de api docs dit nu nog niet zo expliciet. En is daarnaast het feit dat Dashboard inderdaad GameObject extend verwarrend. Of eigenlijk het feit dat superklasse GameObject implements IMouseEvent. Dat laatste is wat mij betreft de ontwerpfout in OOPG.
Ik heb me nog niet precies genoeg verdiept in de functionaliteit van Dashboard
s t.o.v. GameObject
, maar de API vermeld iets dat ze 'above the viewport' worden getoond. Dus als je camera hoek verandert, veranderen dashboards niet mee, maar je perspectief op game objects wel.
Een andere optie is dat er een enkele abstracte superklasse boven GameObject en Dashboard zet ScreenComponent
o.i.d. En dat OOPG hier en lijst van bijhoudt i.p.v. aparte voor dashboards en gameobjects en dat de twee methodes addDashboard en addGameObject() methode ook vervangt door een enkele addScreencomponent(). Het is wel weer en extra niveau van indirectie, wat wellicht verwarrend is.
Verder dat je voor beide als ontwikkelaar zelf expliciet moet aangeven in je eigen specifieke extended klasse of deze IMouseEvent en IKeybioardEvent implementen, net zoals je dit doet voor ICollidableWithTiles
o.i.d. Daar was het ook geen mooie oplossing geweest om een concrete lege implementatie van collissionOccured(...)
in een superklasse te zetten die je optioneel override. Als je de interface zelf implement dan ben je ook verplicht een methode aan te maken, en dan vallen lege methodes meer op. Er worden dan ook niet nodeloos heel veel events doorgestuurd vanuit game engine naar objecten die hier helemaal niks mee doen.
Vanwege Interface seggregation principe zou je in plaats van een enkele IMouseEvent interface moeten opsplitsen eigenlijk aparte IClickable, IMouseMovable, etc. moeten maken, met elke een of enkele methodes (mousePressed en mouseReleased zouden wel in 1 interface kunnen). Analoog voor de keyboard interface. Wellicht hier een onderscheid ook maken tussen handlen van alleen keypress, of ook keyrRelease cq. het ingedrukt houden van een toets, waar je nu ook zelf workaround moet maken.
Anders eindig je weer met een boel methodes met een lege method body (wat ik voor het gemak vaak gewoon kort 'lege methodes' noem in les en bij assessments...).
Vanwege ontwikkelen nieuwe game engine Yaeger is het onwaarschijnlijk dat we dit oppakken. Maar wellicht treden hier soortgelijke issues in op. Waar bovengenoemde aanpak wellicht ook iets van kan oplossen.
mousePressed() bestaat gewoon in een Dashboard, omdat het een GameObject extend. Alleen wordt de mousePressed() niet aangeroepen in de GameEngine voor een dashboard, omdat die niet in de gameobject lijst aanwezig zijn.
https://github.com/HANICA/oopg/blob/d00d3272eec6a9b8ba2c513b4bc4b3d4e5cd2722/src/main/java/nl/han/ica/oopg/engine/GameEngine.java#L386-L396
Dit valt simpel te fixen, door dezelfde code ook te gebruiken voor de dashboards lijst.
Bijvoorbeeld:
(Dit geldt overigens ook voor alle andere mouse events)