you've resolved a lot of comments but I don't see the corresponding changes in the code

forgot to push :D

Codecov Report

Merging #224 (ee7f156) into ign-math6 (b4b6d23) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           ign-math6     #224   +/-   ##
==========================================
  Coverage      99.21%   99.22%           
==========================================
  Files             65       66    +1     
  Lines           6089     6162   +73     
==========================================
+ Hits            6041     6114   +73     
  Misses            48       48           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b4b6d23...ee7f156. Read the comment docs.

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/gsoc-2021-machine-learning-extension-to-ignition-gazebo/1070/1

I'm trying to do a few verifications for testing

I went ahead and ran the example set of points in the unit test through Open3D's CreateFromPoints method for comparison. Here are the results I got (link to the code can be found here):

Box center is: 
0.423213
0.507995
 1.83459

Box extent (size) is: 
6.53071
4.12782
3.25527

roll: -2.01634
pitch: 0.0058244
yaw: 1.1554

It looks like the box's center and size are similar enough between Open3D and this PR, but the rotation seems like it could be an issue. Of course, this is assuming that Open3D should be considered a "source of truth", but I just wanted to mention it here in case this deserves further investigation. The code I linked shows how I compute roll, pitch and yaw from the rotation matrix that is provided by Open3D (hopefully my calculations are correct). cc @scpeters

@adlarkin
I thinks that is because the part of calculating the orientation in the 2 algorithms is different

their code

    Eigen::Matrix3d R = es.eigenvectors();
    R.col(0) /= R.col(0).norm();
    R.col(1) /= R.col(1).norm();
    R.col(2) /= R.col(2).norm();

    if (evals(1) > evals(0)) {
        std::swap(evals(1), evals(0));
        Eigen::Vector3d tmp = R.col(1);
        R.col(1) = R.col(0);
        R.col(0) = tmp;
    }
    if (evals(2) > evals(0)) {
        std::swap(evals(2), evals(0));
        Eigen::Vector3d tmp = R.col(2);
        R.col(2) = R.col(0);
        R.col(0) = tmp;
    }
    if (evals(2) > evals(1)) {
        std::swap(evals(2), evals(1));
        Eigen::Vector3d tmp = R.col(2);
        R.col(2) = R.col(1);
        R.col(1) = tmp;
    }

But anyway I see them are close (not too much close) but they have rotation in the same directions at least .. and also the rotation is acceptable visually, so we don't need to compare it with Open3D

That is our output for multi-links models (double pendolum and cars)

@adlarkin
I thinks that is because the part of calculating the orientation in the 2 algorithms is different

their code

    Eigen::Matrix3d R = es.eigenvectors();
    R.col(0) /= R.col(0).norm();
    R.col(1) /= R.col(1).norm();
    R.col(2) /= R.col(2).norm();

    if (evals(1) > evals(0)) {
        std::swap(evals(1), evals(0));
        Eigen::Vector3d tmp = R.col(1);
        R.col(1) = R.col(0);
        R.col(0) = tmp;
    }
    if (evals(2) > evals(0)) {
        std::swap(evals(2), evals(0));
        Eigen::Vector3d tmp = R.col(2);
        R.col(2) = R.col(0);
        R.col(0) = tmp;
    }
    if (evals(2) > evals(1)) {
        std::swap(evals(2), evals(1));
        Eigen::Vector3d tmp = R.col(2);
        R.col(2) = R.col(1);
        R.col(1) = tmp;
    }

That was the output when I tried that code of Open3D for calculating Rotation instead of the current algorithm
It was bad

have we considered specifying the orientation of the bounding box and then simply expanding its dimensions to include all the passed vertices? this would avoid the weird box rotations at the cost of generating non-minimal boxes

have we considered specifying the orientation of the bounding box and then simply expanding its dimensions to include all the passed vertices? this would avoid the weird box rotations at the cost of generating non-minimal boxes

I think the challenge is that in some scenarios, we have the vertices of an object, but not the orientation of the object. For example, in gazebosim/gz-rendering#334, I believe that we have all of the vertices from the bounding boxes provided by Ogre, but I don't believe that we have a way of getting the "combined orientation" from all of the rendering visuals (I could be wrong about this).

However, that being said, I think that it would be good to provide users with the option to pass in an orientation to verticesToOrientedBox. Perhaps we can add a input parameter that's a pointer to an orientation: if the pointer is null, we ignore it and run the algorithm as-is, but if it isn't null, then we take the orientation into account. Does that sound reasonable?

However, that being said, I think that it would be good to provide users with the option to pass in an orientation to verticesToOrientedBox. Perhaps we can add a input parameter that's a pointer to an orientation: if the pointer is null, we ignore it and run the algorithm as-is, but if it isn't null, then we take the orientation into account. Does that sound reasonable?

I would just add a separate API that accepts a vector of vertices as well as a Quaternion, but this PR is fine without it. We could add it later if desired