aaronvark / PeerReview1819

Repo for peer review assignments for year 2 development class of 18/19
0 stars 0 forks source link

Casper van Battum - Eindopdracht blok 1 #34

Closed Creator13 closed 4 years ago

Creator13 commented 5 years ago

Breakout but it's in a ring instead of above you.

Creator13 commented 5 years ago

This is the first playable version of Breakin; breakout but it's in a circle and you have to break through to get to the center. The idea is that you will get new rings of blocks and that the ball will move faster as you progress through the level. There will be different kinds of blocks to make the game more interesting, but these features are yet to be implemented.

Below is the UML of the first playable version UML v2

aaronvark commented 5 years ago

Goed gebruik van abstract om je Block class generiek te kunnen inzetten in de Ring, zodat je makkelijk andere soorten interacties kunt bouwen op een later moment. Ziet er naar uit alsof je dat al eens eerder hebt gedaan.

Nette UML ook! Goede inzet van de association tussen Bat/Ball, maar de lijn tussen Ball en Block is volgens mij niet eens nodig. Er is vooral een verbinding tussen de twee via de physics engine, dus hebben ze vooral een dependency naar die engine subsystem, maar niet naar elkaar (zover als ik kan zien roepen ze niks van elkaar aan, en bezitten ze ook geen references naar elkaar).

Qua commenting ga je soms een beetje overboard. Bijv. bij de "Check Mouse Click", want de naam spreekt in principe al voor zich. Check of je vooral uitlegt waarom iets gebeurt, en niet gewoon letterlijk wat (zoals in het geval van "Set bat as parent"). Een goed voorbeeld is bijvoorbeeld je comment boven Lives: "When this is set to...", want daar leg je iets niet-obvious uit wat iemand zou missen als ze de code niet zouden lezen (dus een summary vanuit een hover-over geeft hier nuttige informatie).

Als ik echt ga nitpicken kan ik ook nog vragen waarom "SetPosition" met de comment "Apply Position", niet gewoon Apply Position heet ;). Sowieso in deze opzet is het inderdaad een naam die niet past bij wat er gebeurt, dus kan je inderdaad overwegen om het Apply of zelf Update Position te noemen (wat impliceert dat er op basis van een interne state iets veranderd, niet iets wat je van buiten meegeeft zoals Set... ook al zijn er geen parameters).

Alles is prima te volgen en goed leesbaar!

Creator13 commented 5 years ago

Ik heb hier vooral de feedback verwerkt zonder verder het spel uit te breiden.

Ik heb bij zoveel mogelijk comments gekeken of ik er een reden bij kon zetten; een korte uitleg van waarom ik die code uitvoer. Over die naam van SetPosition/Rotation ben ik het helemaal eens, die heb ik dan ook aangepast naar Apply. Ik heb ook de functie die de angle variabele update veranderd naar UpdateAngle in plaats van GetAngle vanuit deze zelfde gedachte.

De lijn in de UML tussen Ball en Block is eigenlijk onstaan doordat ik via tags check of de ball een blok raakt, maar dit is inderdaad niet een echte reference binnen de code omdat het om een tag gaat en niet de class zelf.

Creator13 commented 5 years ago

Waar ik nu ben: Het spel is speelbaar en de score wordt bijgehouden. Er zitten op dit moment geen bugs in (dat ik weet in ieder geval). Voor de ScoreManager heb voor het eerst ik geprobeerd een singleton met een eigen interface te maken.

Wat ik nog wil: De twee grote features die ik nog wil toevoegen zijn het spawnen van nieuwe ringen tijdens het spel, en ik wil meer variatie in het aantal blokken. Voor het spawnen van blokken (en evt ringen) wil ik een object pool gaan gebruiken, omdat dit een typische usecase daarvoor is. Verder wil ik ook level progressie implementeren, en mogelijk nog animaties. Ook kleine beautification features als een soundsystem staan op het programma. Ik wil eigenlijk ook nog kijken of ik een finite state machine ergens kan verwerken maar ik kan nog geen goede plek bedenken.

Creator13 commented 5 years ago

NOTE: er is geen pull request voor deze deadline omdat ik alle changes al gecommit voordat ik een review kreeg en mergde. Je kan kijken bij #50 voor de changes, maar die is dus al gelsoten.

aaronvark commented 5 years ago

Goed om te zien dat je comments wat meer context geven. Vergeet niet dat het OK is om comments gewoon te verwijderen als de code zelf al duidelijk genoeg is, dus neem die keuze ook steeds mee.

Waarschijnlijk is de FSM vooral van toepassing op de macro schaal van je game fasering (build level, play, player death, reset, etc.) Dat stelt je dan ook in staat om bepaalde fase-specifieke code, zoals het opbouwen van een level, op een makkelijk terug te vinden plek uit te voeren.

Qua de pooling van blocks kan je kijken of je dat wilt doen per ring, of per block. Als een ring een set blokken opvraagt zou je die kunnen bewaren tot de hele ring klaar is, of je kunt individuele blocks returnen elke keer dat ze vaak genoeg geraakt zijn. In principe zou ik in eerste instantie denken aan dat laatste. Voor level progressie zou ik aanhouden dat je dezelfde states gebruikt (bijv. StartLevel), en dat die gebruik maakt van andere "data" (of een bestand wat je inleest, of een ScriptableObject waar de vorm van een level in staat). Voorkom states die level specifiek zijn (StartLevel1, StartLevel2, bijv.)

Interessante lambda expression (=> ... ?? ...) bij de Score Manager. Die heb ik nog niet eerder gezien (net zoals die ...?.Invoke(), blijkbaar zijn dat C# features die pas sinds de .Net updates van Unity beschikbaar zijn). Lekker compact!

Creator13 commented 5 years ago

Het is gelukt om een object pool te maken en om een soundsystem te maken (met inspiratie uit het voorbeeld uit de les). Nu de object pool werkt heb ik ook eindelijk het spawnen van nieuwe Rings afgemaakt. Na het even met Valentijn te hebben over mijn pool heb ik deze ook nog generic gemaakt zodat hij hergebruikt kan worden. Ik realiseerde me namelijk dat er geen reden is dat het specifiek voor Blocks moet werken.

Er zijn nog wel wat dingen waar ik over twijfel, zoals het feit dat de blokken op dit moment zichzelf returnen naar de pool door hun IsActive property op false te zetten. Dit geeft me wel meer controle over wat er gebeurt met een block op het moment dat deze teruggegeven wordt, maar voor mijn gevoel kan dit te snel fout gaan als het verkeerd gebruikt wordt. Ook is dit praktischer omdat mijn pool geen singleton is maar een instance member van de spawner is. Zo hoef ik niet een referentie naar de spawner/de pool mee te geven aan ieder blok. Misschien is een singleton wel een betere oplossing maar aan de andere kant is het ook logisch dat een object pool bij hetgeen dat de objecten gebruikt hoort.

Wat ik ook lastig vind is een goede manier bedenken om de game loop/level progressie te implementeren. Je kan een simpele state machine maken met de states Menu -> Playing -> Win/Lose maar ik heb het idee dat ik daarvoor heel veel dingen op z'n kop moet gooien en moet herschrijven (vooral de spawner, omdat deze de spelinstellingen bevat). Een singleton LevelManager zou in dit geval (naar mijn idee) handiger zijn, alleen weet ik dan niet waar de entry point van de game komt (omdat alles gewoon geinitialiseerd wordt in start). Om level progressie voor elkaar te krijgen zonder te switchen naar een nieuwe scene moeten bovendien alle bestaande objecten opnieuw geïnitialiseerd worden, waar veel extra code voor nodig is. In ieder geval wil ik een gecentraliseerde class maken die het spel bestuurt: hier worden ook de levens bijgehouden in plaats van op de bal, via hier wordt een level gewonnen of verloren en wordt het spel gestart. Nu gebeurt alles nog volledig gedecentraliseerd en dit kan bugs opleveren als je de code uitbreid. Alleen de beste manier om dit te uit te voeren is mij nog niet helemaal duidelijk.

Oh en die property expressie is inderdaad nieuw! Ik wist er ook niet van tot mijn IDE begon te zeuren dat het korter kan en mij dit aanbood. Blijkbaar betekent het "return x als niet null, anders doe y". Zo had ik ineens vijf regels code naar één verkort. Link naar docs

vmuijrers commented 5 years ago

Je kunt voor je level progressie nog steeds een GameManager gebruiken die wat referenties heeft naar de settings van je spel/level, deze kan dan gebruik maken van een state machine en de settings doorgeven aan de states die ze nodig hebben, zo houd je je GameManager relatief klein/leesbaar. Voor de objectpool kun je er voor kiezen om de owner van een object mee te geven aan het gepoolde object zodat dit communicatie versimpeld en het gepoolde object een functie kan aanroepen op de pool zonder dat dit meteen een singleton moet zijn (zo kun je nog steeds meerdere pools aanmaken).

Creator13 commented 4 years ago

Link naar project