sofa-framework / sofa

Real-time multi-physics simulation with an emphasis on medical simulation.
https://www.sofa-framework.org
GNU Lesser General Public License v2.1
871 stars 297 forks source link

[SofaKernel] ADD RotatableBoxROI object and test for it #560

Closed sergeiNikolaev closed 6 years ago

sergeiNikolaev commented 6 years ago

A modification of BoxROI object that could be rotated using given angle values and a test for it. The values of angles are defined like in applyRotation method where Euler Angle values in degrees are used.


This PR:

Reviewers will merge only if all these checks are true.

sofabot commented 6 years ago

Thank you for your pull request! Someone will soon check it and start the builds.

Note that by submitting a contribution to SOFA, you hereby accept and agree to the terms and conditions detailed in the associated CLA

hugtalbot commented 6 years ago

There was also the OBBModel in SOFA for OrientedBoundingBox but I don't think there is a ROI function in it. Thank you @sergeiNikolaev

[ci-build][with-scene-tests]

damienmarchal commented 6 years ago

Thank you sergeiNikolaev, for the compontent and for the tests :)

I was writing a long answer...then I looked at the BoxROI implementation and noticed it already implemented axis aligned box and oriented box with the 'orientedBox' parameter.

Maybe it does not work for you and there is bug or it doesn't match what you wanted to do in that case can you provide more details please.

sergeiNikolaev commented 6 years ago

The main problem for me is that it is quite hard to set the right position for the BoxROI in space using three points, which are used in orientedBox. It is much easier to set the right translation for the box and than the right angles for rotation, checking it visually in SOFA. So it would be nice to add the possibility to set translation and rotations parameters separately for oriented boxes in BoxROI component like for mechanical objects. But of course, it would be better to add new functional in the BoxROI component instead of creating the new class.

damienmarchal commented 6 years ago

@sergeiNikolaev

I think your problem is interesting and is an important issue of Sofa usability. If you agree and other interested we could discuss here what ones can come up with possible solution (ie having more clear/simpler way to define component's properties).

damienmarchal commented 6 years ago

PSL version.

I'm not really sure but I think that with the Template mecanism of PSL (https://github.com/sofa-framework/sofa/tree/master/applications/plugins/PSL#templates)

Maybe it is possible to come up with a solution looking like that:


 Node : {
        Import : "pythonlib"

        // The implementation that create a BoxROI and transform the parameters from
        // Use friendly to the one used in the component. 
        Template : {
                name : MyBoxROI
                properties : { 
                        position : "0 0 0"
                         orientation : "0 0 0"
                         dimmension: "10 10 10 "
               }
               BoxROI : {
                     // The convertToOrientedBox is implemented in the "pythonlib" module.
                     orienteBox : convertToOrientedBox(position, orientation, dimmension)
               } 
        }

       /// One instance of the boxroi.
       MyBoxROI  : {
             position : "1 2 3"
             orientation : "0.5 0.5 0.5"
             dimmension : "3 4 5"
       }

       /// A second instance of the boxroi.
       MyBoxROI  : {
             position : "0 2 3"
             orientation : "0.5 0.5 0.5"
             dimmension : "7 4 5"
       }

 }
damienmarchal commented 6 years ago

One alternative solution is, in c++, to add new Data fields into the component and in the function BoxROI::init converts these fields values into the one used internally by BoxROI to do the computation.

The drawback is that adding Data field are costly at component creation time and this can have a big performance cost in Sofa for component that are created in a loop or something.

damienmarchal commented 6 years ago

A third solution, still in c++, to parse the added field into the function BoxROI::parse() and converts these fields values into the DataField used internally by BoxROI to do the computation.

The good is that there is no performance cost. The drawback are the following:

damienmarchal commented 6 years ago

Another solution could be to implement a per-component piece of GUI elements so that in the GUI we have customs way to enter the values. This is a WIP in runSofa2.

sergeiNikolaev commented 6 years ago

Hm... The problem for now is that I don't know who and what way uses this BoxROI component. And how these changes affect their performance. For me it is ok to have it not in real-time.

sergeiNikolaev commented 6 years ago

And also I guess that for different people there are different ways to define the component the most usable way. So, I am afraid there won't be unique solution for this problem.

damienmarchal commented 6 years ago

Have have never used it...let's look at the code :)

, d_orientedBoxes( initData(&d_orientedBoxes, "orientedBox", "List of boxes defined by 3 points (p0, p1, p2) and a depth distance \n"
                                "A parallelogram will be defined by (p0, p1, p2, p3 = p0 + (p2-p1)). \n"
                                "The box will finaly correspond to the parallelogram extrusion of depth/2 \n"
                                "along its normal and depth/2 in the opposite direction. ") )

The documentation is totally unclear... This should be a 10 float vector... and here is what happens

       Vec10 box = orientedBoxes[i];
        Vec3 p0 = Vec3(box[0], box[1], box[2]);
        Vec3 p1 = Vec3(box[3], box[4], box[5]);
        Vec3 p2 = Vec3(box[6], box[7], box[8]);
        double depth = box[9];
        Vec3 normal = (p1-p0).cross(p2-p0);
        normal.normalize();
        Vec3 p3 = p0 + (p2-p1);
        Vec3 p6 = p2 + normal * depth;
        Vec3 plane0 = (p1-p0).cross(normal);
        plane0.normalize();
        Vec3 plane1 = (p2-p3).cross(p6-p3);
        plane1.normalize();
        Vec3 plane2 = (p3-p0).cross(normal);
        plane2.normalize();
        Vec3 plane3 = (p2-p1).cross(p6-p2);
        plane3.normalize();
        m_orientedBoxes[i].p0 = p0;
        m_orientedBoxes[i].p2 = p2;
        m_orientedBoxes[i].normal = normal;
        m_orientedBoxes[i].plane0 = plane0;
        m_orientedBoxes[i].plane1 = plane1;
        m_orientedBoxes[i].plane2 = plane2;
        m_orientedBoxes[i].plane3 = plane3;
        m_orientedBoxes[i].width = fabs(dot((p2-p0),plane0));
        m_orientedBoxes[i].length = fabs(dot((p2-
sergeiNikolaev commented 6 years ago

The 10 float vector is just for initialising the whole object. As I understood they do it like this: first of all for three given points (p0, p1, p2), which define some initial plane, they create another point p3 on the same plane and point p6 that is shifted from point p2 using the normal of initial plane. object

sergeiNikolaev commented 6 years ago

Then they define four planes using points (p0, p1, p2, p3, and normal to initial plane). planes

sergeiNikolaev commented 6 years ago

And finally they define width and length distances to planes as projection of object diagonal on the normals of these planes. distances

sergeiNikolaev commented 6 years ago

Then in method isPointInOrientedBox they calculate difference between given point and two object points , project these differences on the constructed and initial plane normals and verify if the distances is less then specified. If this is the case the point is inside the Box object.

I havn't verified this approach but technically it should work.

damienmarchal commented 6 years ago

Hello Serguei,

Thanks for the clear explaination. This may actually be part of the BoxROI documentation. Now back to your problem...what do you think about implementing a python function to convert from (position, orientation, size) would generates these 10 values ? Is it enough / not enough ?

sergeiNikolaev commented 6 years ago

Yes, that's ok for me and I guess that will be enough.
Are you talking about separate python script or some python component in SOFA?

damienmarchal commented 6 years ago

Hi Sergei,

Sorry to reply that late. So to answer your question... I think a separate python script is fine.

To ease the sharing of those utilitary python script we (at Defrost) have started a dedicated library called STLIB (for Sofa Template Library). I would actually be interested to add your OrientedBoxROI there.

You can find more information by browsing the auto-generated documentation at: http://stlib.readthedocs.io/en/latest/index.html

While the plugin is there: https://github.com/SofaDefrost/STLIB

It is a work in progress but as we are now basing our other plugins examples and scenes to this I hope it to grow fast.