Relax fen parsing - #7010
Conversation
|
This wasn't added without a ground. |
|
On a further reading of #4563 and #6665, I agree this check shouldn't be removed entirely: it prevents constructed positions with too many pseudolegal moves overflowing our movelist. However, it's also explicit that the scope of SF's FEN sanitation is restricted to the minimum set of positions that leads to unsafe behavior. This check is in my opinion way too broad and we can probably achieve something of similar effect by checking the piececount consistency of queen + pawn only |
|
Queen and pawn should be enough, ya. But there’s also another approach if we’re feeling adventurous: lift MAX_MOVES (which is quite low overhead, just a slightly larger stack frame) and get rid of count checks entirely, except a total of <= 32 |
|
I don't see the point of this change. If you're trying to feed illegal positions into |
|
If a revision is required I can make another pull request if you let me know what to change? As for why this one is useful for me personally, it is to allow for many more interesting custom positions to be tested, not for real games. Which is a comprimise I'm assuming is acceptable so long as the change isn't detrimental to real chess. |
|
The advantage of the current strict behavior is that the contract between devs and users is very clear: normal chess (and FRC) only. This allows us to train networks from standard chess positions and perform optimizations (e.g. limit the size of the internal pseudolegal move list). If you want to play with exotic positions, it's still possible to compile your own version. But beware it can crash currently due to optimizations and it might perform bad, because the engine is pushed outside its normal working regime. The latter argument, and the fact that "development" should not commit to this because of even more potential future optimizations, make it hard to justify merging this IMHO. |
There is no fixing this. You can't allow positions that break the invariant all stockfish's code relies on. It may work fine today but crash after a patch in 2 months. |
I am not sure what you mean by an "illegal position". If it is a position that is not reachable from the starting position then the terms of the contract cannot be verified. So it is not a good contract. I would recommend defining illegal position differently. |
|
As it stands, a "legal position" for our purposes is simply:
That's it, we can commit to supporting any position of this type without a real risk of missing some optimizations. Maybe we should write these down somewhere.... Note that common classes of unreachable positions (e.g. an impossible en passant, weird pawn structures) are handled just fine. That's obviously a (strict) superset of positions reachable from startpos. Parts of the code that depend on each of these factors: (1): Movegen output, feature index lists. We could loosen the rules slightly (as this PR does), but we don't want to rule out future optimizations, and the current criterion is very clear. Nothing depends on some subtler aspect of reachability, and it's hard to imagine an optimization that could actually benefit from that. |
Removes some piece count detections for technically impossible to reach, but reasonable cases with almost certainly no issues.