Vinnaren i pepparkakshustävlingen!
  • 1
  • 2
2017-08-22, 14:57
  #1
Medlem
kakelpannas avatar
Vad kan jag förbättra kodmässigt i mitt schackprogram? Att jag använder en dålig AI-algoritm och att någon schackregel inte är implementerad är oviktigt, det har inte varit målet. Jag har medvetet hoppat över alpha-beta pruning.

Jag letar efter bättre kodstruktur, kanske bättre dokumentation, bättre variabelnamn. Vad ni än kan komma på.

https://gist.github.com/anonymous/57...9db9b119631cab

Väldigt tacksam.
__________________
Senast redigerad av kakelpanna 2017-08-22 kl. 14:59.
Citera
2017-08-22, 15:29
  #2
Medlem
En sak du bör fixa i gränssnittet är att använda bokstäver för en dimension. Det är standard inom schack. Ett move kan då vara a7a5.

Använd inte using namespace std. Skriv ut std:: istället.

En liten detalj är att main ska returnera en int. Kompilera med -Wall -Wextra -Werror och fixa till koden tills den kompilerar utan varningar.

Eftersom ChessBoard är en så stor klass skulle jag flytta ut funktionerna.

Kod:
struct ChessBoard{
...
	void reset();
       bool makeMove(Pos from, Pos to);
...
}

void ChessBoard::reset()
{
   ...
}

bool ChessBoard::makeMove(Pos from, Pos to)
{
...
}

Jag skulle försöka plocka bort din goto i promptInput. Det finns tillfällen där goto är motiverat. Det här är inte ett sådant. Det är dessutom mest för C och inte C++. Här borde du ha något i stil med while(haveNotMoved) eller något.

Kombinationen if(move=="help") och if(move.length==4) är inte klockren. I just det här fallet skulle det inte spela någon större roll, men låt oss anta att du i framtiden beslutar plocka bort möjligheten att skriva "help" och bara behåller "?" och "h" så kanske du inte vill att den okritiskt kör koden i if(move.length==4).

if(move=="") ser rätt märklig ut. Vad skulle move annars vara? Låter som något som inte skulle hända och istället möjligtvis ersättas med assert("") eller if(move != "") { std::err << "Expected move to have value "" << std::endl; }
Citera
2017-08-22, 16:36
  #3
Medlem
kakelpannas avatar
Tack för övriga tips. Tankar på några av tipsen.

Citat:
Använd inte using namespace std. Skriv ut std:: istället.
Även i mindre enfilsprogram som denna?


Citat:
Kombinationen if(move=="help") och if(move.length==4) är inte klockren. I just det här fallet skulle det inte spela någon större roll, men låt oss anta att du i framtiden beslutar plocka bort möjligheten att skriva "help" och bara behåller "?" och "h" så kanske du inte vill att den okritiskt kör koden i if(move.length==4).

All input som inte fångats up av if-satserna och sedan inte är nnnn (nummer 1-8 i if(move.length)) kommer faila i makeMove. Tankar kring detta? Hade det här kunna förmedlats bättre eller designats på ett annat sätt eller liknande?

(inmatningsmetoden var väldigt pragmatisk)

Citat:
if(move=="") ser rätt märklig ut.
Sant. Jag missade den, den ska inte vara där. Move var tidigare antingen "" som bad om user input eller "nnnn" skriven av en AI som då hoppade över input.
Citera
2017-08-22, 18:32
  #4
Medlem
Citat:
Ursprungligen postat av kakelpanna
Tack för övriga tips. Tankar på några av tipsen.


Även i mindre enfilsprogram som denna?
Även enfilsprogram som denna. Finns ingen anledning att använda dåliga ovanor bara för att du skriver små program.

Här har du ett exempel på där det blev fel. https://stackoverflow.com/questions/...712125#2712125

Citat:
All input som inte fångats up av if-satserna och sedan inte är nnnn (nummer 1-8 i if(move.length)) kommer faila i makeMove. Tankar kring detta? Hade det här kunna förmedlats bättre eller designats på ett annat sätt eller liknande?

(inmatningsmetoden var väldigt pragmatisk)
Det är ingen jättegrej i ditt fall, men saken är att du blandar funktionalitet i varandra. Börjar du bygga ut programmet kan det där bli en jobbig sak.

Ett sätt att lösa det är att introducera ett kommando för move. Om du exempelvis skriver in "m" så får du skriva in koordinater efteråt. Det kan dock tänkas att det är det du vill göra i 99% av fallen. En annan approach är att standard är att ta emot koordinater, men att du aktiverar kommandoläge med exempelvis ":". Om du använder ett specialtecken blir det lätt att särskilja.

Vill du inte använda något av dessa skulle jag kunna tänka mig en funktion för att klassificera indatan. Den skulle kunna ha den här deklarationen:

Kod:
enum inputType { invalid, command, move };
inputType classifyInput(string input);

och sedan något i stil med
Kod:
inputType i = classicyInput(input);
switch (i) :
{
   case invalid: std::cout << "Invalid command" << std::endl; break;
   case command: performCommand(input); break;
   case move: performMove(input); break;
}
__________________
Senast redigerad av RulleRivare 2017-08-22 kl. 18:39.
Citera
2017-08-23, 09:29
  #5
Medlem
MeanMEs avatar
Kollade igenom det som hastigast.
Skriver här från ett schackperspektiv.

Det du kan fundera på är om värdet för pjäserna bör vara av typen int.
Det är ju sällan en springare är lika mycket värd som en löpare.
Ett löparpar är oftast mer värt än en springare och en löpare.
Bönders värde ökar varefter de avancerar och om de är fribönder osv.
Så av den anledningen kan det vara läge att överge int och ha ovan i tanke på hur du skall värdera olika kombon av pjäser.

Det samma gäller framöver om du tänkt gå vidare och du väljer att bedöma dragen utifrån ställningen och värderar pjäsernas position, sammanhållna bondelinjer, om de är placerade på samma fält som motspelarens löpare eller inte, kontroll över centrumfälten, diagonaler och linjer osv.

Skulle rekommendera att du använder double istället.
Du kan ju välja att öka värdet med en faktor av 100 eller 1000 och fortsätter att använda int men double känns mer naturligt och intuitivt.

Sedan moverPieces...
Låter inget vidare imho.
Citera
2017-08-23, 20:26
  #6
Medlem
Citat:
Ursprungligen postat av MeanME
Kollade igenom det som hastigast.
Skriver här från ett schackperspektiv.

Det du kan fundera på är om värdet för pjäserna bör vara av typen int.
Det är ju sällan en springare är lika mycket värd som en löpare.
Ett löparpar är oftast mer värt än en springare och en löpare.
Bönders värde ökar varefter de avancerar och om de är fribönder osv.
Så av den anledningen kan det vara läge att överge int och ha ovan i tanke på hur du skall värdera olika kombon av pjäser.

Det samma gäller framöver om du tänkt gå vidare och du väljer att bedöma dragen utifrån ställningen och värderar pjäsernas position, sammanhållna bondelinjer, om de är placerade på samma fält som motspelarens löpare eller inte, kontroll över centrumfälten, diagonaler och linjer osv.

Skulle rekommendera att du använder double istället.
Du kan ju välja att öka värdet med en faktor av 100 eller 1000 och fortsätter att använda int men double känns mer naturligt och intuitivt.
Jag tänkte först inte skriva något eftersom det är fel att fokusera på prestanda innan man vet hur man skall lösa problemet. Dock är erfarenheten den att om han faktiskt får programmet att fungera så tenderar poängberäkningen att bli det enskilt dyraste steget i trädsökningen.

Första förbättringen blir att ersätta användandet av "std::map<Piece, int>" med en const-deklarerad array ordnad efter enumens numeriska värde. Ungefär som EnumMap är implementerad i Java. Det gör att uppslagningen tar en maskininstruktion istället för att kräva en trädsökning som kompilatorn aldrig kan optimera bort.

För att snabba upp det ytterligare så kommer han att behöva göra en differentiell poängberäkning så att han subtraherar värdet av pjäsen han skall flytta och adderar värdet efter att den flyttats. Han slipper då iterera över alla pjäser i trädsökningen. Men för att klara differentiella beräkningar bör han använda heltal och inte flyttal eftersom han annars får ett avrundningsfel som ackumuleras i varje iteration. Dessutom är det lättare för kompilatorn att optimera heltalsberäkningar eftersom beräkningsordningen inte spelar någon roll.
__________________
Senast redigerad av WbZV 2017-08-23 kl. 20:33.
Citera
2017-08-23, 22:47
  #7
Medlem
kakelpannas avatar
Citat:
Ursprungligen postat av WbZV
Jag tänkte först inte skriva något eftersom det är fel att fokusera på prestanda innan man vet hur man skall lösa problemet. Dock är erfarenheten den att om han faktiskt får programmet att fungera så tenderar poängberäkningen att bli det enskilt dyraste steget i trädsökningen.

Första förbättringen blir att ersätta användandet av "std::map<Piece, int>" med en const-deklarerad array ordnad efter enumens numeriska värde. Ungefär som EnumMap är implementerad i Java. Det gör att uppslagningen tar en maskininstruktion istället för att kräva en trädsökning som kompilatorn aldrig kan optimera bort.

För att snabba upp det ytterligare så kommer han att behöva göra en differentiell poängberäkning så att han subtraherar värdet av pjäsen han skall flytta och adderar värdet efter att den flyttats. Han slipper då iterera över alla pjäser i trädsökningen. Men för att klara differentiella beräkningar bör han använda heltal och inte flyttal eftersom han annars får ett avrundningsfel som ackumuleras i varje iteration. Dessutom är det lättare för kompilatorn att optimera heltalsberäkningar eftersom beräkningsordningen inte spelar någon roll.

Jag har fått förslaget att det går snabbare att generera moves om schackbrädan är en array istället för map, stämmer det? Att man går igenom alla 64 rutor och genererar moves för relevanta rutor, istället för att iterera över en map av aktuell spelares pjäser. Det dröjer en del innan jag kan testa det, men vad tror du?

Jag provade det du skrev och resultatet var detsamma. Ialllafall inte märkbar.
I båda fallen får jag samma tid för det här testet, ena gången med array av piece values i score() där pjäserna är enum-ints ints och kollas upp med pieceValue[piece]. Andra försöket är med map av pieceValues.

ChessBoard test;
test.reset();
size_t t=time(0);
test.AIMove();
t=time(0)-t;
cout<<t<<endl;
return 0;

Båda ger 6 sekunder, mapen ger 6 och bara ibland 7. Jag provade att göra sökträdet djupare. För minimax med djup 6 istället för 5.. så fick jag: 168 sekunder med map, 156 sekunder med array.
Citera
2017-08-24, 09:03
  #8
Moderator
RostigHinks avatar
Vore jag en kompilator så skulle jag när jag ser:
Kod:
enum class Piece {king, queen, white_pawn, black_pawn, rook, bishop, knight};
static map<Piece,int> pieceValues;
implementera mapen som en enkel array. Mapens nyckel blir i alla fall en unsigned int vilket kan användas som index.

Kanske därför skillnaden blir så liten.
Citera
2017-08-24, 11:13
  #9
Medlem
kakelpannas avatar
Citat:
Ursprungligen postat av RostigHink
Vore jag en kompilator så skulle jag när jag ser:
Kod:
enum class Piece {king, queen, white_pawn, black_pawn, rook, bishop, knight};
static map<Piece,int> pieceValues;
implementera mapen som en enkel array. Mapens nyckel blir i alla fall en unsigned int vilket kan användas som index.

Kanske därför skillnaden blir så liten.

om den hade implementerat det som en array så borde det inte varit någon skillnad mellan map-versionen och array-versionen. Jag ett till test med mappen nu för att vara säker. Med array fick jag 155 (resultat från förra inlägget) och nu 156 sekunder. Mappen 168 (förra inlägget) och nu 168.
Citera
2017-08-24, 11:56
  #10
Moderator
RostigHinks avatar
Citat:
Ursprungligen postat av kakelpanna
om den hade implementerat det som en array så borde det inte varit någon skillnad mellan map-versionen och array-versionen. Jag ett till test med mappen nu för att vara säker. Med array fick jag 155 (resultat från förra inlägget) och nu 156 sekunder. Mappen 168 (förra inlägget) och nu 168.
Säger du till kompilatorn att optimera?

Nu vet jag inte vilken miljö du använder men om det är gcc så skickar man med växeln -O2 för normal optimering.
Citera
2017-08-24, 15:23
  #11
Medlem
kakelpannas avatar
Citat:
Ursprungligen postat av RostigHink
Säger du till kompilatorn att optimera?

Nu vet jag inte vilken miljö du använder men om det är gcc så skickar man med växeln -O2 för normal optimering.

-O3. Jag tror att kompilatorn helt enkelt inte optimerar bort det, utan att mapen är ganska snabb i det här fallet.
Citera
2017-08-24, 18:41
  #12
Medlem
Citat:
Ursprungligen postat av kakelpanna
Jag har fått förslaget att det går snabbare att generera moves om schackbrädan är en array istället för map, stämmer det? Att man går igenom alla 64 rutor och genererar moves för relevanta rutor, istället för att iterera över en map av aktuell spelares pjäser. Det dröjer en del innan jag kan testa det, men vad tror du?

Jag provade det du skrev och resultatet var detsamma. Ialllafall inte märkbar.
I båda fallen får jag samma tid för det här testet, ena gången med array av piece values i score() där pjäserna är enum-ints ints och kollas upp med pieceValue[piece]. Andra försöket är med map av pieceValues.

ChessBoard test;
test.reset();
size_t t=time(0);
test.AIMove();
t=time(0)-t;
cout<<t<<endl;
return 0;

Båda ger 6 sekunder, mapen ger 6 och bara ibland 7. Jag provade att göra sökträdet djupare. För minimax med djup 6 istället för 5.. så fick jag: 168 sekunder med map, 156 sekunder med array.
Som jag skrev innan, börja med att få programmet att fungera som du vill. Algoritmmässiga förbättringar som leder till att du behöver besöka färre noder vinner alltid över kodoptimeringar. Det är först när du inte kommer längre med algoritmerna som du kan vinna något på att slipa på detaljer. Gör man tvärtom så riskerar man att tidigt hamna i kod som är svår att ändra och därför är svår att förbättra.

Eftersom datamodellen som representerar ett schackspel går att göra extremt kompakt så vinner du antagligen på att skapa representationer som gynnar algoritmernas flöde, även om det leder till en viss redundans. Antagligen vill du både kunna iterera över schackbrädets rutor och över spelets pjäser beroende på vad du skall göra. I princip kan du representera brädet med en array på 64 bytes. För varje ruta behöver du 1 bit som talar om ifall den är tom eller inte, 1 bit som talar om färgen på pjäsen och 5 bitar för vilken pjästyp som står där (kung, dam, löpare, häst, torn eller bonde). Vill du sedan kunna iterera över pjäserna så kan du ha en annan array på 32 bytes som talar om vilken ruta varje pjäs står på och krympa storleken när pjäser försvinner. Åtminstone i slutspelet så kan det vara en fördel medan det inte gör så stor skillnad i början.

Eftersom det verkar råda en viss förvirring om standardkontainrarna så ser det ut så här i C++:
std::set/std::map - implementerade genom rödsvarta träd. Ett set eller en map bäddar in en pekare till en rot-nod och varje nod bäddar in ett element plus pekare till förälder samt vänster och höger barn.
std::unordered_set/std::unordered_map - implementerade genom hashtabeller. Ett set eller en map bäddar in en pekare till en array av länkade listor där varje listnod bäddar in ett element plus en pekare till nästföljande nod.
std::vector<T> - bäddar in en pekare till en dymaniskt allokerad array av godtycklig storlek.
std::array - bäddar in en array av element. std::array<T, n> är i princip samma som T[n].

Betrakta följande kodexempel:
Kod:
const std::map<int, int> a = {{ 1, 0}, {2, 0}, ...};
const std::map<int, int> b = {{ 1, 1}, {2, 2}, ...};
int x = a[0];
Även om kompilatorn skulle kunna förstå hur trädet a ser ut så kan den inte vid kompileringstillfället lista ut vilket värde x skall få, utan den måste köra en dyr trädsökning för att hämta ut värdet. Ett oöverstigligt problem för kompilatorn är att a och b pekar ut dynamiskt minne av samma typ och att kompilatorn därför inte kan veta om den skriver över a:s noder när den initierar noderna i b. Const-deklarationen gäller bara a och b, inte noderna som implementerar a och b.

Jämför med följande:
Kod:
const int a[] = { 0, 0, 0, ... };
const int b[] = { 1, 2, 3, ... };
...
int x = a[0];
if (x == 1) foo();
Vi talar om för kompilatorn att a och b är konstanta arrayer vars värden inte går att ändra. Kompilatorn kan redan vid kodgenereringen se att x får värdet 0 och att den inte ens behöver generera anropet till foo() eftersom man aldrig kan komma dit.

I slutänden gör sådana här skillnader genomslag i körtiden. Men skriv programmet först och gör sedan profileringar. Då hittar du vad som faktiskt tar tid och kan åtgärda de viktigaste sakerna först. Ofta spenderar programmet tiden på helt andra ställen än vad man trodde innan.
__________________
Senast redigerad av WbZV 2017-08-24 kl. 18:44.
Citera
  • 1
  • 2

Stöd Flashback

Flashback finansieras genom donationer från våra medlemmar och besökare. Det är med hjälp av dig vi kan fortsätta erbjuda en fri samhällsdebatt. Tack för ditt stöd!

Stöd Flashback