Book 2 8.2 `rotate_y` not necessary to loop through y-value

[jianjianh1]

In rotate_y's constructor, you loop through all three axes to get the maximal bounding box containing the rotated object, but since the rotation is around the y-axis, the y-interval will not change. So looping through y-values can be omitted.

[hollasch]

Duplicate of #819.

[WiHceeg]

Duplicate of #819.

You seem to have misunderstood his meaning, it is not duplicate.
The code in dev-major is like this

    aabb rotate_y(double angle) const {
        auto radians = degrees_to_radians(angle);
        double sin_theta = std::sin(radians);
        double cos_theta = std::cos(radians);

        point3 min( infinity,  infinity,  infinity);
        point3 max(-infinity, -infinity, -infinity);

        for (int i = 0; i < 2; i++) {
            for (int j = 0; j < 2; j++) {
                for (int k = 0; k < 2; k++) {
                    double corner_x = i*x.max + (1-i)*x.min;
                    double corner_y = j*y.max + (1-j)*y.min;
                    double corner_z = k*z.max + (1-k)*z.min;

                    double newx =  cos_theta*corner_x + sin_theta*corner_z;
                    double newz = -sin_theta*corner_x + cos_theta*corner_z;

                    vec3 tester(newx, corner_y, newz);

                    for (int c = 0; c < 3; c++) {
                        min[c] = std::fmin(min[c], tester[c]);
                        max[c] = std::fmax(max[c], tester[c]);
                    }
                }
            }
        }

        return aabb(min, max);
    }/

The y-interval will not change, the triple loop can be optimized into a double loop.

    aabb rotate_y(double angle) const {
        auto radians = degrees_to_radians(angle);
        double sin_theta = std::sin(radians);
        double cos_theta = std::cos(radians);

        point3 min( infinity, y.min,  infinity);
        point3 max(-infinity, y.max, -infinity);

        for (int i = 0; i < 2; i++) {
            for (int k = 0; k < 2; k++) {
                double corner_x = i*x.max + (1 - i)*x.min;
                double corner_z = k*z.max + (1 - k)*z.min;

                double newx =  cos_theta*corner_x + sin_theta*corner_z;
                double newz = -sin_theta*corner_x + cos_theta*corner_z;

                min[0] = std::fmin(min[0], newx);
                max[0] = std::fmax(max[0], newx);
                min[2] = std::fmin(min[2], newz);
                max[2] = std::fmax(max[2], newz);
            }
        }
        return aabb(min, max);
    }

[hollasch]

I'm not sure what else to say here, but ok:

• I referenced issue aabb class should manage AABB rotation, not rotate_y #819, which covers the entire deletion of the code above, so "improving it" is pointless.
• This includes reference to [Book 2, Chapter 8.2] Use range-based for-loop #1482, which covers changes to the triply-nested looping (yes, including removal of the Y iteration), and considering the usage of C++ ranges for clarity.
• Issue aabb class should manage AABB rotation, not rotate_y #819 (the rearchitecting of rotate_y::rotate_y()) is currently tagged with in progress, which I think clearly indicates that other developers currently own this work.

This is why our contribution guidelines include step 3 for PRs:

3. Participate in the discussion as needed. We'll ensure that the work doesn't conflict with or duplicate other work planned or in progress, and decide which development branch is correct for the release type and release schedule.

As always, we encourage you to implement your project exactly as you see fit, so definitely feel free to adopt these changes in your project. We are making corresponding changes in the books.

[WiHceeg]

I apologize for the confusion. I found this function already existing as a member of the aabb class in a dev branch and assumed it was the latest code. I was unaware of other ongoing plans regarding this implementation. Thank you for clarifying.

[hollasch]

No problem — I understand it's a bit confusing. Our backlog is fairly deep and complicated by the various release types. Of course, adding this guidance to the GitHub PR notifications is also on our backlog. 😄