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.

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.

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--;
}