Rubik's Cube rotation with OpenGL

Question

I've been trying to make a rubik's cube project and it succeeds nicely . The only problem(or something like that) is that i need to optimize my code for easier understanding and more flexibility. Here's my code to optimize:

[...]//Code for creating cubes
int main()
{
[...]//some extra codes
 while (!glfwWindowShouldClose(window))
 {
[...]//more extra codes
 cubes.DrawCubes(view, projection);
 chk:
 if (rSpeed > 90)
 {
 rSpeed = 90;
 }
 if (rSpeed < 1)
 {
 rSpeed = 1;
 }
 if (90 % rSpeed != 0)
 {
 if (inc)
 rSpeed++;
 else if (dec)
 rSpeed--;
 goto chk;
 }
 if (cubes.Rot_speed != rSpeed)
 {
 cubes.SetCubeRotationSpeed(rSpeed);
 }
 if (RotateX)
 {
 if (rot_x < 90 / rSpeed)
 {
 cubes.Rotate(0);
 rot_x++;
 cubes.Rotating = true;
 RotateY = false;
 RotateZ = false;
 }
 else
 {
 rot_x = 0;
 RotateX = false;
 cubes.Rotating = false;
 }
 }
 if (RotateY)
 {
 if (rot_y < 90 / rSpeed)
 {
 cubes.Rotate(1);
 rot_y++;
 cubes.Rotating = true;
 RotateX = false;
 RotateZ = false;
 }
 else
 {
 rot_y = 0;
 RotateY = false;
 cubes.Rotating = false;
 }
 }
 if (RotateZ)
 {
 if (rot_z < 90 / rSpeed)
 {
 cubes.Rotate(2);
 rot_z++;
 cubes.Rotating = true;
 RotateY = false;
 RotateX = false;
 }
 else
 {
 rot_z = 0;
 RotateZ = false;
 cubes.Rotating = false;
 }
 }
[...]//more extra codes
 }
}

where rSpeed and cubes.Rot_speed refers to angles for rotation speeds. The places where I kept [...] are codes unrelated to my question. So please don't mind those .

On function cubes.Rotate():

 void x_Cubes::Rotate(int axis)
 {
 if (axis == 0)
 {
 for (auto& mx : m_cubes)
 {
 if (mx.cellx == m_HCube.cellx)
 mx.RotateX();
 }
 }
 else if (axis == 1)
 {
 for (auto& mx : m_cubes)
 {
 if (mx.celly == m_HCube.celly)
 mx.RotateY();
 }
 }
 else if (axis == 2)
 {
 for (auto& mx : m_cubes)
 {
 if (mx.cellz == m_HCube.cellz)
 mx.RotateZ();
 }
 }
 }

m_cubes is just a vector of cube data

And the last piece of code :

 void _Rotate(glm::mat4& mat, float ang_x, float ang_y, float ang_z)
 {
 glm::mat4 transformX = glm::mat4(1.0f);
 glm::mat4 transformY = glm::mat4(1.0f);
 glm::mat4 transformZ = glm::mat4(1.0f);
 transformX = glm::rotate(transformX, glm::radians(ang_x), glm::vec3(1.0f, 0.0f, 0.0f));
 transformY = glm::rotate(transformY, glm::radians(ang_y), glm::vec3(0.0f, 1.0f, 0.0f));
 transformZ = glm::rotate(transformZ, glm::radians(ang_z), glm::vec3(0.0f, 0.0f, 1.0f));
 mat = transformX * transformY * transformZ * mat;
 }
 void Cube::RotateX()
 {
 rot_angle_X++;
 if (rot_angle_X < (90.0f / rot_speed))
 _Rotate(model, rot_speed, 0.0f, 0.0f);
 else
 {
 rot_angle_X = 0.0f;
 _Rotate(model, rot_speed, 0.0f, 0.0f);
 }
 if (rot_angle_X == 90.0f || rot_angle_X == 0.0f)
 SwapCellVals(0);
 }
 void Cube::RotateY()
 {
 rot_angle_Y++;
 if (rot_angle_Y < (90.0f / rot_speed))
 _Rotate(model, 0.0f, rot_speed, 0.0f);
 else
 {
 rot_angle_Y = 0.0f;
 _Rotate(model, 0.0f, rot_speed, 0.0f);
 }
 if (rot_angle_Y == 90.0f || rot_angle_Y == 0.0f)
 SwapCellVals(1);
 }
 void Cube::RotateZ()
 {
 rot_angle_Z++;
 if (rot_angle_Z < (90.0f / rot_speed))
 _Rotate(model, 0.0f, 0.0f, rot_speed);
 else
 {
 rot_angle_Z = 0.0f;
 _Rotate(model, 0.0f, 0.0f, rot_speed);
 }
 if (rot_angle_Z == 90.0f || rot_angle_Z == 0.0f)
 SwapCellVals(2);
 }

Here, x_Cubes is a class that has a vector for storing data of class Cube. The code is too long for rotation and the Booleans are too many for a rotation function. I need to reduce the codes so that i only need to call the function on the main block and flags are only in the rotate function of Cube . I'm using shaders for the drawing actually so i dont know how to actually show the animations properly by reducing the codes.

• Deduplicator: My compiler complains when compiling [...]. Could you replace it with the actual code? If it is too much to fit, at least link it somewhere else?
• Shreeyash Shrestha: i just used [...] so that i dont need to add other codes for my question. the codes in [...] are unrelated to the question so i didn't keep those

Answer 1

Use an enum class to represent the axes

Instead of using an int, declare an enum class to represent the axes of rotation:

enum class Axis {X, Y, Z};

Also use switch statements instead of multiple if-statements, like so:

void x_Cubes::Rotate(Axis axis)
{
 switch (axis) {
 case Axis::X:
 for (auto& mx : m_cubes)
 if (mx.cellx == m_HCube.cellx)
 mx.RotateX();
 break;
 case Axis::Y:
 ...
 break;
 case Axis::Z:
 ...
 break;
 }
}

Compilers will be able to warn if you forgot to check for all possible values of axis if you write it like that.

Avoid repeating yourself

Having duplicate code to deal with each of the three axes is making the code longer, harder to read and harder to maintain. Ideally, you should write the code in such a way that the axis you want to manipulate is just a parameter, and the code is written in such a way that you completely avoid needing something like switch (axis). For example, you want something like this to replace the three individual Cube::Rotate*() functions:

void Cube::Rotate(Axis axis)
{
 auto index = static_cast<int>(axis);
 rot_angle[index]++;
 if (rot_angle[index] >= (90.0f / rot_speed)) {
 rot_angle[index] = 0.0f;
 SwapCellVals(index);
 }
 glm::vec3 rot_vec{};
 rot_vec[index] = rot_speed;
 _Rotate(model, rot_vec);
}

Basically, make everything storing information about each axis indexable, for example by just storing them in an array, or using types that support indexing, like GLM's vector types do. Do this for all the other functions. For example:

void _Rotate(glm::mat4& mat, glm::vec3 rot_vec)
{
 for (int i = 3; i--;)
 mat = glm::rotate(mat, glm::radians(rot_vec[i]));
 }
}

But since you only rotate one axis at a time, the for-loop seems unnecessary, you can just replace it with mat = glm::rotate(math, rot_vec). And then you can just do this in Cube::Rotate() itself, and remove _Rotate().

Rotation angle and speed

It's a bit weird to see rot_angle being increased by one, and then being compared to 90.0f / rot_speed. Why not just add rot_speed to rot_angle?

void Cube::Rotate(Axis axis)
{
 auto index = static_cast<int>(axis);
 rot_angle[index] += rot_speed;
 if (rot_angle[index] >= 90.0f) {
 ...

Also be aware that floating point errors might accumulate over time, so repeatedly applying small rotations to model might eventually cause it to get into an undesired state. I suggest that when finishing a 90 degree rotation of the cube, you recalculate model from scratch.

Use radians everywhere

You can avoid the conversion between degrees and radians by just doing everything in radians. It's really not that hard, and it will clean up your code and make it a bit more efficient.

• Shreeyash Shrestha: Thanks for the suggestion . The only thing I found uneasy was the rotation speed in this . I did not increase the rotation speed with rotation angle . I used the increment function so that the numbers are divisible to 90 and I could get a proper rotation ending .

Answer 2

Use enumss

I totally agree with G. Sliepen, but I should add one more detail: add also None variant, so RotateX/RotateY/RotateZ variables could be represented with only one variable.

Don't put everything in main function

You will probably need to reuse the code rotating the cube - like adding another cube of something. But when everything happens in main, it is hard to refactor. Create additional class for cube rotations and move all the rotation code there. Use main only to setup the application, start it and handle exit.

Instead of two always opposite bool variables, use one

It looks inc and dec are designed to be always opposite, or you'll face an endless loop. Remove one of them, say dec; if you need to check specifically dec, just use !inc, but here just use else.

Avoid goto

Here, all you need is a loop:

while(true)
{
 if (rSpeed > 90)
 {
 rSpeed = 90;
 }
 if (rSpeed < 1)
 {
 rSpeed = 1;
 }
 if (90 % rSpeed != 0)
 {
 if (inc)
 rSpeed++;
 else
 rSpeed--;
 //here we need to repeat the loop - so we'll break it on other branch
 }
 else
 {
 break;
 }
}

but wait, can rSpeed change to break the margins in a loop? If it reaches 1 or 90, the loop will stop. We can move checks out of the loop, and also use std::max and std::min to prettify them:

rSpeed = std::min(rSpeed, 90);
rSpeed = std::max(rSpeed, 1);
while(true)
{
 if (90 % rSpeed != 0)
 {
 if (inc)
 rSpeed++;
 else
 rSpeed--;
 }
 else
 {
 break;
 }
}

Do you see now? if checks the condition for the loop to continue - so we can move it into while and remove break branch:

rSpeed = std::min(rSpeed, 90);
rSpeed = std::max(rSpeed, 1);
while(90 % rSpeed != 0)
{
 if (inc)
 rSpeed++;
 else
 rSpeed--;
}

• Deduplicator: en.cppreference.com/w/cpp/algorithm/clamp
• Shreeyash Shrestha: @Pavlo Slavynskyy I agree with you. If possible will you help in reducing(or making efficient) the codes in rotateX/rotateY/rotateZ flags in main function since those are the main hindrances for me right now. the suggestions you and G. Sliepen gave are nice and really helped me in reducing the codes and gave me a better understanding on making code smaller and efficient I cant seem to reduce the codes on those flags and if i do somehow, the whole program doesnt seem to work like I wanted. please help if you can.
• Pavlo Slavynskyy: Add the enum and rewrite the code, you will see where you can reduce it.
• Shreeyash Shrestha: yeah i edited some places and it did seem short and still works the same. Thanks for mentioning that