Added Ellipsoid Geometry #203
Conversation
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #203 +/- ##
=======================================
Coverage 57.01% 57.01%
=======================================
Files 151 151
Lines 15010 15010
=======================================
Hits 8558 8558
Misses 6452 6452
Continue to review full report at Codecov.
|
Signed-off-by: ahcorde <[email protected]>
|
Depends on gazebosim/gz-common#154 |
ahcorde
added
the
needs upstream release
Signed-off-by: ahcorde <[email protected]>
ogre2/src/Ogre2Scene.cc
Outdated
| EllipsoidPtr Ogre2Scene::CreateEllipsoidImpl(unsigned int _id, | ||
| const std::string &_name) | ||
| { | ||
| Ogre2EllipsoidPtr ellipsoid(new Ogre2Ellipsoid); |
There was a problem hiding this comment.
I'm curious, why did you create an Ogre2Ellipsoid class instead of following the same pattern as sphere, cylinder, box, plane, and using CreateMeshImpl?
There was a problem hiding this comment.
I created an implementation without having a look how the basic shapes were done. I can remove the code in both shapes (capsule and ellipsoid) probably is going to be easy to maintain.
There was a problem hiding this comment.
@iche033 any thoughts?
There was a problem hiding this comment.
+1 on using CreateMeshImpl. We could add a unit_ellipsoid in ign-common and make use of that here in ign-rendering.
Signed-off-by: ahcorde <[email protected]>
ogre/src/OgreScene.cc
Outdated
| MeshPtr OgreScene::CreateEllipsoidImpl( | ||
| unsigned int _id, const std::string &_name) | ||
| { | ||
| return this->CreateMeshImpl(_id, _name, "unit_ellipsoid"); |
There was a problem hiding this comment.
As discussed in gazebosim/gz-common#165, this could just be unit_sphere
examples/simple_demo/Main.cc
Outdated
| // create ellipsoid visual | ||
| VisualPtr ellipsoidVisual = _scene->CreateVisual(); | ||
| auto ellipsoid = _scene->CreateEllipsoid(); | ||
| ellipsoid->SetRadii(ignition::math::Vector3d(1.2, 0.7, 0.5)); |
There was a problem hiding this comment.
This example needs to be updated now that there's no ellipsoid class. The ellipsoid's size will be defined using SetLocalScale, the same way as the sphere above.
Which brings the question of whether special API is needed for the ellipsoid at all. Despite its name, ign-rendering's "sphere" already behaves like an "ellipsoid", because it can be scaled in 3 dimensions.
At this point, my suggestion would be to:
- remove all the ellipsoid logic added in this PR
- update
CreateSphere's documentation to say that it can be used to create ellipsoids - update this example to create an "ellipsoid" through the
CreateSpherefunction
There was a problem hiding this comment.
Thank you for the bullet points 👷
Signed-off-by: ahcorde <[email protected]>
ahcorde
force-pushed
the
ahcorde/feature/ellipsoid
branch
from
19cb831 to
f6f5868
Compare
chapulina
removed
the
needs upstream release
Added Ellipsoid geometry. Related with this issue #198
Ogre 2
Ogre:
Signed-off-by: ahcorde [email protected]