Avoid misuse of StepAttacksBB for pawns

[snicolet]

Make it explicit that first index of StepAttacksBB is a piece, not a piece type.

Non functional change.

[mcostalba]

@snicolet first, also KING should be changed to W_KING:

StepAttacksBB[W_KING][ksq[us]] & ~(StepAttacksBB[W_KING][ksq[~us]] | StepAttacksBB[W_PAWN][psq]

Second, this is definitely overkill for me.

StepAttacksBB[] is an array, not a function. It can be indexed by a Piece, but why not by an int or a size_t?

Indeed PieceType is a strict subset of Piece and indexing StepAttacksBB[] with PieceType is fully safe and at most a very small style nuance, but the proposed "fix" is clearly less readable and needlessly pedantic IMO.

[snicolet]

@mcostalba

My reasoning is that for knight, bishop, rook, queen and king it doesn't matter if we access StepAttacksBB with a piece type rather than a piece, because these five pieces move backwards as well as forwards and the result will always be correct.

For instance StepAttacksBB[W_KING] == StepAttacksBB[B_KING] == StepAttacksBB[KING], and I'm perfectly fine with either style -- we could even use StepAttacksBB[KNIGHT] in line 83 of movegen.cpp to improve readability.

However, for pawns StepAttacksBB[W_PAWN] and StepAttacksBB[B_PAWN] are indeed subtly different. Hence the assert in position.h to protect newcomers from misuses (in master pos.attacks_from<PAWN>(s)compiles but only gives the white pawn attacks, even if square s contains a black pawn...).

Having put the assert in position.h, it is then natural to be more precise in the only two cases in the code where we use StepAttacksBB[PAWN] :-)

Hope this helps,
Stéphane

[mcostalba]

@snicolet yes, it helps :-)

You are correct!

[Rocky640]

In bitboards.cpp,
int steps[][9] = { {}, { 7, 9 }, { 17, 15, 10, 6, -6, -10, -15, -17 },
{}, {}, {}, { 9, 7, -7, -9, 8, 1, -1, -8 } };
means that StepAttacksBB is calculated only for pawn, knight and king.

Would it make sense to

• retire StepAttackBB[16][64]
• use, for the non-pawns, the PseudoAttackBB[8][64] (by piece type and square)
  (and add the StepAttackBB for knight, king in this array)
• use, for pawns, a new PawnAttackBB[2][64] (by color and square)

[lucasart]

@Rocky640: Indeed, this StepAttacksBB[] array was poor design. The color dependance is specific to pawns, and trying to generalise like this, proves neither useful nor convinient in practice.

[Rocky640]

Here is a first working version.
Rocky640@157a472

Some possible improvements:
a) Under inline Bitboard Position::attacks_from(Square s) const {
we might want to add an assert
assert(Pt != PAWN);

b) Maybe we can get rid of the following inline in positions.h, and use directly the PawnAttacks where applicable
inline Bitboard Position::attacks_from(Square s, Color c) const {

[Rocky640]

And this version only use PieceType (and not Piece) for "attacks_from"
I think it is better.
Rocky640@c84cbc3
You can view all the changes compared to master here:
master...Rocky640:RetireStepAttacks

[mcostalba]

@Rocky640 the alternative should be simpler than original. The fist one is not, the second one is a possible candidate but I have not looked at it deep enough.

[zamar]

Hmm... I agree that StepAttacks array is misdesigned.

[zamar]

Anyway this patch itself is fine, so I'm going to commit it.

[mcostalba]

@Rocky640 I have open a PR: #1086