improving storage of coordinates #75

JeromeSchmied · 2024-06-03T18:53:02Z

Title

Description

IMO the current implementation for coordinations [i8; 2] is not straightforward at all, also doesn't utilize Rust's awesome type-safety features.

I made a

struct Coord {
    row: u8,
    col: u8,
}

for this, which I think fits the purpose.

I've also implemented std::ops::Index and std::ops::IndexMut for Board to be able to index it with Coord, just like @joshka suggested.

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
My changes generate no new warnings
New and existing unit tests pass locally with my changes

JeromeSchmied · 2024-06-09T11:11:49Z

But I have difficulties, as for several functions, only board::Board.board is passed, for which I can not implement std::ops::Index, as it's not a struct or anything defined in this crate, and therefore I can't utilize the [] operator in those functions, which is quite sad.

To address this issue, what I could do is:

make a new

 struct JustTheBoard { board: [[Option<(PieceType, PieceColor)>; 8]; 8] }

and implement std::ops::Index for that

to every function, that only gets board: [[Option<(PieceType, PieceColor)>; 8]; 8], pass the whole board::Board struct
almost same as previous: make functions, that use board: [[Option<(PieceType, PieceColor)>; 8]; 8] methods of board::Board

thomas-mauran · 2024-06-11T12:01:44Z

Looking it soon

nicholasmello · 2024-06-19T03:12:25Z

But I have difficulties, as for several functions, only board::Board.board is passed, for which I can not implement std::ops::Index, as it's not a struct or anything defined in this crate, and therefore I can't utilize the [] operator in those functions, which is quite sad.

To address this issue, what I could do is:
* make a new
 struct JustTheBoard { board: [[Option<(PieceType, PieceColor)>; 8]; 8] }
and implement std::ops::Index for that
* to every function, that only gets `board: [[Option<(PieceType, PieceColor)>; 8]; 8]`, pass the whole `board::Board` struct

* almost same as previous: make functions, that use `board: [[Option<(PieceType, PieceColor)>; 8]; 8]` methods of `board::Board`

If we defined our own type, something like type Board = [[Option<(PieceType, PieceColor)>; 8]; 8];, we should be able to implement using it without the need for the struct. I coded up a quick test just to make sure it would work.

type Board = [[Option<i32>; 2]; 2];

struct Coord {
    row: u8,
    col: u8
}

impl std::ops::Index<&Coord> for Board {
    type Output = Option<i32>;

    fn index(&self, index: &Coord) -> &Self::Output {
        &self[index.row as usize][index.col as usize]
    }
}

fn main() {
    let board: Board = [
        [Some(0),Some(1)],
        [Some(2),Some(3)],
    ];
    let coord: Coord = Coord {row: 1, col: 0};
    println!("{:?}", board);
    println!("{:?}", board[&coord]);
}

nicholasmello

Overall love the changes, we should probably squash the commits before merging when we get to that point

nicholasmello · 2024-06-19T02:27:02Z

src/pieces/knight.rs

-            let new_coordinates = [y + dy, x + dx];
-
-            if !is_valid(new_coordinates) {
+            let Some(new_coordinates) =
+                Coord::opt_new(coordinates.row as i8 + dy, coordinates.col as i8 + dx)
+            else {


Missing is_valid check for the knight moves

please check whether this commit: be4b1e4 fixes this

nicholasmello · 2024-06-19T02:32:48Z

src/board.rs

+impl std::ops::Index<&Coord> for Board {
+    type Output = Option<(PieceType, PieceColor)>;
+
+    fn index(&self, index: &Coord) -> &Self::Output {
+        &self.board[index.row as usize][index.col as usize]
+    }
+}
+
+impl std::ops::IndexMut<&Coord> for Board {
+    fn index_mut(&mut self, index: &Coord) -> &mut Self::Output {
+        &mut self.board[index.row as usize][index.col as usize]
+    }
+}
+


I might be missing it but I don't see these functions being used anywhere

Ignore this, I didn't fully read your comment... my bad 😳

nicholasmello · 2024-06-19T02:42:42Z

src/pieces/bishop.rs

-        for i in 1..8i8 {
-            let new_x = x + i;
-            let new_y = y - i;
-            let new_coordinates = [new_y, new_x];
-
-            // Invalid coords
-            if !is_valid(new_coordinates) {
+        for i in 1..8u8 {
+            let new_x = x as i8 + i as i8;
+            let new_y = y as i8 - i as i8;
+            let Some(new_coordinates) = Coord::opt_new(new_y, new_x) else {


x and y are of type u8 since they came from the new Coord, I think it would be better to keep them as u8 - u8 with checked sub to prevent a panic. Something like:

let some(new_x) = x.checked_sub(i) else { // Same thing as our invalid coord case }

Also this is another case where we lost the is_valid check on the coordinates

nicholasmello · 2024-06-19T02:49:19Z

src/pieces/pawn.rs

-        let new_y_front_one = y + direction;
-        let new_coordinates_front_one = [new_y_front_one, new_x_front_one];
+        let new_y_front_one = y as i8 + direction;
+        let new_coordinates_front_one = Coord::new(new_y_front_one as u8, new_x_front_one);


I think this is another case where using checked_sub to avoid the signed and unsigned conversions would look a lot better

In this case especially, since we are just converting right back to u8, we aren't gaining any prevention from panicking over just doing the subtraction and unwrapping or something.

I don't want to leave a ton of comments but this applies to most of the rest of this file

I agree with you, but what you're suggesting might require loads of changes.
My idea is to make Coord::opt_new() check validity as well as overflow kind of things, I'll push the changes soon.

thomas-mauran

A very nice addition overall ! thanks a lot to you and @nicholasmello for the additional comments, I don't see anything else that could be a problem. Sorry for the long delay of answer I have been occupied with a lot of other things those past weeks :D.

Waiting for you to take care of the suggestions, squash some of the commits and rebase from main to make sure there is no conflict and it's a merge ! thanks again for taking the time to improve chess-tui 🎉

JeromeSchmied · 2024-06-28T19:17:27Z

Okay guys, thanks for the suggestions, I'll see what can be done and check back soon.
One more thing though: what does squashing commits mean?

nicholasmello · 2024-06-29T01:57:49Z

Okay guys, thanks for the suggestions, I'll see what can be done and check back soon. One more thing though: what does squashing commits mean?

Squash is a rebase option to combine multiple commits into a single one. It is often done before merges to keep git history simpler, especially when history includes a lot of fixes that are related to each other. It doesn't have to be a single commit for something this big but fewer commits to not spam the git history would be preferred.

JeromeSchmied · 2024-07-01T16:22:09Z

okay, so if I squash the commits, is this pr ready to merge?

JeromeSchmied · 2024-07-05T20:58:07Z

squashing done

nicholasmello

Overall, looks great! Just a couple places I found where we can fully switch over to the new type :)

nicholasmello · 2024-07-05T23:03:26Z

src/board.rs

+}
+
+impl std::ops::Index<&Coord> for GameBoard {
+    type Output = Option<(PieceType, PieceColor)>;


This can be the new GameBoard type

It wont let me add a separate comment since it isn't edited code but same applies for pub fn is_piece_opposite_king(piece: Option<(PieceType, PieceColor)>, color: PieceColor) -> bool { in utils.rs on line 229.

I'm sorry, but I'm afraid, you're mistaken.
The GameBoard looks like: [[Option<(PieceType, PieceColor)>; 8]; 8], while the ones you suggested to change are only Option<(PieceType, PieceColor)> which we don't (yet) have a seperate type for.
Please correct me if I'm wrong.

JeromeSchmied · 2024-07-15T17:35:36Z

is there anything else, I may do?

thomas-mauran · 2024-07-15T20:55:04Z

Nope my bad forgot to merge that, doing that tomorrow for sure been a bit overbooked recently 😅 @JeromeSchmied

thomas-mauran · 2024-07-16T18:30:05Z

Thank you very much for your contribution @JeromeSchmied and thx for the review @nicholasmello 💪

JeromeSchmied marked this pull request as ready for review June 6, 2024 17:27

nicholasmello suggested changes Jun 19, 2024

View reviewed changes

thomas-mauran reviewed Jun 22, 2024

View reviewed changes

fix: Coord struct for storing coordinates

a101066

JeromeSchmied force-pushed the coord branch from cc3a480 to a101066 Compare July 5, 2024 11:14

fix(Coord): opt_new() is cleaner w/ TryInto

163402d

nicholasmello reviewed Jul 5, 2024

View reviewed changes

thomas-mauran merged commit 84c4525 into thomas-mauran:main Jul 16, 2024
4 checks passed

thomas-mauran added the enhancement New feature or request label Nov 13, 2024

thomas-mauran assigned nicholasmello and JeromeSchmied Nov 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improving storage of coordinates #75

improving storage of coordinates #75

JeromeSchmied commented Jun 3, 2024 •

edited

Loading

JeromeSchmied commented Jun 9, 2024 •

edited

Loading

thomas-mauran commented Jun 11, 2024

nicholasmello commented Jun 19, 2024

nicholasmello left a comment

nicholasmello Jun 19, 2024

JeromeSchmied Jun 28, 2024

nicholasmello Jun 19, 2024

nicholasmello Jun 19, 2024

nicholasmello Jun 19, 2024

nicholasmello Jun 19, 2024

nicholasmello Jun 19, 2024

JeromeSchmied Jun 28, 2024

thomas-mauran left a comment

JeromeSchmied commented Jun 28, 2024

nicholasmello commented Jun 29, 2024

JeromeSchmied commented Jul 1, 2024

JeromeSchmied commented Jul 5, 2024

nicholasmello left a comment

nicholasmello Jul 5, 2024

nicholasmello Jul 5, 2024

JeromeSchmied Jul 6, 2024

JeromeSchmied commented Jul 15, 2024

thomas-mauran commented Jul 15, 2024

thomas-mauran commented Jul 16, 2024

improving storage of coordinates #75

improving storage of coordinates #75

Conversation

JeromeSchmied commented Jun 3, 2024 • edited Loading

Title

Description

Checklist:

JeromeSchmied commented Jun 9, 2024 • edited Loading

thomas-mauran commented Jun 11, 2024

nicholasmello commented Jun 19, 2024

nicholasmello left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomas-mauran left a comment

Choose a reason for hiding this comment

JeromeSchmied commented Jun 28, 2024

nicholasmello commented Jun 29, 2024

JeromeSchmied commented Jul 1, 2024

JeromeSchmied commented Jul 5, 2024

nicholasmello left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeromeSchmied commented Jul 15, 2024

thomas-mauran commented Jul 15, 2024

thomas-mauran commented Jul 16, 2024

JeromeSchmied commented Jun 3, 2024 •

edited

Loading

JeromeSchmied commented Jun 9, 2024 •

edited

Loading