Tough promotion bug(s)

[Sven]

Yes as I said the code is not written for others. By the way If I look at code from other people I'm shocked too.

Your design is fairly different from many other chess program designs. It is somehow unique that way, which is a good thing.

Also I get the impression that your design is consistent to a certain degree. It looks unusual for the eyes of most of us but at least I would say that it is a *possible* design. If I would be asked to create a really pure OO design of chess board, squares and pieces I would do it in a completely different way but it would still be necessary to have some concept for linking squares to pieces, which you do with the Location reference in the Piece object. That is not so bad.

I tend to like your "Location.BitCoord" concept. It corresponds to "1ULL << sqr" for a given square number 'sqr' from 0..63.

Nevertheless I believe that your program is overdesigned. I think that a lot of your code is actually not needed for a working board representation, even with OO. It would be sufficient to only have a (somewhat larger) Position class with low-level operations like

PutPieceOnBoard(Location location, PieceType pieceType)
RemovePieceFromBoard(Location location)
MovePiece(Location from, Location to)
PromotePiece(Location location, PieceType promotionPieceType)
UnpromotePiece(Location location)

and high-level operations like

MakeMove(Move move)
UnmakeMove(Move move)

that are based upon the first ones. It would drastically reduce the complexity of your program, and it would put the code where it belongs. Your current methods Piece::PutOnBoard() and Piece::RemoveFromBoard() actually do not operate only on pieces but they modify the Board as well, so that should be part of methods of class Board resp. class Position.

Things can be done differently, of course, and again: I don't say the design is "wrong". I just say, it will most probably be a dead end, it will not help you to reach higher search depths due to inefficiency.

It is just a proposal, maybe you want to think about it.

Sven

[Gerd Isenberg]

Yes as I said the code is not written for others. By the way If I look at code from other people I'm shocked too.

Your design is fairly different from many other chess program designs. It is somehow unique that way, which is a good thing.

Also I get the impression that your design is consistent to a certain degree. It looks unusual for the eyes of most of us but at least I would say that it is a *possible* design. If I would be asked to create a really pure OO design of chess board, squares and pieces I would do it in a completely different way but it would still be necessary to have some concept for linking squares to pieces, which you do with the Location reference in the Piece object. That is not so bad.

I tend to like your "Location.BitCoord" concept. It corresponds to "1ULL << sqr" for a given square number 'sqr' from 0..63.

Nevertheless I believe that your program is overdesigned. I think that a lot of your code is actually not needed for a working board representation, even with OO. It would be sufficient to only have a (somewhat larger) Position class with low-level operations like

PutPieceOnBoard(Location location, PieceType pieceType)
RemovePieceFromBoard(Location location)
MovePiece(Location from, Location to)
PromotePiece(Location location, PieceType promotionPieceType)
UnpromotePiece(Location location)

and high-level operations like

MakeMove(Move move)
UnmakeMove(Move move)

that are based upon the first ones. It would drastically reduce the complexity of your program, and it would put the code where it belongs. Your current methods Piece::PutOnBoard() and Piece::RemoveFromBoard() actually do not operate only on pieces but they modify the Board as well, so that should be part of methods of class Board resp. class Position.

Things can be done differently, of course, and again: I don't say the design is "wrong". I just say, it will most probably be a dead end, it will not help you to reach higher search depths due to inefficiency.

It is just a proposal, maybe you want to think about it.

Sven

It seems Alice has a somewhat similar design with piece objects. For an oo-design it somehow makes sense to use classes for pieces and to use virtuals instead of switch/case. But I agree with you, 100% on Piece::PutOnBoard() and that like.

[sje]

Early in the time of the Big Sales Pitch for OOP (Object Oriented Programming), a now infamous statement was made to the effect that a switch statement in OOP was as bad as a goto statement in structured programming. Over the years this proclamation has caused a lot of confusion and damage.

The view was that the code in a case body really should instead be implemented as an instance method, and for each possible case body there would be a different class definition. Example: In Symbolic, an instance of its Move class has a member variable mc which is one of eight enumeration constants which describe the move (regular, en passant, queen side castling, king side castling, and the four promotions). For each of the three purposes of execution, retraction, and formatting, there is a switch on the mc member to facilitate specific processing. I suspect that most other programs have a similar arrangement.

But in an extreme object oriented program, there would be no such switch statements. Instead, each of the eight kinds of move would have its own class, and each of these classes would have its own methods for execution, retraction, and formatting.

This extreme approach makes for a hell of a mess. While the various methods for, say, formatting are all different, they are also quite closely semantically related. But instead of having some spacial coherence in the source, they are scattered throughout the class definitions. At compile time, the compiler likely has less information available for optimization. At run time, there will always be the extra cost of invoking scattered object methods instead of execution of inline code.

Similarly, having different classes for the pieces is bad and will scatter the code for generation, evaluation, and other computational needs all over the source. Really, if having a class which holds a single scalar is a poor idea, then having a class which represents a constant can only be worse.

A switch statement is not evil, and its use is not in conflict with real OOP. A switch is much easier to code and to read than an long if else if chain, and it is well structured compared to any goto usage.

In Symbolic's source, only about one out of five hundred lines marks the beginning of a switch statement. There are only two instances of a nested switch. In no way are any of the program's use of a switch violations of valid OOP principles.

Remember: A switch is not a witch, there is no need for fear.

[Sven]

A switch statement is not evil, and its use is not in conflict with real OOP. A switch is much easier to code and to read than an long if else if chain, and it is well structured compared to any goto usage.

I fully agree, and would like to add that it is also not inefficient when used with case labels that are consecutive numbers, as in 0, 1, 2, 3, 4, 5. The compiler can and will generate code that jumps directly to the right case-specific code with O(1) effort instead of O(n).

Especially I also agree to your important note that modelling a constant (e.g. BlackKnight) as a class is really poor design. You perfectly explained why.

Sven

[Henk]

I just don't like switch statements. Actually I want to minimize if statements as well. If methods get longer it's more difficult to see which else matches which if. I want to keep methods short.

The more classes the shorter the methods. If functions are spread over classes one can use the Visitor design pattern to keep it together.

I do not like deep inheritance chains of classes as well.

I just want to simplify code as much as possible but efficiency ruins that.

[sje]

It would be sufficient to only have a (somewhat larger) Position class with low-level operations like

PutPieceOnBoard(Location location, PieceType pieceType)
RemovePieceFromBoard(Location location)
MovePiece(Location from, Location to)
PromotePiece(Location location, PieceType promotionPieceType)
UnpromotePiece(Location location)

and high-level operations like

MakeMove(Move move)
UnmakeMove(Move move)

that are based upon the first ones.

In Symbolic, the Position class has the private methods:

Code: Select all

  void AddMan(const Sq sq, const Color color, const Man man);
  void DelMan(const Sq sq, const Color color, const Man man);

  void MoveMan(const Sq frsq, const Sq tosq, const Color color, const Man man);

  void Capture(const Sq frsq, const Sq tosq,
               const Color frcolor, const Color tocolor,
               const Man frman, const Man toman);

  void RevCapt(const Sq frsq, const Sq tosq,
               const Color frcolor, const Color tocolor,
               const Man frman, const Man toman);

Which are called from the public methods:

Code: Select all

  void Execute(const Move move);
  void Retract(void);

Promotion is handled by calling DelMan() for the pawn and AddMan() for the new piece.

The Position methods do much of their work by calling corresponding methods for the Board, BBDB (bitboard database), Census, and Tracker class member variables. Example:

Code: Select all

void Position::Capture(const Sq frsq, const Sq tosq,
                       const Color frcolor, const Color tocolor,
                       const Man frman, const Man toman)
{
  fullhash.FoldHashManSq(toman, tosq);
  fullhash.FoldHashManSq(frman, frsq);
  fullhash.FoldHashManSq(frman, tosq);

  if (IsManKP(toman))
  {
    if (IsManKing(toman))
      ksbc[tocolor] = SqNil;
    else
      pawnhash.FoldHashManSq(toman, tosq);
  };

  if (IsManKP(frman))
  {
    if (IsManKing(frman))
      ksbc[frcolor] = tosq;
    else
    {
      pawnhash.FoldHashManSq(frman, frsq);
      pawnhash.FoldHashManSq(frman, tosq);
    };
  };

  tmbc[tocolor] -= Score::ManSvVec[toman];
  board.MoveMan(frsq, tosq);
  census.DelMan(tocolor, toman);
  mtrlhash.FoldMaterialHash(toman, census.GetManCount(toman));
  bbdb.Capture(frsq, tosq, frcolor, tocolor, frman, toman);
}

[Aleks Peshkov]

I just want to simplify code as much as possible but efficiency ruins that.

Typical bitboard program represent a piece as a single bit, how can your design compete in efficiency and simplicity of a bit?

[sje]

I should mention that nearly all of the methods used here are inline calls, so the parameter passing doesn't take any time in reality. Running the code in debug mode with no inline expansion is horrendously slow as you might imagine.

Nearly all the time used for piece motion is consumed by some BBDB (bitboard database) private methods which are not inline.

Code: Select all

  void AttackAdd(const Sq sq, const Color color, const Man man);
  void AttackDel(const Sq sq, const Color color);
  void AttackPro(const Sq sq);
  void AttackCut(const Sq sq);

These routines do what you think they do, operating on the private member variables:

Code: Select all

  Bitboard merge;             // All men merged
  Bitboard sweep;             // Sweeper men merged (bishops, rooks, queens)
  Bitboard locbm[ManRCLen];   // Locus by man
  Bitboard locbc[ColorRCLen]; // Locus by color
  Bitboard atkbc[ColorRCLen]; // Attacked by color
  Bitboard atkfs[SqLen];      // Attacks from a square
  Bitboard atkts[SqLen];      // Attacks to a square

Which have names (but not code!) shamelessly stolen from the 1970s Chess 4.x paper.

[Henk]

I finally removed my promotion stack that stored the pawns that were promoted. It never gave any problems and I even think it is faster then what I have now. For now I have to get a (new) pawn from the factory and set it's properties. Also when pawn gets promoted I have to put it back in the factory otherwise it runs out of pawns. So perhaps this solution is inferior unless a piece is not represented by an object but a (small) number.

By the way I forget that this gives opportunities to make move objects smaller.