Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop / Promotion / Multi-Step Region Per Piece Type (Closing Milestone v14.0.2) #792

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

yjf2002ghty
Copy link
Contributor

@yjf2002ghty yjf2002ghty commented May 3, 2024

Pull Request Target

#618 and Milestone https://github.com/fairy-stockfish/Fairy-Stockfish/milestone/10

Overview

After this change, I added a structure PieceTypeBitboardGroup consisting of 26 bitboards as a vector to store the corresponding bitboards of each piece type (from A to Z, which is 26). It also supports parsing from variants.ini.

Extra Options

The following options are added:

Piece Specific Drop Region

bool pieceSpecificDropRegion - When true, enable "Drop Region Per Piece Type" feature, and piece specific drop regions are applied based on dropRegionWhite / dropRegionBlack (i.e. It only filters moves that not allowed in piece specific drop regions based on dropRegionWhite / dropRegionBlack and does not add a drop that is allowed in piece specific drop regions as a new drop if the drop is banned in dropRegionWhite / dropRegionBlack). When false, does nothing. Existing variants are not affected.
PieceTypeBitboardGroup whitePieceDropRegion - The region of different pieces that can drop for white.
PieceTypeBitboardGroup blackPieceDropRegion - The region of different pieces that can drop for black.

Piece Specific Promotion Region

bool pieceSpecificPromotionRegion - When true, enable "Promotion Region Per Piece Type" feature, and piece specific promotion regions are applied based on promotionRegionWhite / promotionRegionBlack (i.e. It only filters moves that not allowed in piece specific promotion regions based on promotionRegionWhite / promotionRegionBlack and does not add a promotion that is allowed in piece specific promotion regions as a new promotion if the promotion is banned in promotionRegionWhite / promotionRegionBlack). When false, does nothing. Existing variants are not affected.
PieceTypeBitboardGroup whitePiecePromotionRegion - The region of different pieces that can promote for white.
PieceTypeBitboardGroup blackPiecePromotionRegion - The region of different pieces that can promote for black.

Piece Specific Multi-Step Region

Note: Enabling "Piece Specific Multi-Step Region" will allow non-pawn pieces to move forward for N squares without capturing. The move is considered a a sliding move. For example, when double stepping enabled for piece A, it will have fW2 as an extra betza notation move as long as it is not moved.

Double Stepping

bool pieceSpecificDoubleStepRegion - When true, enable "Double Step Region Per Piece Type" feature, and piece specific double step regions are applied based on doubleStepRegionWhite / doubleStepRegionBlack (i.e. It only filters moves that not allowed in piece specific double step regions based on doubleStepRegionWhite / doubleStepRegionBlack and does not add a double step that is allowed in piece specific double step regions as a new double step if the double step is banned in doubleStepRegionWhite / doubleStepRegionBlack). When false, does nothing. Existing variants are not affected.
PieceTypeBitboardGroup whitePieceDoubleStepRegion - The region of different pieces that can double step for white.
PieceTypeBitboardGroup blackPieceDoubleStepRegion - The region of different pieces that can double step for black.

Triple Stepping

bool pieceSpecificTripleStepRegion - When true, enable "Triple Step Region Per Piece Type" feature, and piece specific triple step regions are applied based on tripleStepRegionWhite / tripleStepRegionBlack (i.e. It only filters moves that not allowed in piece specific triple step regions based on tripleStepRegionWhite / tripleStepRegionBlack and does not add a triple step that is allowed in piece specific triple step regions as a new triple step if the triple step is banned in tripleStepRegionWhite / tripleStepRegionBlack). When false, does nothing. Existing variants are not affected.
PieceTypeBitboardGroup whitePieceTripleStepRegion - The region of different pieces that can double step for white.
PieceTypeBitboardGroup blackPieceTripleStepRegion - The region of different pieces that can double step for black.

Testing

Test File

# Test pieceSpecificDropRegion.
# Expected result: 
# White Pawns can be only dropped on the e file
# Black Pawns can be only dropped on the d file
# White Queen can be only dropped on rank 1
# Black Queen can be only dropped on rank 8
# Rooks can be dropped anywhere
# White Knight can be only dropped on e3 and d3
# Black Knight can be only dropped on e6 and d6
# Bishops cannot be dropped

[test001:crazyhouse]
pieceSpecificDropRegion = true
whitePieceDropRegion = P(e*);Q(*1);R(**);N(e3, d3);
blackPieceDropRegion = P(d*);Q(*8);R(**);N(e6, d6);


# Test pieceSpecificPromotionRegion
# Expected result: 
# White Pawns can promote at rank 8 as well as b4, d4, f4, h4
# Black Pawns can promote at rank 1 as well as a5, c5, e5, g5
# White Knights can promote to rooks at f3, c3
# Black Knights can promote to rooks at f6, c6
# Note: This feature might conflict with custom pawn types in endgame.cpp system, when pawn = -

[test002:chess]
promotionRegionWhite = *1 *2 *3 *4 *5 *6 *7 *8
promotionRegionBlack = *1 *2 *3 *4 *5 *6 *7 *8
promotedPieceType = n:r
pieceSpecificPromotionRegion = true
whitePiecePromotionRegion = P(*8, b4, d4, f4, h4);N(f3, c3);
blackPiecePromotionRegion = P(*1, a5, c5, e5, g5);N(f6, c6);


# Test pieceSpecificDoubleStepRegion and pieceSpecificTripleStepRegion
# Expected result: 
# White Pawns can double step at a2, c2, e2, g2 while triple step at b2, d2, f2, h2
# Black Pawns can triple step at a7, c7, e7, g7 while double step at b7, d7, f7, h7
# White Bishops can double step forward at c1, f1
# Black Bishops can triple step forward at c8, f8
# Note: This feature might conflict with custom pawn types in endgame.cpp & evaluate.cpp system, when pawn = -

[test003:chess]
doubleStep = true
doubleStepRegionWhite = *1 *2 *3 *4 *5 *6 *7 *8
doubleStepRegionBlack = *1 *2 *3 *4 *5 *6 *7 *8
tripleStepRegionWhite = *1 *2 *3 *4 *5 *6 *7 *8
tripleStepRegionBlack = *1 *2 *3 *4 *5 *6 *7 *8
pieceSpecificDoubleStepRegion = true
pieceSpecificTripleStepRegion = true
whitePieceDoubleStepRegion = P(a2, c2, e2, g2);B(c1, f1);
blackPieceDoubleStepRegion = P(b7, d7, f7, h7);
whitePieceTripleStepRegion = P(b2, d2, f2, h2);
blackPieceTripleStepRegion = P(a7, c7, e7, g7);B(c8, f8);


# Test combination of the 3 above
# Expected result: 
# White Pawns can be only dropped on the e file
# Black Pawns can be only dropped on the d file
# White Queen can be only dropped on rank 1
# Black Queen can be only dropped on rank 8
# Rooks can be dropped anywhere
# White Knight can be only dropped on e3 and d3
# Black Knight can be only dropped on e6 and d6
# Bishops cannot be dropped
# White Pawns can promote at rank 8 as well as b4, d4, f4, h4
# Black Pawns can promote at rank 1 as well as a5, c5, e5, g5
# White Knights can promote to rooks at f3, c3
# Black Knights can promote to rooks at f6, c6
# White Pawns can double step at a2, c2, e2, g2 while triple step at b2, d2, f2, h2
# Black Pawns can triple step at a7, c7, e7, g7 while double step at b7, d7, f7, h7
# White Bishops can double step forward at c1, f1
# Black Bishops can triple step forward at c8, f8
# Note: This feature might conflict with custom pawn types in endgame.cpp & evaluate.cpp system, when pawn = -

[test004:crazyhouse]
pieceSpecificDropRegion = true
whitePieceDropRegion = P(e*);Q(*1);R(**);N(e3, d3);
blackPieceDropRegion = P(d*);Q(*8);R(**);N(e6, d6);
promotionRegionWhite = *1 *2 *3 *4 *5 *6 *7 *8
promotionRegionBlack = *1 *2 *3 *4 *5 *6 *7 *8
promotedPieceType = n:r
pieceSpecificPromotionRegion = true
whitePiecePromotionRegion = P(*8, b4, d4, f4, h4);N(f3, c3);
blackPiecePromotionRegion = P(*1, a5, c5, e5, g5);N(f6, c6);
doubleStep = true
doubleStepRegionWhite = *1 *2 *3 *4 *5 *6 *7 *8
doubleStepRegionBlack = *1 *2 *3 *4 *5 *6 *7 *8
tripleStepRegionWhite = *1 *2 *3 *4 *5 *6 *7 *8
tripleStepRegionBlack = *1 *2 *3 *4 *5 *6 *7 *8
pieceSpecificDoubleStepRegion = true
pieceSpecificTripleStepRegion = true
whitePieceDoubleStepRegion = P(a2, c2, e2, g2);B(c1, f1);
blackPieceDoubleStepRegion = P(b7, d7, f7, h7);
whitePieceTripleStepRegion = P(b2, d2, f2, h2);
blackPieceTripleStepRegion = P(a7, c7, e7, g7);B(c8, f8);

Test Results

  • Pass continuous integration
  • No assertion error during execution using built-in variants and the test variant given above when debug=yes
  • The search & evaluation results are reproducible in single-threaded environment
  • ffish.js & pyffish works as intended
  • No performance reduction

Potential Problems

Compilation Problem (Moved to #810 )

Cannot apply optimization -fstrict-aliasing in GCC as it breaks program logic. The error is reproducible on my machine and the reason is unknown.

Here is an error message:

make[2]: Leaving directory '/e/Fairy-Stockfish-master/test/Fairy-Stockfish/src'
make[1]: Leaving directory '/e/Fairy-Stockfish-master/test/Fairy-Stockfish/src'

Step 2/4. Running benchmark for pgo-build ...
./stockfish.exe bench > /dev/null
Assertion failed: r, file bitboard.h, line 467
make: *** [Makefile:803: profile-build] Error 3

Evaluation (Hand-crafted) & Endgame Play Strength Problem

In evaluate.cpp and endgame.cpp, some of the double / triple step regions are set to pawn's region which may cause a play strength reduction if the variant is using a custom piece as a pawn (e.g. using pawnTypes in the variant configuration). I think hand crafted evaluation play strength is a key factor which is as important as generality so some further changes are needed.

@ianfab
Copy link
Member

ianfab commented May 3, 2024

Thanks. Is there a specific reason why you didn't choose a similar approach as for other config options, i.e., to simply have a 2x64 array of Bitboards in the variant object? My first impression is that this feels a bit too complex for what it does.

@yjf2002ghty

This comment was marked as outdated.

@yjf2002ghty

This comment was marked as outdated.

@ianfab
Copy link
Member

ianfab commented May 3, 2024

All variant objects, whether built-in or parsed from the INI, are dynamically allocated, so I would assume they and their (POD) content are on the heap and not the stack. Or am I mistaken?

@yjf2002ghty

This comment was marked as outdated.

@yjf2002ghty

This comment was marked as outdated.

@ianfab
Copy link
Member

ianfab commented May 3, 2024

For the purposes of maintenance I have to assume C++ does what it is supposed to do unless there is a reproducible issue showing the contrary, so for now I will assume variant objects are not an issue for stack usage.

Adding substructure can maybe be considered, but I would prefer to keep memory management simple and follow a POD design. Also consistency is important, so if substructure is introduced it should be considered what should long-term be migrated to a similar approach to make it consistent and whether that is worth it.

@yjf2002ghty

This comment was marked as outdated.

@yjf2002ghty yjf2002ghty force-pushed the patch-1 branch 2 times, most recently from 80d9cba to ce2c450 Compare May 5, 2024 13:08
@yjf2002ghty
Copy link
Contributor Author

yjf2002ghty commented May 7, 2024

@ianfab I've tried to add features in #618 and updated the PR description. You can test my changes using the provided variants.ini in the description at the top.
If there are anything problematic, please point it out.

@yjf2002ghty yjf2002ghty changed the title Drop Region Per Piece Type Drop / Promotion / Multi-Step Region Per Piece Type (Closing Milestone v14.0.2) May 7, 2024
@yjf2002ghty

This comment was marked as resolved.

@ianfab
Copy link
Member

ianfab commented Jun 1, 2024

Thanks a lot for looking into the compiler issue. As far as I can see this seems independent from the topic of this PR though, so I would like to ask to separate the PRs. For context, with non-core subprojects like fairyground I am much more relaxed about mixing topics in one PR or things like that, but Fairy-SF itself is the core which many projects rely on, so I have to be very strict and cautious about every non-trivial change here.

Regarding the added comments, I am generally open to adding more comments to make the project more accessible, but my usual policy is to focus on explaining differences to the base project, and inheriting other comments from upstream, which also reduces merge conflicts. As for the prefix of the comments, you can remove these, since the author of the code/comment usually is not relevant information, and in the rare case that it is, git history (e.g., git blame) can tell that.

@yjf2002ghty

This comment was marked as outdated.

@ianfab
Copy link
Member

ianfab commented Jun 1, 2024

Sorry, maybe what I said was a bit ambiguous. What I wanted to say is that unrelated changes should be separated, so that they can be reviewed and evaluated independently. Changes that are strongly related or dependent, e.g., because they rely on the same new code infrastructure (like a new type) usually belong in the same PR, since they can not reasonably be split without introducing interdependency of PRs. As far as I can see the only independent topics here are the introduction of the new config type (along with specific usages of it) as well as the bugfix, so that would be the two PRs from my perspective.

@yjf2002ghty

This comment was marked as resolved.

@yjf2002ghty

This comment was marked as resolved.

@ianfab
Copy link
Member

ianfab commented Jul 8, 2024

Thanks. I actually also get the failed assertion on master locally (g++ (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0), so I don't think it is related to your changes. I do not fully understand yet what is the cause, but maybe calling internals like __builtin_ctzll on enums might constitute an alias violation, not sure. -fno-strict-aliasing also fixes it for me though. So I think it would be best to have the makefile change and the CI changes as an upfront PR to fix the issue, then this PR can focus purely on the features.

@yjf2002ghty
Copy link
Contributor Author

@ianfab I've separated the fix of assertion error and the addition of these new features.
The new pull request is #810 .

Signed-off-by: yjf2002ghty <47345902+yjf2002ghty@users.noreply.github.com>
@yjf2002ghty
Copy link
Contributor Author

@ianfab I've removed prefixes of comments and cleared unnecessary comments.

Signed-off-by: yjf2002ghty <47345902+yjf2002ghty@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants