Skip to content

solving 5 scramble#18

Open
ShahriarAhnaf wants to merge 2 commits into
mainfrom
solving_steps_fix
Open

solving 5 scramble#18
ShahriarAhnaf wants to merge 2 commits into
mainfrom
solving_steps_fix

Conversation

@ShahriarAhnaf

Copy link
Copy Markdown
Owner

Doing a index based inner loop search to fix the performance bottlekeck. Several correctness fixed from adding tests.

…eck. Several correctness fixed from adding tests.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves Rubik’s Cube solver performance by switching the DFS inner loop to index-based move application, and adds/updates tests to catch move/solver correctness issues.

Changes:

  • Replaced string-based DFS expansion with an index-based DFS (apply_move_index) and path reconstruction in Solver.
  • Reworked Solve_Cube into staged target-state solves (plus a quick full-solve attempt for short scrambles).
  • Overhauled the test suite into a simple PASS/FAIL runner and expanded move/solver correctness checks.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
Rubix/Testing/Test_Cube.cpp Replaced interactive/assert-style tests with a small test harness and added more correctness/solver scenarios.
Rubix/src/Solver.cpp Implemented faster IDFS/DFS using move indices; refactored Solve_Cube into staged target-state solving with a “quick solve” attempt.
Rubix/src/Cube.cpp Fixed solved-state initialization, corrected U_PRIME/D_PRIME multi-turn handling, and added apply_move_index; extended apply_moves to parse *2 moves.
Rubix/Inc/Solver.h Added target “relevant” helpers and declared the new fast DFS/IDFS APIs + index redundancy pruning.
Rubix/Inc/Cube.h Exposed apply_move_index(int) on the cube API.
Comments suppressed due to low confidence (1)

Rubix/src/Solver.cpp:10

  • DFS_count is declared as a file-scope static but is no longer used after the refactor to the member dfs_count. With -Wall -Wextra this will trigger an unused-variable warning; please remove it (or wire it up consistently if it’s still needed).
static long int DFS_count = 0;
// static long int IDFS_count = 0;

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread Rubix/Testing/Test_Cube.cpp Outdated
std::string solution = solver.Solve_Cube(cube, 5);
bool ok = is_cube_solved(cube);
std::string label = std::string("Scramble: ") + scramble;
check(ok, label.c_str());

Copilot AI Mar 31, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test_solver_one_move() assigns solution but never uses it, which will trigger an unused-variable warning under -Wall -Wextra. Consider removing it, or additionally asserting that applying solution to an equivalent scrambled cube produces a solved cube (verifies returned strings are correct).

Suggested change
check(ok, label.c_str());
check(ok, label.c_str());
// Additionally verify that the returned solution string actually solves
// an equivalently scrambled cube.
RubixCube cube2;
cube2.apply_moves(scramble);
cube2.apply_moves(solution.c_str());
bool ok_solution = is_cube_solved(cube2);
std::string label_solution = std::string("Solution applies for scramble: ") + scramble;
check(ok_solution, label_solution.c_str());

Copilot uses AI. Check for mistakes.
Comment thread Rubix/src/Cube.cpp
Comment on lines +531 to +552
void RubixCube::apply_move_index(int move_index) {
switch(move_index) {
case 0: L(1); break;
case 1: L_PRIME(1); break;
case 2: L(2); break;
case 3: R(1); break;
case 4: R_PRIME(1); break;
case 5: R(2); break;
case 6: U(1); break;
case 7: U_PRIME(1); break;
case 8: U(2); break;
case 9: D(1); break;
case 10: D_PRIME(1); break;
case 11: D(2); break;
case 12: F(1); break;
case 13: F_PRIME(1); break;
case 14: F(2); break;
case 15: B(1); break;
case 16: B_PRIME(1); break;
case 17: B(2); break;
}
}

Copilot AI Mar 31, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apply_move_index() silently does nothing for out-of-range values (no default case / validation). Since this is now a public API and is used for solver correctness/perf, consider adding a bounds check (or assert(move_index >= 0 && move_index < 18)) to fail fast on invalid indices instead of producing incorrect/partial solves silently.

Copilot uses AI. Check for mistakes.
Comment thread Rubix/src/Solver.cpp
Comment on lines +320 to +355
std::string moves = Solve_IDFS(given_cube, white_cross, Depth_Limit);
if(!moves.empty()) {
given_cube = Apply_Moves(given_cube, moves);
all_moves += moves;
}

// --- Step 2: First layer (white face complete + top row of side faces) ---
TargetState first_layer;
for(int j = 0; j < 8; j++)
SET_COLOR(first_layer.faces[FACE_UP], j, WHITE);
for(int i = FACE_LEFT; i <= FACE_BACK; i++) {
uint8_t c = i;
SET_COLOR(first_layer.faces[i], TOP_LEFT, c);
SET_COLOR(first_layer.faces[i], TOP, c);
SET_COLOR(first_layer.faces[i], TOP_RIGHT, c);
for(int j = 0; j < 8; j++)
if(j != TOP_LEFT && j != TOP && j != TOP_RIGHT)
first_layer.set_dont_care(i, j);
}
first_layer.set_face_dont_care(FACE_BOTTOM);

moves = Solve_IDFS(given_cube, first_layer, Depth_Limit);
if(!moves.empty()) {
given_cube = Apply_Moves(given_cube, moves);
all_moves += " " + moves;
}

// --- Step 3: Second layer (middle-layer edges) ---
TargetState second_layer;
set_two_layers(second_layer);

moves = Solve_IDFS(given_cube, second_layer, Depth_Limit);
if(!moves.empty()) {
given_cube = Apply_Moves(given_cube, moves);
all_moves += " " + moves;
}

Copilot AI Mar 31, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all_moves is built via all_moves += " " + moves for later stages. If an earlier stage produces no moves but a later one does, this yields a leading space in the returned string. Consider using an append helper that conditionally inserts a separator only when all_moves is non-empty to keep output consistent.

Copilot uses AI. Check for mistakes.
Comment thread Rubix/Inc/Cube.h
Comment on lines 128 to 132
void L_PRIME(uint64_t num_of_turns);
void B_PRIME(uint64_t num_of_turns);
void apply_moves(std::string moves);
void apply_move_index(int move_index);
};

Copilot AI Mar 31, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apply_moves(std::string ...) uses std::string, but Cube.h doesn’t include <string>. This makes the header non-self-contained and can fail to compile when Cube.h is included before any <string> include. Please add #include <string> to Cube.h (or otherwise ensure std::string is declared before use).

Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

@SHONG0504 SHONG0504 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implement move states for slice moves

Comment thread Rubix/src/Cube.cpp
void RubixCube::apply_moves(std::string moves) {
std::istringstream iss(moves);
std::string move;
while (iss >> move) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where 'M', 'E', and 'S' moves?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot gaslight him and disprove the need for these moves and reply back here

@SHONG0504

Copy link
Copy Markdown

Also change 'Rubix' to 'Rubiks'

@ShahriarAhnaf

ShahriarAhnaf commented Apr 1, 2026

Copy link
Copy Markdown
Owner Author

Also change 'Rubix' to 'Rubiks'

                              No 

⠀⠀⠘⡀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⡜⠀⠀⠀
⠀⠀⠀⠑⡀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⡔⠁⠀⠀⠀
⠀⠀⠀⠀⠈⠢⢄⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⣀⠴⠊⠀⠀⠀⠀⠀
⠀⠀⠀⠀⠀⠀⠀⢸⠀⠀⠀⢀⣀⣀⣀⣀⣀⡀⠤⠄⠒⠈⠀⠀⠀⠀⠀⠀⠀⠀
⠀⠀⠀⠀⠀⠀⠀⠘⣀⠄⠊⠁⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀

⣿⣿⣿⣿⣿⣿⣿⣿⡿⠿⠛⠛⠛⠋⠉⠈⠉⠉⠉⠉⠛⠻⢿⣿⣿⣿⣿⣿⣿⣿
⣿⣿⣿⣿⣿⡿⠋⠁⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠉⠛⢿⣿⣿⣿⣿
⣿⣿⣿⣿⡏⣀⠀⠀⠀⠀⠀⠀⠀⣀⣤⣤⣤⣄⡀⠀⠀⠀⠀⠀⠀⠀⠙⢿⣿⣿
⣿⣿⣿⢏⣴⣿⣷⠀⠀⠀⠀⠀⢾⣿⣿⣿⣿⣿⣿⡆⠀⠀⠀⠀⠀⠀⠀⠈⣿⣿
⣿⣿⣟⣾⣿⡟⠁⠀⠀⠀⠀⠀⢀⣾⣿⣿⣿⣿⣿⣷⢢⠀⠀⠀⠀⠀⠀⠀⢸⣿
⣿⣿⣿⣿⣟⠀⡴⠄⠀⠀⠀⠀⠀⠀⠙⠻⣿⣿⣿⣿⣷⣄⠀⠀⠀⠀⠀⠀⠀⣿
⣿⣿⣿⠟⠻⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠶⢴⣿⣿⣿⣿⣿⣧⠀⠀⠀⠀⠀⠀⣿
⣿⣁⡀⠀⠀⢰⢠⣦⠀⠀⠀⠀⠀⠀⠀⠀⢀⣼⣿⣿⣿⣿⣿⡄⠀⣴⣶⣿⡄⣿
⣿⡋⠀⠀⠀⠎⢸⣿⡆⠀⠀⠀⠀⠀⠀⣴⣿⣿⣿⣿⣿⣿⣿⠗⢘⣿⣟⠛⠿⣼
⣿⣿⠋⢀⡌⢰⣿⡿⢿⡀⠀⠀⠀⠀⠀⠙⠿⣿⣿⣿⣿⣿⡇⠀⢸⣿⣿⣧⢀⣼
⣿⣿⣷⢻⠄⠘⠛⠋⠛⠃⠀⠀⠀⠀⠀⢿⣧⠈⠉⠙⠛⠋⠀⠀⠀⣿⣿⣿⣿⣿
⣿⣿⣧⠀⠈⢸⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠟⠀⠀⠀⠀⢀⢃⠀⠀⢸⣿⣿⣿⣿
⣿⣿⡿⠀⠴⢗⣠⣤⣴⡶⠶⠖⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⣀⡸⠀⣿⣿⣿⣿
⣿⣿⣿⡀⢠⣾⣿⠏⠀⠠⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠛⠉⠀⣿⣿⣿⣿
⣿⣿⣿⣧⠈⢹⡇⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⣰⣿⣿⣿⣿
⣿⣿⣿⣿⡄⠈⠃⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⢀⣠⣴⣾⣿⣿⣿⣿⣿
⣿⣿⣿⣿⣧⡀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⢀⣠⣾⣿⣿⣿⣿⣿⣿⣿⣿⣿
⣿⣿⣿⣿⣷⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⢀⣴⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿
⣿⣿⣿⣿⣿⣦⣄⣀⣀⣀⣀⠀⠀⠀⠀⠘⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿
⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣷⡄⠀⠀⠀⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿
⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣧⠀⠀⠀⠙⣿⣿⡟⢻⣿⣿⣿⣿⣿⣿⣿⣿⣿
⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⠇⠀⠁⠀⠀⠹⣿⠃⠀⣿⣿⣿⣿⣿⣿⣿⣿⣿
⣿⣿⣿⣿⣿⣿⣿⣿⡿⠛⣿⣿⠀⠀⠀⠀⠀⠀⠀⠀⢐⣿⣿⣿⣿⣿⣿⣿⣿⣿
⣿⣿⣿⣿⠿⠛⠉⠉⠁⠀⢻⣿⡇⠀⠀⠀⠀⠀⠀⢀⠈⣿⣿⡿⠉⠛⠛⠛⠉⠉
⣿⡿⠋⠁⠀⠀⢀⣀⣠⡴⣸⣿⣇⡄⠀⠀⠀⠀⢀⡿⠄⠙⠛⠀⣀⣠⣤⣤⠄⠀

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.

3 participants