koobas.hobune.stream
My code works but only with a waitforseconds() in a coroutine
Hi all, I'm currently makin a 2d chess game and I am trying to eliminate illegal moves from the highlighted tiles. The problem occurs when I want to make it so that pinned pieces (pieces where if they move the king will be under attack) moving is illegal.
To make these types of illegal moves I wrote code so that it will temporarily set the piece transform.position to each of the possible moves and test if the king is under attack. (The code is below) For pieces with only a few tiles to check it does this somewhat accurately, but with pieces with a lot of tiles to check like the queen, the "check if king is in danger" method seems to not work entirely.
The "check if king is in danger" method does work 100% since I checked after each turn and it consistently answered correctly if the king were in danger.
I then suspected that it can't handle checking each move with the "king in danger function" and thus labels each move as possible so I added everything to a coroutine and added a waitforseconds() of 0.1f seconds per move and now the code works.
However before highlighting available moves the piece visually moves around to each of the positions before showing possible moves, which doesn't exactly look appealing... I was wondering if there is anything I could do to make it efficient enough to check for illegal moves without the wait function?
private void OnMouseDown()
{
if (selected)
{
selected = false;
gm.selectedUnit = null;
gm.ResetTiles();
transform.localScale = new Vector3(flip * 1f, 1f);
}
else
{
transform.localScale = new Vector3(flip * 1.25f, 1.25f);
if (playerNumber == gm.playerTurn)
{
if (gm.selectedUnit != null)
{
gm.selectedUnit.selected = false;
}
selected = true;
gm.selectedUnit = this;
gm.ResetTiles();
eliminateIllegalMoves(playerNumber, GetWalkableTiles());
}
}
tileUnder = GetTileBelow();
tileUnder.OnMouseDown();
}
public void allPossibleMoves(int side)
{
foreach (Unit unit in FindObjectsOfType<Unit>())
{
if (unit.playerNumber == side)
{
foreach (Tile tile in unit.GetWalkableTiles())
{
tile.isAttacked = true;
}
}
}
}
public bool isKingInDanger(int whichPlayer)
{
foreach(Tile tile in FindObjectsOfType<Tile>())
{
tile.isAttacked = false;
}
int enemyNumber;
if (whichPlayer == 1)
{
enemyNumber = 2;
}
else
{
enemyNumber = 1;
}
allPossibleMoves(enemyNumber);
foreach (Unit possibleKing in FindObjectsOfType<Unit>())
{
if (possibleKing.pieceType == 1 && possibleKing.playerNumber == whichPlayer)
{
if (possibleKing.GetTileBelow().isAttacked)
{
return true;
}
else
{
return false;
}
}
}
return false;
}
public List<Tile> eliminateIllegalMoves(int whichPlayer, List<Tile> allMoves)
{
legalMoves = new List<Tile>();
StartCoroutine(eraseMoves(whichPlayer, allMoves));
return legalMoves;
}
IEnumerator eraseMoves(int whichPlayer, List<Tile> allMoves)
{
Vector3 temp = transform.position;
foreach (Tile tile in allMoves)
{
tile.isLegal = true;
transform.position = tile.transform.position;
//yield return new WaitForSeconds(0.1f);
if (isKingInDanger(whichPlayer))
{
tile.isLegal = false;
}
if (tile.isLegal)
{
legalMoves.Add(tile);
}
}
transform.position = temp;
HighlightWalkableTiles(legalMoves);
yield return null;
}
Hey there,
i cannot really see a direct way how to solve this issue but can offer a long term solution anyway:
this is a point in your development where you should take 5 steps back and rework some stuff.
Why? Because you should not be dependant on actually moving parts to check if a piece is checked or not. You should be able to just "simulate" this. Basically the best way would be to have the chess logic that only exists in non-monobehaviours and unitys only part is to take input, trigger functions in your chess logic and at the end of all calculations to display the visuals.
Why is that so important? Multiple reasons. Your current system is horrible on performance. You have a loop in line 87 that calls a function where each iteration calls 2 instances of FindObjectsOfType (which should be avoided in general, but is ok in Start functions) where each time the same objects are beeing searched.
your current system with your coroutine gives 1 frame of possibility in which an impossible move might be executed when someone clicks reaaaaally fast.
In general i'd strongly suggest you rebuild some systems to simply eleminate your problem by a rework. When you do this you should also read into enum as you can create something like this:
enum Pieces { King, Queen, Pawn .....}
so that in your code you don't have to have int piecetype = 1 where noone knows what it actually is but you can say Pieces piecetype = Pieces.King; and check for a type with if(piecetype == Pieces.Pawn) for example.
On top of that your logic should be (as already pointed out) based on functions which take objects with 2D Vectors as positions that are not monobehaviours so your don't have to actually set transform positions.
Thanks so much for your reply! I'm still new to this so your feedback is really helpful! So what I'm should do is create a separate script that simulates the chessboard and checks for moves and all that and only use the unity gameobjects as actors for that simulated chessboard once a user makes a move?
I think you understood what i tried to describe in my post yes. In the end your current logic already does something similar. You have some objects with a variable position which you take and then calculate some things based on the position of all objects.
So by taking Unity out of this you can speed things up, make them testable and more slim in general.
I like how your put it with "unity gameobjects as actors". That is just perfect.
As an addition for my answer: An enum can also be used perfectly to encapsulate the player you are currently talking about. (e.g enum PlayerColor {White, Black} - you get the idea)
As a nifty little thing that might intrest you is this:
if for example you create an enum for the playercolors black and white:
enum PlayerColor {White, Black};
you can add an extension method for the PlayerColor enum like this to get the other player color in one line wherever you want:
public static class PlayerColorExtensions
{
public static PlayerColor OtherPlayer(this PlayerColor thisPlayer)
{
if(thisPlayer == PlayerColor.White)
return PlayerColor.Black;
return PlayerColor.White;
}
}
so when you have a variable that is defined like this:
PlayerColor playerColorOfThisPiece;
then you can get the other color by:
var otherColor = playerColorOfThisPiece.OtherPlayer();
Not tested but should work. You may scold me for talking s**t if it doesn't :)
